Libguestfs - Sep 2014 - Re: [RFC PATCH] resize: add support for MBR logical partitions some question

On Tue, Jul 15, 2014 at 09:01:47AM +0100, Richard W.M. Jones
wrote:
> The answer is I don't know.  But there are a few things you can try:
> 
> (1) Most importantly, enable tracing (export LIBGUESTFS_TRACE=1) and
> get a list of operations that are performed in the order they are
> performed.  This is vital for debugging this.
> 
> (2) When the error happens, run `lsof'.  Something like this:
> 
>   try
>     g#part_add "/dev/sdb" (mbr_part_type p) p.p_target_start
p.p_target_end;
>     if p.p_type = ContentExtendedPartition then
>       List.iter (
>         fun p ->
>           g#part_add "/dev/sdb" "logical"
p.p_target_start p.p_target_end
>       ) p.p_partitions
>   with
>     G.Error msg ->
>       eprintf "lsof:\n---\n%s\n---\n" (g#debug "sh" [|
"lsof" |]);
>       eprintf "original error:\n" msg;
>       exit 1
> 
> This should tell you which processes have partitions open, which
> should give the reason why the kernel cannot reread the partition
> table.
> 
> (3) "Rebooting the appliance" means reopening the libguestfs
handle,
> which virt-resize already does at least once.  See the comment:
> 
>   (* After copying the data over we must shut down and restart the
>    * appliance in order to expand the content.  The reason for this may
>    * not be obvious, but it's because otherwise we'll have
duplicate VGs
>    * (the old VG(s) and the new VG(s)) which breaks LVM.
>    *
>    * The restart is only required if we're going to expand something.
>    *)
> 
> I hope we don't have to do it again, but it might be necessary based
> on the full analysis of (1) & (2).
> 
> Rich.
Thanks, your suggestions are helpful, I'll have a try.

Regards,
Hu

Hi,

The attached patch adds support for resizing MBR logical partitions. The
failure is still there, I can't get any helpful information from lsof.
Any suggestions?
>From b30a847db88e5ce75aa04d656d05b6cdb01e54fd Mon Sep 17 00:00:00 2001
From: root <root@localhost.localdomain>
Date: Mon, 2 Jun 2014 23:30:57 -0400
Subject: [PATCH] resize: add support for MBR logical partitions

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 resize/resize.ml | 243 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 202 insertions(+), 41 deletions(-)

diff --git a/resize/resize.ml b/resize/resize.ml
index 9614ec7..a476076 100644
--- a/resize/resize.ml
+++ b/resize/resize.ml
@@ -50,6 +50,10 @@ type partition = {
   p_id : partition_id;           (* Partition (MBR/GPT) ID. *)
   p_type : partition_content;    (* Content type and content size. *)
   p_label : string option;       (* Label/name. *)
+  p_part_num: int;               (* partition number *)
+
+  mutable p_partitions : partition list; (* MBR logical partitions. Non-empty
+                                            list implies extended partition *)
 
   (* What we're going to do: *)
   mutable p_operation : partition_operation;
@@ -90,7 +94,8 @@ let rec debug_partition p      (match p.p_label with
     | Some label -> label
     | None -> "(none)"
-    )
+    );
+  List.iter debug_partition p.p_partitions
 and string_of_partition_content = function
   | ContentUnknown -> "unknown data"
   | ContentPV sz -> sprintf "LVM PV (%Ld bytes)" sz
@@ -443,6 +448,14 @@ read the man page virt-resize(1).
     if List.length parts = 0 then
       error (f_"the source disk has no partitions");
 
+    let logical_partitions +      match parttype with
+      | GPT -> []
+      | MBR ->
+          List.filter (fun ({ G.part_num = part_num }) ->
+            part_num >= 5_l
+          ) parts in
+
     (* Filter out logical partitions.  See note above. *)
     let parts        match parttype with
@@ -453,10 +466,8 @@ read the man page virt-resize(1).
         | _ -> true
         ) parts in
 
-    let partitions -      List.map (
-        fun ({ G.part_num = part_num } as part) ->
-          let part_num = Int32.to_int part_num in
+    let to_partition part +          let part_num = Int32.to_int
part.G.part_num in
           let name = sprintf "/dev/sda%d" part_num in
           let bootable = g#part_get_bootable "/dev/sda" part_num in
           let id @@ -476,13 +487,58 @@ read the man page virt-resize(1).
 
           { p_name = name; p_part = part;
             p_bootable = bootable; p_id = id; p_type = typ;
-            p_label = label;
+            p_label = label; p_part_num = part_num;
+            p_partitions = [];
             p_operation = OpCopy; p_target_partnum = 0;
             p_target_start = 0L; p_target_end = 0L }
+    in
+
+    let logical_partitions +      List.map (
+        fun part -> to_partition part
+          ) logical_partitions in
+
+    let logical_partitions_num = List.length logical_partitions in
+
+    let partitions +      List.map (
+        fun part -> to_partition part
       ) parts in
 
+    let extended_partitions +      match parttype with
+      | GPT -> []
+      | MBR ->
+          List.filter (fun ({ p_type = typ }) ->
+            typ = ContentExtendedPartition
+          ) partitions in
+
+
+    (* Filter out extended partition. *)
+    let partitions +      match parttype with
+      | GPT -> partitions
+      | MBR ->
+        List.filter (fun ({ p_type = typ }) ->
+          typ <> ContentExtendedPartition
+        ) partitions in
+
+    assert ((List.length extended_partitions) <= 1);
+
+    let cmp_partition parta partb +      if parta.p_part_num <
partb.p_part_num then -1
+      else if parta.p_part_num = partb.p_part_num then 0
+      else 1 in
+
+    List.iter (fun part ->
+      part.p_partitions <- List.merge cmp_partition part.p_partitions
logical_partitions
+    ) extended_partitions;
+
+    let partitions = List.merge cmp_partition partitions extended_partitions in
+
     if verbose then (
-      eprintf "%d partitions found\n" (List.length partitions);
+      eprintf "%d partitions found\n" ((List.length partitions) +
+      logical_partitions_num);
       List.iter debug_partition partitions
     );
 
@@ -506,14 +562,27 @@ read the man page virt-resize(1).
     ) partitions;
 
     (* Check partitions don't overlap. *)
-    let rec loop end_of_prev = function
+    let rec loop end_of_prev prev_typ = function
+      | [] -> ()
+      | { p_name = name; p_part = { G.part_start = part_start }; p_type = typ }
:: _
+          when end_of_prev > part_start && prev_typ <>
ContentExtendedPartition ->
+              error (f_"%s: this partition overlaps the previous
one") name
+      | { p_part = { G.part_end = part_end }; p_type = typ } :: parts ->
loop part_end typ parts
+    in
+    loop 0L ContentUnknown partitions;
+
+    (* check logical partitions don't overlap *)
+    let rec loop end_of_prev extended_part = function
       | [] -> ()
-      | { p_name = name; p_part = { G.part_start = part_start } } :: _
-          when end_of_prev > part_start ->
+      | {p_name = name; p_part = {G.part_start = part_start } } :: _
+      when end_of_prev > part_start ->
         error (f_"%s: this partition overlaps the previous one") name
-      | { p_part = { G.part_end = part_end } } :: parts -> loop part_end
parts
+      | { p_part = { G.part_end = part_end; G.part_start = part_start } } ::
parts ->
+          loop part_end extended_part parts
     in
-    loop 0L partitions;
+    List.iter (fun part ->
+      loop part.p_part.G.part_start part part.p_partitions
+    ) extended_partitions;
 
     partitions in
 
@@ -578,6 +647,12 @@ read the man page virt-resize(1).
     let hash = Hashtbl.create 13 in
     List.iter (fun ({ p_name = name } as p) -> Hashtbl.add hash name p)
       partitions;
+    List.iter (fun p ->
+      if p.p_type = ContentExtendedPartition then (
+        List.iter (fun ({ p_name = name } as p) -> Hashtbl.add hash name p)
+        p.p_partitions
+      )
+    ) partitions;
     fun ~option name ->
       let name          if String.length name < 5 || String.sub name 0 5
<> "/dev/" then
@@ -692,6 +767,26 @@ read the man page virt-resize(1).
   List.iter (do_resize ~option:"--resize") resizes;
   List.iter (do_resize ~option:"--resize-force" ~force:true)
resizes_force;
 
+  (* handle resizing of logical partitions *)
+  List.iter (
+    fun p ->
+      if p.p_type = ContentExtendedPartition then (
+        let sectsize = Int64.of_int sectsize in
+        let size = roundup64 p.p_part.G.part_size sectsize in
+        let logical_sizes = List.fold_left (
+          fun total p ->
+            match p.p_operation with
+              | OpDelete -> total +^ 0L
+              | OpCopy | OpIgnore -> total +^ p.p_part.G.part_size
+              | OpResize newsize -> total +^ newsize
+          ) 0L p.p_partitions in
+        if logical_sizes > size then
+          p.p_operation <- OpResize logical_sizes
+        (* don't touch the extended partition if logical sizes less
+         * then the original size *)
+      )
+  ) partitions;
+
   (* Helper function calculates the surplus space, given the total
    * required so far for the current partition layout, compared to
    * the size of the target disk.  If the return value >= 0 then it's
@@ -816,29 +911,31 @@ read the man page virt-resize(1).
     printf "**********\n\n";
     printf "Summary of changes:\n\n";
 
-    List.iter (
-      fun ({ p_name = name; p_part = { G.part_size = oldsize }} as p) ->
+    let rec print_summary p          let text            match p.p_operation
with
           | OpCopy ->
-              sprintf (f_"%s: This partition will be left alone.")
name
+              sprintf (f_"%s: This partition will be left alone.")
p.p_name
           | OpIgnore ->
-              sprintf (f_"%s: This partition will be created, but the
contents will be ignored (ie. not copied to the target).") name
+              sprintf (f_"%s: This partition will be created, but the
contents will be ignored (ie. not copied to the target).") p.p_name
           | OpDelete ->
-              sprintf (f_"%s: This partition will be deleted.") name
+              sprintf (f_"%s: This partition will be deleted.")
p.p_name
           | OpResize newsize ->
               sprintf (f_"%s: This partition will be resized from %s to
%s.")
-                name (human_size oldsize) (human_size newsize) ^
+                p.p_name (human_size p.p_part.G.part_size) (human_size newsize)
^
               if can_expand_content p.p_type then (
                 sprintf (f_"  The %s on %s will be expanded using the
'%s' method.")
                   (string_of_partition_content_no_size p.p_type)
-                  name
+                  p.p_name
                   (string_of_expand_content_method
                      (expand_content_method p.p_type))
               ) else "" in
 
-        wrap ~indent:4 (text ^ "\n\n")
-    ) partitions;
+        wrap ~indent:4 (text ^ "\n\n");
+        
+        List.iter print_summary p.p_partitions in
+
+    List.iter print_summary partitions;
 
     List.iter (
       fun ({ lv_name = name } as lv) ->
@@ -1009,10 +1106,11 @@ read the man page virt-resize(1).
   let partitions      let sectsize = Int64.of_int sectsize in
 
-    let rec loop partnum start = function
+    let rec loop partnum start gap create_surplus = function
       | p :: ps ->
+        let start = start +^ gap in
         (match p.p_operation with
-        | OpDelete -> loop partnum start ps (* skip p *)
+        | OpDelete -> loop partnum start 0L create_surplus ps (* skip p *)
 
         | OpIgnore | OpCopy ->          (* same size *)
           (* Size in sectors. *)
@@ -1026,26 +1124,28 @@ read the man page virt-resize(1).
               partnum start (end_ -^ 1L);
 
           { p with p_target_start = start; p_target_end = end_ -^ 1L;
-            p_target_partnum = partnum } :: loop (partnum+1) next ps
+            p_target_partnum = partnum; p_partitions = loop 5 start 1L
+      false p.p_partitions } :: loop (partnum+1) next 0L create_surplus ps
 
         | OpResize newsize ->           (* resized partition *)
+            let oldsize = p.p_part.G.part_size in
           (* New size in sectors. *)
           let size = (newsize +^ sectsize -^ 1L) /^ sectsize in
           (* Start of next partition + alignment. *)
           let next = start +^ size in
           let next = roundup64 next alignment in
 
-          if verbose then
-            eprintf "target partition %d: resize: newsize=%Ld start=%Ld
end=%Ld\n%!"
-              partnum newsize start (next -^ 1L);
+            eprintf "target partition %d: resize: oldsize=%Ld newsize=%Ld
start=%Ld end=%Ld\n%!"
+              partnum oldsize newsize start (next -^ 1L);
 
           { p with p_target_start = start; p_target_end = next -^ 1L;
-            p_target_partnum = partnum } :: loop (partnum+1) next ps
+            p_target_partnum = partnum; p_partitions = loop 5 start 1L
+            false p.p_partitions } :: loop (partnum+1) next 0L create_surplus
ps
         )
 
       | [] ->
         (* Create the surplus partition if there is room for it. *)
-        if extra_partition && surplus >= min_extra_partition then (
+        if create_surplus && extra_partition && surplus >=
min_extra_partition then (
           [ {
             (* Since this partition has no source, this data is
              * meaningless and not used since the operation is
@@ -1056,6 +1156,8 @@ read the man page virt-resize(1).
                        part_size = 0L };
             p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
             p_label = None;
+            p_part_num = 0;
+            p_partitions = [];
 
             (* Target information is meaningful. *)
             p_operation = OpIgnore;
@@ -1078,14 +1180,52 @@ read the man page virt-resize(1).
         (* Preserve the existing start, but convert to sectors. *)
         (List.hd partitions).p_part.G.part_start /^ sectsize in
 
-    loop 1 start partitions in
+    loop 1 start 0L true partitions in
+
+  let mbr_part_type x = 
+    if x.p_part_num <= 4 && x.p_type <>
ContentExtendedPartition then "primary"
+    else if x.p_part_num <= 4 && x.p_type = ContentExtendedPartition
then "extended"
+    else "logical" in
 
   (* Now partition the target disk. *)
   List.iter (
     fun p ->
-      g#part_add "/dev/sdb" "primary" p.p_target_start
p.p_target_end
+      try
+        if p.p_operation <> OpIgnore then (
+          g#part_add "/dev/sdb" (mbr_part_type p) p.p_target_start
p.p_target_end
+        );
+        if p.p_type = ContentExtendedPartition then
+          List.iter (
+            fun p ->
+            if p.p_operation <> OpIgnore then
+              try
+                g#part_add "/dev/sdb" "logical"
p.p_target_start p.p_target_end
+              with G.Error msg -> ()
+          ) p.p_partitions
+      with
+        G.Error msg ->
+          (* eprintf "lsof:\n---\n%s\n---\n" (g#debug "sh"
[| "lsof" * |]); *)
+          eprintf "original error: %s\n" msg;
   ) partitions;
 
+  let g +    g#shutdown ();
+      g#close ();
+
+      let g = new G.guestfs () in
+      if trace then g#set_trace true;
+      if verbose then g#set_verbose true;
+      let _, { URI.path = path; protocol = protocol;
+             server = server; username = username;
+             password = password } = infile in
+      g#add_drive ?format ~readonly:true ~protocol ?server ?username
?secret:password path;
+      (* The output disk is being created, so use cache=unsafe here. *)
+      g#add_drive ?format:output_format ~readonly:false
~cachemode:"unsafe"
+        outfile;
+      if not quiet then Progress.set_up_progress_bar ~machine_readable g;
+      g#launch ();
+      g in
+
   (* Copy over the data. *)
   List.iter (
     fun p ->
@@ -1113,13 +1253,31 @@ read the man page virt-resize(1).
            g#copy_device_to_device ~size:copysize ~sparse source target
 
          | ContentExtendedPartition ->
-           (* You can't just copy an extended partition by name, eg.
-            * source = "/dev/sda2", because the device name only
covers
-            * the first 1K of the partition.  Instead, copy the
-            * source bytes from the parent disk (/dev/sda).
-            *)
-           let srcoffset = p.p_part.G.part_start in
-           g#copy_device_to_device ~srcoffset ~size:copysize
"/dev/sda" target
+             List.iter (
+               fun p ->
+                 match p.p_operation with
+                 | OpCopy | OpResize _ ->
+                   let oldsize = p.p_part.G.part_size in
+                   let newsize +                     match p.p_operation with
OpResize s -> s | _ -> oldsize in
+
+                   let copysize = if newsize < oldsize then
+                     newsize else oldsize in
+
+                   let source = p.p_name in
+                   let target = sprintf "/dev/sdb%d"
p.p_target_partnum in
+
+                   if not quiet then
+                     printf (f_"Copying %s ...\n%!") source;
+
+                   (match p.p_type with
+                     | ContentUnknown | ContentPV _ | ContentFS _ ->
+                         g#copy_device_to_device  ~size:copysize ~sparse source
target
+                     | _ -> ()
+                   )
+                 | OpIgnore | OpDelete -> ()
+             ) p.p_partitions
+
         )
       | OpIgnore | OpDelete -> ()
   ) partitions;
@@ -1240,8 +1398,7 @@ read the man page virt-resize(1).
     in
 
     (* Expand partition content as required. *)
-    List.iter (
-      function
+    let expand_partition = function
       | ({ p_operation = OpResize _ } as p)
           when can_expand_content p.p_type ->
           let source = p.p_name in
@@ -1256,7 +1413,11 @@ read the man page virt-resize(1).
 
           do_expand_content target meth
       | { p_operation = (OpCopy | OpIgnore | OpDelete | OpResize _) }
-        -> ()
+        -> () in
+
+    List.iter (
+      fun p -> expand_partition p; List.iter expand_partition
+      p.p_partitions
     ) partitions;
 
     (* Expand logical volume content as required. *)
-- 
1.9.3

On Tue, Aug 26, 2014 at 06:16:50PM +0800, Hu Tao wrote:
> Hi,
> 
> The attached patch adds support for resizing MBR logical partitions. The
> failure is still there, I can't get any helpful information from lsof.
> Any suggestions?
I don't see the error:

  Error: Error informing the kernel about modifications to partition /dev/sdb5

However I do see this error:

  virt-resize: error: libguestfs error: copy_device_to_device: 
  copy_device_to_device_stub: /dev/sdb5: No such file or directory

For debugging with lsof, I would need to actually see the trace output
and the lsof output.  See what I wrote here:

  https://www.redhat.com/archives/libguestfs/2014-July/msg00051.html
> +  p_part_num: int;               (* partition number *)
I think you should split out this change into a separate patch.  It's
uncontroversial to keep p_part_num in the structure, and will simplify
review of the patch.
> +  mutable p_partitions : partition list; (* MBR logical partitions.
Non-empty
> +                                            list implies extended
partition
I'm very unclear about this change to the structure.

Originally 'type partition' is a single primary/extended partition,
and we keep a list of them.  That's simple to understand.

After this patch, how does it work?

Looking at the rest of the patch it seemed to me that you'd probably
want to keep the list of logical partitions as a completely separate
list.
>    (* Helper function calculates the surplus space, given the total
>     * required so far for the current partition layout, compared to
>     * the size of the target disk.  If the return value >= 0 then
it's
> @@ -816,29 +911,31 @@ read the man page virt-resize(1).
>      printf "**********\n\n";
>      printf "Summary of changes:\n\n";
>  
> -    List.iter (
> -      fun ({ p_name = name; p_part = { G.part_size = oldsize }} as p)
->
> +    let rec print_summary p >          let text >            match
p.p_operation with
>            | OpCopy ->
> -              sprintf (f_"%s: This partition will be left
alone.") name
> +              sprintf (f_"%s: This partition will be left
alone.") p.p_name
>            | OpIgnore ->
> -              sprintf (f_"%s: This partition will be created, but the
contents will be ignored (ie. not copied to the target).") name
> +              sprintf (f_"%s: This partition will be created, but the
contents will be ignored (ie. not copied to the target).") p.p_name
>            | OpDelete ->
> -              sprintf (f_"%s: This partition will be deleted.")
name
> +              sprintf (f_"%s: This partition will be deleted.")
p.p_name
>            | OpResize newsize ->
>                sprintf (f_"%s: This partition will be resized from %s
to %s.")
> -                name (human_size oldsize) (human_size newsize) ^
> +                p.p_name (human_size p.p_part.G.part_size) (human_size
newsize) ^
>                if can_expand_content p.p_type then (
>                  sprintf (f_"  The %s on %s will be expanded using the
'%s' method.")
>                    (string_of_partition_content_no_size p.p_type)
> -                  name
> +                  p.p_name
>                    (string_of_expand_content_method
>                       (expand_content_method p.p_type))
>                ) else "" in
This bit seems to rename a variable for no particular reason.  If you
think this is simpler to read, then please submit it as a separate
patch.  Otherwise leave it out.

> +  let g > +    g#shutdown ();
> +      g#close ();
> +
> +      let g = new G.guestfs () in
> +      if trace then g#set_trace true;
> +      if verbose then g#set_verbose true;
> +      let _, { URI.path = path; protocol = protocol;
> +             server = server; username = username;
> +             password = password } = infile in
> +      g#add_drive ?format ~readonly:true ~protocol ?server ?username
?secret:password path;
> +      (* The output disk is being created, so use cache=unsafe here. *)
> +      g#add_drive ?format:output_format ~readonly:false
~cachemode:"unsafe"
> +        outfile;
> +      if not quiet then Progress.set_up_progress_bar ~machine_readable g;
> +      g#launch ();
> +      g in
What's this bit for?


----------------------------------------------------------------------

How are you testing this?  It really needs a script that tests this
case, since it obviously makes the code a lot more complex.  Also when
you see the error message, what virt-resize and other commands are you
using?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v