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

[Pino Toscano]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20171106/b276bdf3/attachment.sig>