Libguestfs - Aug 2016 - Re: [PATCH 1/2] v2v: Make fstrim warning clearer (RHBZ#1366456).

This reverts the change made for RHBZ#1168144.  The warning is now
always displayed.

It would be nice to make the warning actionable, but there is not a
lot that end users can do since fstrim is such a complex topic
interacting with all filesystem and storage layers.
---
 v2v/v2v.ml | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index e221f29..8365aae 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -397,11 +397,7 @@ and do_fstrim g inspect        if mounted then (
         try g#fstrim "/"
         with G.Error msg ->
-          (* Only emit this warning when debugging, because otherwise
-           * it causes distress (RHBZ#1168144).
-           *)
-          if verbose () then
-            warning (f_"%s (ignored)") msg
+          warning (f_"fstrim on guest filesystem %s failed.  This may mean
that conversion takes longer than normal.  Usually you can ignore this as fstrim
is just an optimization.  Not all filesystems support trimming, and it can
depend on specific details of the filesystem, alignment and backing storage. 
Original message: %s") dev msg
       )
   ) fses
 
-- 
2.7.4

This *may* be required by some filesystems in order for fstrim to
work.  I'm not actually sure if this is true, but it's what
virt-sparsify --in-place does, and that utility has been tested a lot
more in regards to trimming.
---
 v2v/v2v.ml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index 8365aae..8ae3dec 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -393,7 +393,10 @@ and do_fstrim g inspect    List.iter (
     fun dev ->
       g#umount_all ();
-      let mounted = try g#mount dev "/"; true with G.Error _ ->
false in
+      let mounted +        try g#mount_options "discard" dev
"/"; true
+        with G.Error _ -> false in
+
       if mounted then (
         try g#fstrim "/"
         with G.Error msg ->
-- 
2.7.4

On Friday, 12 August 2016 10:37:29 CEST Richard W.M. Jones
wrote:
> This reverts the change made for RHBZ#1168144.  The warning is now
> always displayed.
> 
> It would be nice to make the warning actionable, but there is not a
> lot that end users can do since fstrim is such a complex topic
> interacting with all filesystem and storage layers.
> ---
>  v2v/v2v.ml | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/v2v/v2v.ml b/v2v/v2v.ml
> index e221f29..8365aae 100644
> --- a/v2v/v2v.ml
> +++ b/v2v/v2v.ml
> @@ -397,11 +397,7 @@ and do_fstrim g inspect >        if mounted then (
>          try g#fstrim "/"
>          with G.Error msg ->
> -          (* Only emit this warning when debugging, because otherwise
> -           * it causes distress (RHBZ#1168144).
> -           *)
> -          if verbose () then
> -            warning (f_"%s (ignored)") msg
> +          warning (f_"fstrim on guest filesystem %s failed.  This may
mean that conversion takes longer than normal.  Usually you can ignore this as
fstrim is just an optimization.  Not all filesystems support trimming, and it
can depend on specific details of the filesystem, alignment and backing storage.
Original message: %s") dev msg
>        )
The idea is OK, but the message a bit too long perhaps? I'd keep the
warning short here, and add the longer bit in an own paragraph in the
manual.

-- 
Pino Toscano

On Friday, 12 August 2016 10:37:30 CEST Richard W.M. Jones
wrote:
> This *may* be required by some filesystems in order for fstrim to
> work.  I'm not actually sure if this is true, but it's what
> virt-sparsify --in-place does, and that utility has been tested a lot
> more in regards to trimming.
> ---
LGTM

Thanks,
-- 
Pino Toscano

On Fri, Aug 12, 2016 at 04:50:31PM +0200, Pino Toscano
wrote:
> On Friday, 12 August 2016 10:37:29 CEST Richard W.M. Jones wrote:
> > This reverts the change made for RHBZ#1168144.  The warning is now
> > always displayed.
> > 
> > It would be nice to make the warning actionable, but there is not a
> > lot that end users can do since fstrim is such a complex topic
> > interacting with all filesystem and storage layers.
> > ---
> >  v2v/v2v.ml | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/v2v/v2v.ml b/v2v/v2v.ml
> > index e221f29..8365aae 100644
> > --- a/v2v/v2v.ml
> > +++ b/v2v/v2v.ml
> > @@ -397,11 +397,7 @@ and do_fstrim g inspect > >        if
mounted then (
> >          try g#fstrim "/"
> >          with G.Error msg ->
> > -          (* Only emit this warning when debugging, because otherwise
> > -           * it causes distress (RHBZ#1168144).
> > -           *)
> > -          if verbose () then
> > -            warning (f_"%s (ignored)") msg
> > +          warning (f_"fstrim on guest filesystem %s failed. 
This may mean that conversion takes longer than normal.  Usually you can ignore
this as fstrim is just an optimization.  Not all filesystems support trimming,
and it can depend on specific details of the filesystem, alignment and backing
storage.  Original message: %s") dev msg
> >        )
> 
> The idea is OK, but the message a bit too long perhaps? I'd keep the
> warning short here, and add the longer bit in an own paragraph in the
> manual.
I despair that no one reads the manual ...

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