virt-resize: support to MBR logical partitions and some question
Hi, I'm adding support to resizing logical partitions(patch is in progess). But encounter an error when adding a logical partition in dest image: virt-resize: libguestfs error: part_add: parted: /dev/sdb: Warning: The resulting partition is not properly aligned for best performance. Error: Error informing the kernel about modifications to partition /dev/sdb5 -- Device or resource busy. This means Linux won't know about any changes you made to /dev/sdb5 until you reboot -- so you shouldn't mount it or use it in any way before rebooting. Error: Failed to add partition 5 (Device or resource busy) The error is actually no harm since the logical partition has been added successfully, and I don't want to inform kernel at all. But it prevents virt-resize from adding further logical partitions. I can ignore the error when adding logical partitions manually using parted. The question is, is there any way to ignore such errors in virt-resize? Regards, Hu Tao
On Wed, Jun 04, 2014 at 10:21:41AM +0800, Hu Tao wrote:
Hi,
I'm adding support to resizing logical partitions(patch is in progess). But encounter an error when adding a logical partition in dest image:
virt-resize: libguestfs error: part_add: parted: /dev/sdb: Warning: The resulting partition is not properly aligned for best performance. Error: Error informing the kernel about modifications to partition /dev/sdb5 -- Device or resource busy. This means Linux won't know about any changes you made to /dev/sdb5 until you reboot -- so you shouldn't mount it or use it in any way before rebooting. Error: Failed to add partition 5 (Device or resource busy)
The error is actually no harm since the logical partition has been added successfully, and I don't want to inform kernel at all. But it prevents virt-resize from adding further logical partitions.
I can ignore the error when adding logical partitions manually using parted. The question is, is there any way to ignore such errors in virt-resize?
From virt-resize code you can catch and ignore exceptions, but this is
This may indicate a bug in the daemon. Normally after parted runs, it tries to get the kernel to re-read partition tables. This can fail because for several reasons -- for example, because a partition is mounted (and for many other reasons, including udev and/or the kernel just getting confused. We try to avoid this if we can by various tricks, eg. see use of udev_settle function in the daemon. Can you come up with a minimal reproducer (eg. using just guestfish commands)? If so, then file a bug. potentially dangerous since it doesn't distinguish between this "harmless" error and other actual errors. I'm on holiday at the moment, but I'll try to offer help and advice on modifying virt-resize when I can. I'm back next week. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Wed, Jun 04, 2014 at 10:02:55AM +0100, Richard W.M. Jones wrote:
On Wed, Jun 04, 2014 at 10:21:41AM +0800, Hu Tao wrote:
Hi,
I'm adding support to resizing logical partitions(patch is in progess). But encounter an error when adding a logical partition in dest image:
virt-resize: libguestfs error: part_add: parted: /dev/sdb: Warning: The resulting partition is not properly aligned for best performance. Error: Error informing the kernel about modifications to partition /dev/sdb5 -- Device or resource busy. This means Linux won't know about any changes you made to /dev/sdb5 until you reboot -- so you shouldn't mount it or use it in any way before rebooting. Error: Failed to add partition 5 (Device or resource busy)
The error is actually no harm since the logical partition has been added successfully, and I don't want to inform kernel at all. But it prevents virt-resize from adding further logical partitions.
I can ignore the error when adding logical partitions manually using parted. The question is, is there any way to ignore such errors in virt-resize?
This may indicate a bug in the daemon.
Normally after parted runs, it tries to get the kernel to re-read partition tables. This can fail because for several reasons -- for example, because a partition is mounted (and for many other reasons, including udev and/or the kernel just getting confused.
We try to avoid this if we can by various tricks, eg. see use of udev_settle function in the daemon.
It seems udev_settle() doesn't help in the case. I think I need to digg more of the code for a sulation.
Can you come up with a minimal reproducer (eg. using just guestfish commands)? If so, then file a bug.
No, because guestfish doens't support adding logical partition but the problem is reproduced with logical partition only.
From virt-resize code you can catch and ignore exceptions, but this is potentially dangerous since it doesn't distinguish between this "harmless" error and other actual errors.
I'm on holiday at the moment, but I'll try to offer help and advice on modifying virt-resize when I can. I'm back next week.
Cool! Hu
Hi Rich, On Wed, Jun 04, 2014 at 10:02:55AM +0100, Richard W.M. Jones wrote:
On Wed, Jun 04, 2014 at 10:21:41AM +0800, Hu Tao wrote:
Hi,
I'm adding support to resizing logical partitions(patch is in progess). But encounter an error when adding a logical partition in dest image:
virt-resize: libguestfs error: part_add: parted: /dev/sdb: Warning: The resulting partition is not properly aligned for best performance. Error: Error informing the kernel about modifications to partition /dev/sdb5 -- Device or resource busy. This means Linux won't know about any changes you made to /dev/sdb5 until you reboot -- so you shouldn't mount it or use it in any way before rebooting. Error: Failed to add partition 5 (Device or resource busy)
The error is actually no harm since the logical partition has been added successfully, and I don't want to inform kernel at all. But it prevents virt-resize from adding further logical partitions.
I can ignore the error when adding logical partitions manually using parted. The question is, is there any way to ignore such errors in virt-resize?
This may indicate a bug in the daemon.
Normally after parted runs, it tries to get the kernel to re-read partition tables. This can fail because for several reasons -- for example, because a partition is mounted (and for many other reasons, including udev and/or the kernel just getting confused.
We try to avoid this if we can by various tricks, eg. see use of udev_settle function in the daemon.
Can you come up with a minimal reproducer (eg. using just guestfish commands)? If so, then file a bug.
guestfish just works with my patch(attached): --- [root@f20-x64 libguestfs-git]# rm -f delme.img && truncate -s 5G delme.img && ./run guestfish -a delme.img <<EOF run part-init /dev/sda msdos part-add /dev/sda extended 512 -1 part-add /dev/sda logical 2049 -1 EOF [root@f20-x64 libguestfs-git]# fish/guestfish -a delme.img <<EOF run list-partitions EOF /dev/sda1 /dev/sda5 --- But virt-resize doesn't. The error message is: Setting up initial partition table on test.img ... virt-resize: libguestfs error: part_add: parted: /dev/sdb: Warning: The resulting partition is not properly aligned for best performance. Error: Error informing the kernel about modifications to partition /dev/sdb5 -- Device or resource busy. This means Linux won't know about any changes you made to /dev/sdb5 until you reboot -- so you shouldn't mount it or use it in any way before rebooting. Error: Failed to add partition 5 (Device or resource busy) This is the same as I reported earlier. At this time /dev/sdb has been added successfully despite of the error message. But the bad is I can't write to /dev/sdb5, aka copy /dev/sda5 to /dev/sdb5. The error message is: --- Setting up initial partition table on test.img ... Copying /dev/sda1 ... 100% ⟦▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒⟧ 00:00 Copying /dev/sda2 ... 100% ⟦▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒⟧ 00:00 Copying /dev/sda3 ... Copying /dev/sda5 ... virt-resize: libguestfs error: copy_device_to_device: copy_device_to_device_stub: /dev/sdb5: No such file or directory --- The solution could be: 1. as the error message suggests, rebooting. But I don't known how since it runs in an applicance if I'm right. 2. fix parted informing the kernel about modifications to partitions. Do you have any insight of this? If you think this is a possible solution I'll start invetigate it. Regards, Hu
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. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
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
Attached is the test script I was using. You have to run it like this: ./run ./test.sh Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Hi Rich, On Mon, Sep 08, 2014 at 03:13:32PM +0100, Richard W.M. Jones wrote:
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
I've found the reason of this error. It's because of a bug of this patch related to --expand. I tested it with --resize so haven't been able to find the bug. I'll send the updated patch later.
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
Thanks, I'll post lsof output and trace output.
+ 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.
Okay.
+ 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.
Yes, it is the list of logical 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
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.
Okay.
+ 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?
It's for restarting the guest so the kernel can re-read the partition table, otherwise even if the logical partitions have been added successfully, the kernel can't read them for writing.
----------------------------------------------------------------------
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?
Basically I was using virt-resize --resize to test the patch, other commands are very similar with you script, except that I pre-created the image with partitions and some contents in them. I think a test script is a good idea, should I add it to the repo? Regards, Hu
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
On Mon, Sep 15, 2014 at 05:47:03PM +0800, Hu Tao wrote:
Hi Rich,
On Mon, Sep 08, 2014 at 03:13:32PM +0100, Richard W.M. Jones wrote:
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
I've found the reason of this error. It's because of a bug of this patch related to --expand. I tested it with --resize so haven't been able to find the bug. I'll send the updated patch later.
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
Thanks, I'll post lsof output and trace output.
This weekend I found a bug that might be similar to this one. See: https://bugzilla.redhat.com/show_bug.cgi?id=1141451 If it's the same thing, it might be fixed by calling 'udev_settle' after copy_device_to_device (see daemon/copy.c).
+ 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.
Okay.
+ 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.
Yes, it is the list of logical partitions.
So let's make that clear by having a 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.
Okay.
+ 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?
It's for restarting the guest so the kernel can re-read the partition table, otherwise even if the logical partitions have been added successfully, the kernel can't read them for writing.
----------------------------------------------------------------------
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?
Basically I was using virt-resize --resize to test the patch, other commands are very similar with you script, except that I pre-created the image with partitions and some contents in them. I think a test script is a good idea, should I add it to the repo?
Yes, definitely it needs a test. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Mon, Sep 15, 2014 at 10:54:17AM +0100, Richard W.M. Jones wrote:
On Mon, Sep 15, 2014 at 05:47:03PM +0800, Hu Tao wrote:
Hi Rich,
On Mon, Sep 08, 2014 at 03:13:32PM +0100, Richard W.M. Jones wrote:
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
I've found the reason of this error. It's because of a bug of this patch related to --expand. I tested it with --resize so haven't been able to find the bug. I'll send the updated patch later.
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
Thanks, I'll post lsof output and trace output.
This weekend I found a bug that might be similar to this one. See:
https://bugzilla.redhat.com/show_bug.cgi?id=1141451
If it's the same thing, it might be fixed by calling 'udev_settle' after copy_device_to_device (see daemon/copy.c).
Thanks, I'll check it out.
+ 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.
Okay.
+ 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.
Yes, it is the list of logical partitions.
So let's make that clear by having a separate list.
Working on it.
(* 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.
Okay.
+ 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?
It's for restarting the guest so the kernel can re-read the partition table, otherwise even if the logical partitions have been added successfully, the kernel can't read them for writing.
----------------------------------------------------------------------
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?
Basically I was using virt-resize --resize to test the patch, other commands are very similar with you script, except that I pre-created the image with partitions and some contents in them. I think a test script is a good idea, should I add it to the repo?
Yes, definitely it needs a test.
Sure. Regards, Hu
participants (2)
-
Hu Tao -
Richard W.M. Jones