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

- 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. *)
-- 
2.13.6

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.

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 Monday, 6 November 2017 15:54:14 CET Richard W.M. Jones
wrote:
> 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.
I'm still trying to double-guess what were the reasons behind the 100M
change (commit 918dd3705d3d34f28eb3cf30cd79d16358d525e3), although so
far I did not find any (since all the mountpoints are actual ones, not
fake filesystems).  The only way we could know about them is to inspect
the v2v logs... although we usually get them only when a conversion
fails.

-- 
Pino Toscano