[PATCH 0/4] sparsify: Warn instead of error if a filesystem cannot be fstrimmed.
This replaces the indirect exception catching from commit 931aec5b8846ce63b0e2b6beba5861a019b5f694 with a direct feature test. Fixes commit 931aec5b8846ce63b0e2b6beba5861a019b5f694. --- sparsify/in_place.ml | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml index 389ea44..5cd03ef 100644 --- a/sparsify/in_place.ml +++ b/sparsify/in_place.ml @@ -29,7 +29,7 @@ open Cmdline module G = Guestfs -let rec run disk format ignores machine_readable zeroes = +let run disk format ignores machine_readable zeroes = (* Connect to libguestfs. *) let g = open_guestfs () in @@ -40,17 +40,6 @@ let rec run disk format ignores machine_readable zeroes = Sys.set_signal Sys.sigquit (Sys.Signal_handle set_quit); g#set_pgroup true; - try - perform g disk format ignores machine_readable zeroes quit - with - G.Error msg as exn -> - if g#last_errno () = G.Errno.errno_ENOTSUP then ( - (* for exit code 3, see man page *) - error ~exit_code:3 (f_"discard/trim is not supported: %s") msg; - ) - else raise exn - -and perform g disk format ignores machine_readable zeroes quit = (* XXX Current limitation of the API. Can remove this hunk in future. *) let format = match format with @@ -62,6 +51,12 @@ and perform g disk format ignores machine_readable zeroes quit = if not (quiet ()) then Progress.set_up_progress_bar ~machine_readable g; g#launch (); + (* If discard is not supported in the appliance, we must return exit + * code 3. See the man page. + *) + if not (g#feature_available [|"fstrim"|]) then + error ~exit_code:3 (f_"discard/trim is not supported"); + (* Discard non-ignored filesystems that we are able to mount, and * selected swap partitions. *) -- 2.7.4
Because we run the external fstrim command we don't have access to the kernel errno when it fails. However in the case where it prints this specific error message, turn that into errno ENOTSUP. --- daemon/fstrim.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/daemon/fstrim.c b/daemon/fstrim.c index 1ad3630..527acfd 100644 --- a/daemon/fstrim.c +++ b/daemon/fstrim.c @@ -105,7 +105,13 @@ do_fstrim (const char *path, r = commandv (&out, &err, argv); if (r == -1) { - reply_with_error ("%s", err); + /* If the error is about the kernel operation not being supported + * for this filesystem type, then return errno ENOTSUP here. + */ + if (strstr (err, "discard operation is not supported")) + reply_with_error_errno (ENOTSUP, "%s", err); + else + reply_with_error ("%s", err); return -1; } -- 2.7.4
On Friday, 5 August 2016 12:22:45 CEST Richard W.M. Jones wrote:
Because we run the external fstrim command we don't have access to the kernel errno when it fails. However in the case where it prints this specific error message, turn that into errno ENOTSUP. ---
Can you please note this behaviour in the documentation of the fstrim API? Thanks, -- Pino Toscano
On Fri, Aug 05, 2016 at 04:05:15PM +0200, Pino Toscano wrote:
On Friday, 5 August 2016 12:22:45 CEST Richard W.M. Jones wrote:
Because we run the external fstrim command we don't have access to the kernel errno when it fails. However in the case where it prints this specific error message, turn that into errno ENOTSUP. ---
Can you please note this behaviour in the documentation of the fstrim API?
Sure. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Thanks: Xianghua Chen --- sparsify/in_place.ml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml index 5cd03ef..e2ee9d9 100644 --- a/sparsify/in_place.ml +++ b/sparsify/in_place.ml @@ -89,7 +89,14 @@ let run disk format ignores machine_readable zeroes = if mounted then ( message (f_"Trimming %s") fs; - g#fstrim "/" + try g#fstrim "/" + with G.Error msg as exn -> + if g#last_errno () = G.Errno.errno_ENOTSUP then ( + let vfs_type = try g#vfs_type fs with _ -> "unknown" in + warning (f_"fstrim operation is not supported on %s (%s). Suppress this warning using '--ignore %s', or use copying mode instead.") + fs vfs_type fs + ) + else raise exn ) else ( let is_linux_x86_swap = (* Look for the signature for Linux swap on i386. -- 2.7.4
Add a regression test for the previous commit. --- sparsify/Makefile.am | 4 +- ...st-virt-sparsify-in-place-fstrim-unsupported.sh | 84 ++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100755 sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh diff --git a/sparsify/Makefile.am b/sparsify/Makefile.am index f8d2641..5e42f0b 100644 --- a/sparsify/Makefile.am +++ b/sparsify/Makefile.am @@ -22,6 +22,7 @@ EXTRA_DIST = \ test-virt-sparsify.sh \ test-virt-sparsify-docs.sh \ test-virt-sparsify-in-place.sh \ + test-virt-sparsify-in-place-fstrim-unsupported.sh \ virt-sparsify.pod CLEANFILES = *~ *.annot *.cmi *.cmo *.cmx *.cmxa *.o virt-sparsify @@ -126,7 +127,8 @@ TESTS = \ if ENABLE_APPLIANCE TESTS += \ test-virt-sparsify.sh \ - test-virt-sparsify-in-place.sh + test-virt-sparsify-in-place.sh \ + test-virt-sparsify-in-place-fstrim-unsupported.sh endif ENABLE_APPLIANCE check-valgrind: diff --git a/sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh b/sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh new file mode 100755 index 0000000..d6a300d --- /dev/null +++ b/sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh @@ -0,0 +1,84 @@ +#!/bin/bash - +# libguestfs virt-sparsify --in-place test script +# Copyright (C) 2011-2016 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +# Test that we do the right thing for filesystems where the fstrim +# operation is not supported. +# +# https://bugzilla.redhat.com/show_bug.cgi?id=1364347 +# +# This test assumes that the kernel vfat driver does not support +# fstrim. It might become supported in a future kernel version in +# which case we could use a different filesystem for this test, or +# delete the test if we are confident that all common filesystems are +# supported. +# +# The reason why vfat is significant is because UEFI guests use it. + +export LANG=C +set -e +set -x + +if [ -n "$SKIP_TEST_VIRT_SPARSIFY_IN_PLACE_FSTRIM_UNSUPPORTED_SH" ]; then + echo "$0: skipping test (environment variable set)" + exit 77 +fi + +if [ "$(guestfish get-backend)" = "uml" ]; then + echo "$0: skipping test because uml backend does not support discard" + exit 77 +fi + +img=test-virt-sparsify-in-place-fstrim-unsupported.img +log=test-virt-sparsify-in-place-fstrim-unsupported.log +rm -f $img $log + +# Create a test filesystem with a single vfat filesystem. +guestfish -N $img=fs:vfat exit + +# This should warn. +virt-sparsify --in-place $img |& tee $log + +# Check the warning was emitted. +grep "warning:.*fstrim" $log + +# This should ignore the filesystem and not warn. +virt-sparsify --in-place --ignore /dev/sda1 $img |& tee $log + +if grep "warning:.*fstrim.*not supported" $log; then + echo "$0: filesystem /dev/sda1 was not ignored" + exit 1 +fi + +# Create a test filesystem with vfat and ext4 filesystems. +guestfish -N $img=bootroot:vfat:ext4 exit + +# This should warn. +virt-sparsify --in-place $img |& tee $log + +# Check the warning was emitted. +grep "warning:.*fstrim" $log + +# This should ignore the filesystem and not warn. +virt-sparsify --in-place --ignore /dev/sda1 $img |& tee $log + +if grep "warning:.*fstrim.*not supported" $log; then + echo "$0: filesystem /dev/sda1 was not ignored" + exit 1 +fi + +rm $img $log -- 2.7.4
On Friday, 5 August 2016 12:22:43 CEST Richard W.M. Jones wrote:
The series mostly LGTM -- left only one note for patch #2 (so it's a documented behaviour). Thanks, -- Pino Toscano
participants (2)
-
Pino Toscano -
Richard W.M. Jones