Re: [Libguestfs] [PATCH] v2v: rework free space check in guest mountpoints

On Mon, Nov 06, 2017 at 03:45:14PM +0100, Pino Toscano wrote: > - move the space needed by mountpoint mapping to an own function, > outside of the checking loop > - remove the minimum limit of 100M on partitions, since existing > partitions that we check (e.g. /boot) may be smaller than that > - in case /boot is not on a separate mountpoint, enforce the space check > on the root mountpoint instead, since it is where /boot is in that > case > --- > v2v/v2v.ml | 55 ++++++++++++++++++++++++++++--------------------------- > 1 file changed, 28 insertions(+), 27 deletions(-) > > diff --git a/v2v/v2v.ml b/v2v/v2v.ml > index b4c41e188..24b38458f 100644 > --- a/v2v/v2v.ml > +++ b/v2v/v2v.ml > @@ -415,34 +415,35 @@ and print_mpstat chan { mp_dev = dev; mp_path = path; > *) > and check_guest_free_space mpstats = > message (f_"Checking for sufficient free disk space in the guest"); > + > + (* Check whether /boot has its own mount point. *) > + let has_boot = List.exists (fun { mp_path } -> mp_path = "/boot") mpstats in > + > + let needed_bytes_for_mp = function > + | "/boot" > + | "/" when not has_boot -> > + (* We usually regenerate the initramfs, which has a > + * typical size of 20-30MB. Hence: > + *) > + 50_000_000L > + | "/" -> > + (* We may install some packages, and they would usually go > + * on the root filesystem. > + *) > + 20_000_000L > + | _ -> > + (* For everything else, just make sure there is some free space. *) > + 10_000_000L > + in > + > List.iter ( > - fun { mp_path = mp; > - mp_statvfs = { G.bfree; blocks; bsize } } -> > - (* Ignore small filesystems. *) > - let total_size = blocks *^ bsize in > - if total_size > 100_000_000L then ( > - (* bfree = free blocks for root user *) > - let free_bytes = bfree *^ bsize in > - let needed_bytes = > - match mp with > - | "/" -> > - (* We may install some packages, and they would usually go > - * on the root filesystem. > - *) > - 20_000_000L > - | "/boot" -> > - (* We usually regenerate the initramfs, which has a > - * typical size of 20-30MB. Hence: > - *) > - 50_000_000L > - | _ -> > - (* For everything else, just make sure there is some free space. *) > - 10_000_000L in > - > - if free_bytes < needed_bytes then > - error (f_"not enough free space for conversion on filesystem ‘%s’. %Ld bytes free < %Ld bytes needed") > - mp free_bytes needed_bytes > - ) > + fun { mp_path; mp_statvfs = { G.bfree; bsize } } -> > + (* bfree = free blocks for root user *) > + let free_bytes = bfree *^ bsize in > + let needed_bytes = needed_bytes_for_mp mp_path in > + if free_bytes < needed_bytes then > + error (f_"not enough free space for conversion on filesystem ‘%s’. %Ld bytes free < %Ld bytes needed") > + mp_path free_bytes needed_bytes > ) mpstats > > (* Perform the fstrim. *) Yeah, it's a pretty straightforward refactoring except for losing the 100 MB min size and the bit about checking root if /boot is not separately mounted. Therefore: ACK I suspect this change is a bit dangerous for RHEL 7.5 at this point. Better to give it a bit more testing in case the 100 MB min size was really needed for some reason.