[Qemu-devel] [PATCH v4 0/2] Fix guest-fstrim behaviour

[Qemu-devel] [PATCH v4 0/2] Fix guest-fstrim behaviour

[Justin Ossevoort]

The qemu-ga 'guest-fstrim' command is currently not working properly.

There are 2 issues:
- The current implementation reuses a struct between ioctl() calls without
  reinitialising it's fields. This struct however is updated to reflect
  the result of the trim operation.
  Therefor only the first filesystem is thoroughly trimmed, the rest is only
  trimmed up to the amount that was trimmed by the previous filesystem.
- The current implementation will return an error if some filesystem returned
  an unexpected error. The first issue consistently causes this issue when
  the 'guest-fstrim' is performed multiple times in a row when multiple
  filesystems are being trimmed, as this causes a trim request for at most
  0 bytes, which is an error.

The first patch fixes the first issue by explicitly resetting the struct used
to perform the trim ioctl for each path. This is a pretty mundane change and
fixes most use-cases.

The second patch fixes the second issue by changing the returned value to
return a per-path result. This way all paths are always trimmed and dependening
on the outcome of the ioctl an error or some details about the trim are
returned. The returned values for error, minimum and trimmed are filesystem,
storage stack and kernel version dependant.

There was an earlier request to mirror the fields from the 'guest-fsinfo'
operation. The trim operation however need not happen at the mountpoint level.
A logical future improvement would be to allow the caller to supply an optional
list of paths they want to trim, without needing to have intimate details about
the filesystem layout of the guest.

[Changes since v3]
- Patch 2: Change return type of qmp_guest_fstrim in qga/command-win32.c
- Patch 2: Change commit message on patch 2 to indicate returned values are
           filesystem, storage stack and kernel version dependant

Justin Ossevoort (2):
  qga/commands-posix: Fix bug in guest-fstrim
  qga/commands-posix: Return per path fstrim result

 qga/commands-posix.c | 63 ++++++++++++++++++++++++++++++++++++----------------
 qga/commands-win32.c |  4 +++-
 qga/qapi-schema.json | 32 +++++++++++++++++++++++---
 3 files changed, 76 insertions(+), 23 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v4 1/2] qga/commands-posix: Fix bug in guest-fstrim

[Justin Ossevoort]

The FITRIM ioctl updates the fstrim_range structure it receives. This
way the caller can determine how many bytes were trimmed. The
guest-fstrim logic reuses the same fstrim_range for each filesystem,
effectively limiting each filesystem to trim at most as much as the
previous was able to trim.

If a previous filesystem would have trimmed 0 bytes, than the next
filesystem would report an error 'Invalid argument' because a FITRIM
request with length 0 is not valid.

This change resets the fstrim_range structure for each filesystem.

Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
---
 qga/commands-posix.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ba8de62..4449628 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1332,11 +1332,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
     struct FsMount *mount;
     int fd;
     Error *local_err = NULL;
-    struct fstrim_range r = {
-        .start = 0,
-        .len = -1,
-        .minlen = has_minimum ? minimum : 0,
-    };
+    struct fstrim_range r;
 
     slog("guest-fstrim called");
 
@@ -1360,6 +1356,9 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
          * error means an unexpected error, so return it in those cases.  In
          * some other cases ENOTTY will be reported (e.g. CD-ROMs).
          */
+        r.start = 0;
+        r.len = -1;
+        r.minlen = has_minimum ? minimum : 0;
         ret = ioctl(fd, FITRIM, &r);
         if (ret == -1) {
             if (errno != ENOTTY && errno != EOPNOTSUPP) {
-- 
2.1.4

[Qemu-devel] [PATCH v4 2/2] qga/qmp_guest_fstrim: Return per path fstrim result

[Justin Ossevoort]

The current guest-fstrim support only returns an error if some
mountpoint was unable to be trimmed, skipping any possible additional
mountpoints. The result of the TRIM operation itself is also discarded.

This change returns a per mountpoint result of the TRIM operation. If an
error occurs on some mountpoints that error is returned and the
guest-fstrim continue with any additional mountpoints.

The returned values for errors, minimum and trimmed are dependant on the
filesystem, storage stacks and kernel version.

Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
---
 qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
 qga/commands-win32.c |  4 +++-
 qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
 3 files changed, 72 insertions(+), 18 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 4449628..ec0d69e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1325,8 +1325,12 @@ static void guest_fsfreeze_cleanup(void)
 /*
  * Walk list of mounted file systems in the guest, and trim them.
  */
-void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
+GuestFilesystemTrimResponse *
+qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
+    GuestFilesystemTrimResponse *response;
+    GuestFilesystemTrimResultList *list;
+    GuestFilesystemTrimResult *result;
     int ret = 0;
     FsMountList mounts;
     struct FsMount *mount;
@@ -1340,39 +1344,59 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
     build_fs_mount_list(&mounts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        return NULL;
     }
 
+    response = g_malloc0(sizeof(*response));
+
     QTAILQ_FOREACH(mount, &mounts, next) {
+        result = g_malloc0(sizeof(*result));
+        result->path = g_strdup(mount->dirname);
+
+        list = g_malloc0(sizeof(*list));
+        list->value = result;
+        list->next = response->paths;
+        response->paths = list;
+
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
-            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
-            goto error;
+            result->error = g_strdup_printf("failed to open: %s",
+                                            strerror(errno));
+            result->has_error = true;
+            continue;
         }
 
         /* We try to cull filesytems we know won't work in advance, but other
          * filesytems may not implement fstrim for less obvious reasons.  These
-         * will report EOPNOTSUPP; we simply ignore these errors.  Any other
-         * error means an unexpected error, so return it in those cases.  In
-         * some other cases ENOTTY will be reported (e.g. CD-ROMs).
+         * will report EOPNOTSUPP; while in some other cases ENOTTY will be
+         * reported (e.g. CD-ROMs).
+         * Any other error means an unexpected error.
          */
         r.start = 0;
         r.len = -1;
         r.minlen = has_minimum ? minimum : 0;
         ret = ioctl(fd, FITRIM, &r);
         if (ret == -1) {
-            if (errno != ENOTTY && errno != EOPNOTSUPP) {
-                error_setg_errno(errp, errno, "failed to trim %s",
-                                 mount->dirname);
-                close(fd);
-                goto error;
+            result->has_error = true;
+            if (errno == ENOTTY || errno == EOPNOTSUPP) {
+                result->error = g_strdup("trim not supported");
+            } else {
+                result->error = g_strdup_printf("failed to trim: %s",
+                                                strerror(errno));
             }
+            close(fd);
+            continue;
         }
+
+        result->has_minimum = true;
+        result->minimum = r.minlen;
+        result->has_trimmed = true;
+        result->trimmed = r.len;
         close(fd);
     }
 
-error:
     free_fs_mount_list(&mounts);
+    return response;
 }
 #endif /* CONFIG_FSTRIM */
 
@@ -2401,9 +2425,11 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 #endif /* CONFIG_FSFREEZE */
 
 #if !defined(CONFIG_FSTRIM)
-void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
+GuestFilesystemTrimResponse *
+qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
     error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
 }
 #endif
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3ef0549..cc407f3 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -493,9 +493,11 @@ static void guest_fsfreeze_cleanup(void)
  * Walk list of mounted file systems in the guest, and discard unused
  * areas.
  */
-void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
+GuestFilesystemTrimResponse *
+qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
     error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
 }
 
 typedef enum {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 95f49e3..b4f4b93 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -425,6 +425,30 @@
   'returns': 'int' }
 
 ##
+# @GuestFilesystemTrimResult
+#
+# @path: path that was trimmed
+# @error: an error message when trim failed
+# @trimmed: bytes trimmed for this path
+# @minimum: reported effective minimum for this path
+#
+# Since: 2.4
+##
+{ 'type': 'GuestFilesystemTrimResult',
+  'data': {'path': 'str',
+           '*trimmed': 'int', '*minimum': 'int', '*error': 'str'} }
+
+##
+# @GuestFilesystemTrimResponse
+#
+# @paths: list of @GuestFilesystemTrimResult per path that was trimmed
+#
+# Since: 2.4
+##
+{ 'type': 'GuestFilesystemTrimResponse',
+  'data': {'paths': ['GuestFilesystemTrimResult']} }
+
+##
 # @guest-fstrim:
 #
 # Discard (or "trim") blocks which are not in use by the filesystem.
@@ -437,12 +461,14 @@
 #       fragmented free space, although not all blocks will be discarded.
 #       The default value is zero, meaning "discard every free block".
 #
-# Returns: Nothing.
+# Returns: A @GuestFilesystemTrimResponse which contains the
+#          status of all trimmed paths.
 #
-# Since: 1.2
+# Since: 2.4
 ##
 { 'command': 'guest-fstrim',
-  'data': { '*minimum': 'int' } }
+  'data': { '*minimum': 'int' },
+  'returns': 'GuestFilesystemTrimResponse' }
 
 ##
 # @guest-suspend-disk
-- 
2.1.4

Re: [Qemu-devel] [PATCH v4 2/2] qga/qmp_guest_fstrim: Return per path fstrim result

[Olga Krishtal]

[-- Attachment #1: Type: text/plain, Size: 7548 bytes --]

On 11/05/15 09:58, Justin Ossevoort wrote:
> The current guest-fstrim support only returns an error if some
> mountpoint was unable to be trimmed, skipping any possible additional
> mountpoints. The result of the TRIM operation itself is also discarded.
>
> This change returns a per mountpoint result of the TRIM operation. If an
> error occurs on some mountpoints that error is returned and the
> guest-fstrim continue with any additional mountpoints.
>
> The returned values for errors, minimum and trimmed are dependant on the
> filesystem, storage stacks and kernel version.
>
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
> ---
>   qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
>   qga/commands-win32.c |  4 +++-
>   qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
>   3 files changed, 72 insertions(+), 18 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 4449628..ec0d69e 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1325,8 +1325,12 @@ static void guest_fsfreeze_cleanup(void)
>   /*
>    * Walk list of mounted file systems in the guest, and trim them.
>    */
> -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> +GuestFilesystemTrimResponse *
> +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>   {
> +    GuestFilesystemTrimResponse *response;
> +    GuestFilesystemTrimResultList *list;
> +    GuestFilesystemTrimResult *result;
>       int ret = 0;
>       FsMountList mounts;
>       struct FsMount *mount;
> @@ -1340,39 +1344,59 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>       build_fs_mount_list(&mounts, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        return;
> +        return NULL;
>       }
>   
> +    response = g_malloc0(sizeof(*response));
> +
>       QTAILQ_FOREACH(mount, &mounts, next) {
> +        result = g_malloc0(sizeof(*result));
> +        result->path = g_strdup(mount->dirname);
> +
> +        list = g_malloc0(sizeof(*list));
> +        list->value = result;
> +        list->next = response->paths;
> +        response->paths = list;
> +
>           fd = qemu_open(mount->dirname, O_RDONLY);
>           if (fd == -1) {
> -            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
> -            goto error;
> +            result->error = g_strdup_printf("failed to open: %s",
> +                                            strerror(errno));
> +            result->has_error = true;
> +            continue;
>           }
>   
>           /* We try to cull filesytems we know won't work in advance, but other
>            * filesytems may not implement fstrim for less obvious reasons.  These
> -         * will report EOPNOTSUPP; we simply ignore these errors.  Any other
> -         * error means an unexpected error, so return it in those cases.  In
> -         * some other cases ENOTTY will be reported (e.g. CD-ROMs).
> +         * will report EOPNOTSUPP; while in some other cases ENOTTY will be
> +         * reported (e.g. CD-ROMs).
> +         * Any other error means an unexpected error.
>            */
>           r.start = 0;
>           r.len = -1;
>           r.minlen = has_minimum ? minimum : 0;
>           ret = ioctl(fd, FITRIM, &r);
>           if (ret == -1) {
> -            if (errno != ENOTTY && errno != EOPNOTSUPP) {
> -                error_setg_errno(errp, errno, "failed to trim %s",
> -                                 mount->dirname);
> -                close(fd);
> -                goto error;
> +            result->has_error = true;
> +            if (errno == ENOTTY || errno == EOPNOTSUPP) {
> +                result->error = g_strdup("trim not supported");
> +            } else {
> +                result->error = g_strdup_printf("failed to trim: %s",
> +                                                strerror(errno));
>               }
> +            close(fd);
> +            continue;
>           }
> +
> +        result->has_minimum = true;
> +        result->minimum = r.minlen;
> +        result->has_trimmed = true;
> +        result->trimmed = r.len;
>           close(fd);
>       }
>   
> -error:
>       free_fs_mount_list(&mounts);
> +    return response;
>   }
>   #endif /* CONFIG_FSTRIM */
>   
> @@ -2401,9 +2425,11 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>   #endif /* CONFIG_FSFREEZE */
>   
>   #if !defined(CONFIG_FSTRIM)
> -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> +GuestFilesystemTrimResponse *
> +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>   {
>       error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
>   }
>   #endif
>   
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3ef0549..cc407f3 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -493,9 +493,11 @@ static void guest_fsfreeze_cleanup(void)
>    * Walk list of mounted file systems in the guest, and discard unused
>    * areas.
>    */
> -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> +GuestFilesystemTrimResponse *
> +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>   {
>       error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
>   }
>   
>   typedef enum {
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 95f49e3..b4f4b93 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -425,6 +425,30 @@
>     'returns': 'int' }
>   
>   ##
> +# @GuestFilesystemTrimResult
> +#
> +# @path: path that was trimmed
The path means the mount point of fs?
> +# @error: an error message when trim failed
> +# @trimmed: bytes trimmed for this path
> +# @minimum: reported effective minimum for this path
> +#
> +# Since: 2.4
> +##
> +{ 'type': 'GuestFilesystemTrimResult',
> +  'data': {'path': 'str',
> +           '*trimmed': 'int', '*minimum': 'int', '*error': 'str'} }
Actually i am not quite sure about the whole structure, because when 
someone decides to implement
this command for Windows they may face some difficulties.
For Win 8 we have only ioctl FSCTL_FILE_LEVEL_TRIM, that allows as to 
specify
https://msdn.microsoft.com/en-us/library/windows/desktop/hh447300(v=vs.85).aspx
- Offset, in bytes, from the start of the file for the range to be trimmed.
- Length, in bytes, for the range to be trimmed.

And as output we have
Contains the number of ranges that were successfully processed.
If as path fs mountpoint will be used, then what should be done with 
other variables?
> +#
> +##
> +# @GuestFilesystemTrimResponse
> +#
> +# @paths: list of @GuestFilesystemTrimResult per path that was trimmed
> +#
> +# Since: 2.4
> +##
> +{ 'type': 'GuestFilesystemTrimResponse',
> +  'data': {'paths': ['GuestFilesystemTrimResult']} }
Afaik instead of type, struct should be used
> +##
>   # @guest-fstrim:
>   #
>   # Discard (or "trim") blocks which are not in use by the filesystem.
> @@ -437,12 +461,14 @@
>   #       fragmented free space, although not all blocks will be discarded.
>   #       The default value is zero, meaning "discard every free block".
>   #
> -# Returns: Nothing.
> +# Returns: A @GuestFilesystemTrimResponse which contains the
> +#          status of all trimmed paths.
>   #
> -# Since: 1.2
> +# Since: 2.4
>   ##
>   { 'command': 'guest-fstrim',
> -  'data': { '*minimum': 'int' } }
> +  'data': { '*minimum': 'int' },
> +  'returns': 'GuestFilesystemTrimResponse' }
>   
>   ##
>   # @guest-suspend-disk


[-- Attachment #2: Type: text/html, Size: 10304 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] qga/commands-posix: Fix bug in guest-fstrim

[Thomas Huth]

On Mon, 11 May 2015 08:58:44 +0200
Justin Ossevoort <justin@quarantainenet.nl> wrote:

> The FITRIM ioctl updates the fstrim_range structure it receives. This
> way the caller can determine how many bytes were trimmed. The
> guest-fstrim logic reuses the same fstrim_range for each filesystem,
> effectively limiting each filesystem to trim at most as much as the
> previous was able to trim.
> 
> If a previous filesystem would have trimmed 0 bytes, than the next
> filesystem would report an error 'Invalid argument' because a FITRIM
> request with length 0 is not valid.
> 
> This change resets the fstrim_range structure for each filesystem.
> 
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
> ---
>  qga/commands-posix.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ba8de62..4449628 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1332,11 +1332,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      struct FsMount *mount;
>      int fd;
>      Error *local_err = NULL;
> -    struct fstrim_range r = {
> -        .start = 0,
> -        .len = -1,
> -        .minlen = has_minimum ? minimum : 0,
> -    };
> +    struct fstrim_range r;
>  
>      slog("guest-fstrim called");
>  
> @@ -1360,6 +1356,9 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>           * error means an unexpected error, so return it in those cases.  In
>           * some other cases ENOTTY will be reported (e.g. CD-ROMs).
>           */
> +        r.start = 0;
> +        r.len = -1;
> +        r.minlen = has_minimum ? minimum : 0;
>          ret = ioctl(fd, FITRIM, &r);
>          if (ret == -1) {
>              if (errno != ENOTTY && errno != EOPNOTSUPP) {

*ping*

Michael, could you please have a look at this bugfix? Without it, the
fstrim command is pretty useless if the guest uses more than one
filesystem that could be trimmed...

 Thomas

Re: [Qemu-devel] [PATCH v4 1/2] qga/commands-posix: Fix bug in guest-fstrim

[Michael Roth]

Quoting Justin Ossevoort (2015-05-11 01:58:44)
> The FITRIM ioctl updates the fstrim_range structure it receives. This
> way the caller can determine how many bytes were trimmed. The
> guest-fstrim logic reuses the same fstrim_range for each filesystem,
> effectively limiting each filesystem to trim at most as much as the
> previous was able to trim.
> 
> If a previous filesystem would have trimmed 0 bytes, than the next
> filesystem would report an error 'Invalid argument' because a FITRIM
> request with length 0 is not valid.
> 
> This change resets the fstrim_range structure for each filesystem.
> 
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Cc'ing qemu-stable@nongnu.org

> ---
>  qga/commands-posix.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ba8de62..4449628 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1332,11 +1332,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      struct FsMount *mount;
>      int fd;
>      Error *local_err = NULL;
> -    struct fstrim_range r = {
> -        .start = 0,
> -        .len = -1,
> -        .minlen = has_minimum ? minimum : 0,
> -    };
> +    struct fstrim_range r;
> 
>      slog("guest-fstrim called");
> 
> @@ -1360,6 +1356,9 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>           * error means an unexpected error, so return it in those cases.  In
>           * some other cases ENOTTY will be reported (e.g. CD-ROMs).
>           */
> +        r.start = 0;
> +        r.len = -1;
> +        r.minlen = has_minimum ? minimum : 0;
>          ret = ioctl(fd, FITRIM, &r);
>          if (ret == -1) {
>              if (errno != ENOTTY && errno != EOPNOTSUPP) {
> -- 
> 2.1.4
> 
> 

Re: [Qemu-devel] [PATCH v4 2/2] qga/qmp_guest_fstrim: Return per path fstrim result

[Michael Roth]

Quoting Olga Krishtal (2015-05-25 05:41:22)
> On 11/05/15 09:58, Justin Ossevoort wrote:
> 
>     The current guest-fstrim support only returns an error if some
>     mountpoint was unable to be trimmed, skipping any possible additional
>     mountpoints. The result of the TRIM operation itself is also discarded.
> 
>     This change returns a per mountpoint result of the TRIM operation. If an
>     error occurs on some mountpoints that error is returned and the
>     guest-fstrim continue with any additional mountpoints.
> 
>     The returned values for errors, minimum and trimmed are dependant on the
>     filesystem, storage stacks and kernel version.
> 
>     Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
>     ---
>      qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
>      qga/commands-win32.c |  4 +++-
>      qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
>      3 files changed, 72 insertions(+), 18 deletions(-)
> 
>     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     index 4449628..ec0d69e 100644
>     --- a/qga/commands-posix.c
>     +++ b/qga/commands-posix.c
>     @@ -1325,8 +1325,12 @@ static void guest_fsfreeze_cleanup(void)
>      /*
>       * Walk list of mounted file systems in the guest, and trim them.
>       */
>     -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>     +GuestFilesystemTrimResponse *
>     +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      {
>     +    GuestFilesystemTrimResponse *response;
>     +    GuestFilesystemTrimResultList *list;
>     +    GuestFilesystemTrimResult *result;
>          int ret = 0;
>          FsMountList mounts;
>          struct FsMount *mount;
>     @@ -1340,39 +1344,59 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>          build_fs_mount_list(&mounts, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>     -        return;
>     +        return NULL;
>          }
> 
>     +    response = g_malloc0(sizeof(*response));
>     +
>          QTAILQ_FOREACH(mount, &mounts, next) {
>     +        result = g_malloc0(sizeof(*result));
>     +        result->path = g_strdup(mount->dirname);
>     +
>     +        list = g_malloc0(sizeof(*list));
>     +        list->value = result;
>     +        list->next = response->paths;
>     +        response->paths = list;
>     +
>              fd = qemu_open(mount->dirname, O_RDONLY);
>              if (fd == -1) {
>     -            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
>     -            goto error;
>     +            result->error = g_strdup_printf("failed to open: %s",
>     +                                            strerror(errno));
>     +            result->has_error = true;
>     +            continue;
>              }
> 
>              /* We try to cull filesytems we know won't work in advance, but other
>               * filesytems may not implement fstrim for less obvious reasons.  These
>     -         * will report EOPNOTSUPP; we simply ignore these errors.  Any other
>     -         * error means an unexpected error, so return it in those cases.  In
>     -         * some other cases ENOTTY will be reported (e.g. CD-ROMs).
>     +         * will report EOPNOTSUPP; while in some other cases ENOTTY will be
>     +         * reported (e.g. CD-ROMs).
>     +         * Any other error means an unexpected error.
>               */
>              r.start = 0;
>              r.len = -1;
>              r.minlen = has_minimum ? minimum : 0;
>              ret = ioctl(fd, FITRIM, &r);
>              if (ret == -1) {
>     -            if (errno != ENOTTY && errno != EOPNOTSUPP) {
>     -                error_setg_errno(errp, errno, "failed to trim %s",
>     -                                 mount->dirname);
>     -                close(fd);
>     -                goto error;
>     +            result->has_error = true;
>     +            if (errno == ENOTTY || errno == EOPNOTSUPP) {
>     +                result->error = g_strdup("trim not supported");
>     +            } else {
>     +                result->error = g_strdup_printf("failed to trim: %s",
>     +                                                strerror(errno));
>                  }
>     +            close(fd);
>     +            continue;
>              }
>     +
>     +        result->has_minimum = true;
>     +        result->minimum = r.minlen;
>     +        result->has_trimmed = true;
>     +        result->trimmed = r.len;
>              close(fd);
>          }
> 
>     -error:
>          free_fs_mount_list(&mounts);
>     +    return response;
>      }
>      #endif /* CONFIG_FSTRIM */
> 
>     @@ -2401,9 +2425,11 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>      #endif /* CONFIG_FSFREEZE */
> 
>      #if !defined(CONFIG_FSTRIM)
>     -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>     +GuestFilesystemTrimResponse *
>     +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      {
>          error_set(errp, QERR_UNSUPPORTED);
>     +    return NULL;
>      }
>      #endif
> 
>     diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>     index 3ef0549..cc407f3 100644
>     --- a/qga/commands-win32.c
>     +++ b/qga/commands-win32.c
>     @@ -493,9 +493,11 @@ static void guest_fsfreeze_cleanup(void)
>       * Walk list of mounted file systems in the guest, and discard unused
>       * areas.
>       */
>     -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>     +GuestFilesystemTrimResponse *
>     +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      {
>          error_set(errp, QERR_UNSUPPORTED);
>     +    return NULL;
>      }
> 
>      typedef enum {
>     diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>     index 95f49e3..b4f4b93 100644
>     --- a/qga/qapi-schema.json
>     +++ b/qga/qapi-schema.json
>     @@ -425,6 +425,30 @@
>        'returns': 'int' }
> 
>      ##
>     +# @GuestFilesystemTrimResult
>     +#
>     +# @path: path that was trimmed
> 
> The path means the mount point of fs?
> 
>     +# @error: an error message when trim failed
>     +# @trimmed: bytes trimmed for this path
>     +# @minimum: reported effective minimum for this path
> 
>     +#
>     +# Since: 2.4
>     +##
>     +{ 'type': 'GuestFilesystemTrimResult',
>     +  'data': {'path': 'str',
>     +           '*trimmed': 'int', '*minimum': 'int', '*error': 'str'} }
> 
> Actually i am not quite sure about the whole structure, because when someone
> decides to implement
> this command for Windows they may face some difficulties.
> For Win 8 we have only ioctl  FSCTL_FILE_LEVEL_TRIM, that allows as to specify
> https://msdn.microsoft.com/en-us/library/windows/desktop/hh447300(v=vs.85).aspx
> - Offset, in bytes, from the start of the file for the range to be trimmed.
> - Length, in bytes, for the range to be trimmed.
> 
> And as output we have
> Contains the number of ranges that were successfully processed.
> If as path fs mountpoint will be used, then what should be done with other
> variables?

From what I can tell, we pass in an array of FILE_LEVEL_TRIM_RANGE,
wherein the offset/length is specified. These are handled in order,
so when we get back the number of ranges completed, N, we can sum up
the lengths of each range up until N. Anything after that
was not completed, so we can set each error to to GetLastError, or
perhaps set N+1 mount's error to that, and trimmed = 0 for the rest
(with no error, since they were simply skipped according to docs)

Not sure where minimum would fit in, but simply not setting it is
acceptable if I'm not too out of touch with current guidelines for
schemas.

So it seems like it would be workable for w32 implementations.

> 
>     +#
> 
>     +##
>     +# @GuestFilesystemTrimResponse
>     +#
>     +# @paths: list of @GuestFilesystemTrimResult per path that was trimmed
>     +#
>     +# Since: 2.4
>     +##
>     +{ 'type': 'GuestFilesystemTrimResponse',
>     +  'data': {'paths': ['GuestFilesystemTrimResult']} }
> 
> Afaik instead of type, struct should be used

Yes, as of:

commit 895a2a80e0e054f0d5d3715aa93d10d15e49f9f7
Author: Eric Blake <eblake@redhat.com>
Date:   Mon May 4 09:05:27 2015 -0600

    qapi: Use 'struct' instead of 'type' in schema

> 
>     +##
>      # @guest-fstrim:
>      #
>      # Discard (or "trim") blocks which are not in use by the filesystem.
>     @@ -437,12 +461,14 @@
>      #       fragmented free space, although not all blocks will be discarded.
>      #       The default value is zero, meaning "discard every free block".
>      #
>     -# Returns: Nothing.
>     +# Returns: A @GuestFilesystemTrimResponse which contains the
>     +#          status of all trimmed paths.
>      #
>     -# Since: 1.2
>     +# Since: 2.4
>      ##
>      { 'command': 'guest-fstrim',
>     -  'data': { '*minimum': 'int' } }
>     +  'data': { '*minimum': 'int' },
>     +  'returns': 'GuestFilesystemTrimResponse' }
> 
>      ##
>      # @guest-suspend-disk
> 
> 

Re: [Qemu-devel] [PATCH v4 2/2] qga/qmp_guest_fstrim: Return per path fstrim result

[Michael Roth]

Quoting Justin Ossevoort (2015-05-11 01:58:45)
> The current guest-fstrim support only returns an error if some
> mountpoint was unable to be trimmed, skipping any possible additional
> mountpoints. The result of the TRIM operation itself is also discarded.
> 
> This change returns a per mountpoint result of the TRIM operation. If an
> error occurs on some mountpoints that error is returned and the
> guest-fstrim continue with any additional mountpoints.
> 
> The returned values for errors, minimum and trimmed are dependant on the
> filesystem, storage stacks and kernel version.
> 
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>

Looks good other than s/type/struct/ in schema that Olga pointed out.
No need to respin, can fix it up in my tree.

> ---
>  qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
>  qga/commands-win32.c |  4 +++-
>  qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
>  3 files changed, 72 insertions(+), 18 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 4449628..ec0d69e 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1325,8 +1325,12 @@ static void guest_fsfreeze_cleanup(void)
>  /*
>   * Walk list of mounted file systems in the guest, and trim them.
>   */
> -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> +GuestFilesystemTrimResponse *
> +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
> +    GuestFilesystemTrimResponse *response;
> +    GuestFilesystemTrimResultList *list;
> +    GuestFilesystemTrimResult *result;
>      int ret = 0;
>      FsMountList mounts;
>      struct FsMount *mount;
> @@ -1340,39 +1344,59 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      build_fs_mount_list(&mounts, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return;
> +        return NULL;
>      }
> 
> +    response = g_malloc0(sizeof(*response));
> +
>      QTAILQ_FOREACH(mount, &mounts, next) {
> +        result = g_malloc0(sizeof(*result));
> +        result->path = g_strdup(mount->dirname);
> +
> +        list = g_malloc0(sizeof(*list));
> +        list->value = result;
> +        list->next = response->paths;
> +        response->paths = list;
> +
>          fd = qemu_open(mount->dirname, O_RDONLY);
>          if (fd == -1) {
> -            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
> -            goto error;
> +            result->error = g_strdup_printf("failed to open: %s",
> +                                            strerror(errno));
> +            result->has_error = true;
> +            continue;
>          }
> 
>          /* We try to cull filesytems we know won't work in advance, but other
>           * filesytems may not implement fstrim for less obvious reasons.  These
> -         * will report EOPNOTSUPP; we simply ignore these errors.  Any other
> -         * error means an unexpected error, so return it in those cases.  In
> -         * some other cases ENOTTY will be reported (e.g. CD-ROMs).
> +         * will report EOPNOTSUPP; while in some other cases ENOTTY will be
> +         * reported (e.g. CD-ROMs).
> +         * Any other error means an unexpected error.
>           */
>          r.start = 0;
>          r.len = -1;
>          r.minlen = has_minimum ? minimum : 0;
>          ret = ioctl(fd, FITRIM, &r);
>          if (ret == -1) {
> -            if (errno != ENOTTY && errno != EOPNOTSUPP) {
> -                error_setg_errno(errp, errno, "failed to trim %s",
> -                                 mount->dirname);
> -                close(fd);
> -                goto error;
> +            result->has_error = true;
> +            if (errno == ENOTTY || errno == EOPNOTSUPP) {
> +                result->error = g_strdup("trim not supported");
> +            } else {
> +                result->error = g_strdup_printf("failed to trim: %s",
> +                                                strerror(errno));
>              }
> +            close(fd);
> +            continue;
>          }
> +
> +        result->has_minimum = true;
> +        result->minimum = r.minlen;
> +        result->has_trimmed = true;
> +        result->trimmed = r.len;
>          close(fd);
>      }
> 
> -error:
>      free_fs_mount_list(&mounts);
> +    return response;
>  }
>  #endif /* CONFIG_FSTRIM */
> 
> @@ -2401,9 +2425,11 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  #endif /* CONFIG_FSFREEZE */
> 
>  #if !defined(CONFIG_FSTRIM)
> -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> +GuestFilesystemTrimResponse *
> +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
>      error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
>  }
>  #endif
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3ef0549..cc407f3 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -493,9 +493,11 @@ static void guest_fsfreeze_cleanup(void)
>   * Walk list of mounted file systems in the guest, and discard unused
>   * areas.
>   */
> -void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> +GuestFilesystemTrimResponse *
> +qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
>      error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
>  }
> 
>  typedef enum {
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 95f49e3..b4f4b93 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -425,6 +425,30 @@
>    'returns': 'int' }
> 
>  ##
> +# @GuestFilesystemTrimResult
> +#
> +# @path: path that was trimmed
> +# @error: an error message when trim failed
> +# @trimmed: bytes trimmed for this path
> +# @minimum: reported effective minimum for this path
> +#
> +# Since: 2.4
> +##
> +{ 'type': 'GuestFilesystemTrimResult',
> +  'data': {'path': 'str',
> +           '*trimmed': 'int', '*minimum': 'int', '*error': 'str'} }
> +
> +##
> +# @GuestFilesystemTrimResponse
> +#
> +# @paths: list of @GuestFilesystemTrimResult per path that was trimmed
> +#
> +# Since: 2.4
> +##
> +{ 'type': 'GuestFilesystemTrimResponse',
> +  'data': {'paths': ['GuestFilesystemTrimResult']} }
> +
> +##
>  # @guest-fstrim:
>  #
>  # Discard (or "trim") blocks which are not in use by the filesystem.
> @@ -437,12 +461,14 @@
>  #       fragmented free space, although not all blocks will be discarded.
>  #       The default value is zero, meaning "discard every free block".
>  #
> -# Returns: Nothing.
> +# Returns: A @GuestFilesystemTrimResponse which contains the
> +#          status of all trimmed paths.
>  #
> -# Since: 1.2
> +# Since: 2.4
>  ##
>  { 'command': 'guest-fstrim',
> -  'data': { '*minimum': 'int' } }
> +  'data': { '*minimum': 'int' },
> +  'returns': 'GuestFilesystemTrimResponse' }
> 
>  ##
>  # @guest-suspend-disk
> -- 
> 2.1.4
> 
> 

Re: [Qemu-devel] [PATCH v4 2/2] qga/qmp_guest_fstrim: Return per path fstrim result

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On 05/11/2015 12:58 AM, Justin Ossevoort wrote:
> The current guest-fstrim support only returns an error if some
> mountpoint was unable to be trimmed, skipping any possible additional
> mountpoints. The result of the TRIM operation itself is also discarded.
> 
> This change returns a per mountpoint result of the TRIM operation. If an
> error occurs on some mountpoints that error is returned and the
> guest-fstrim continue with any additional mountpoints.
> 
> The returned values for errors, minimum and trimmed are dependant on the
> filesystem, storage stacks and kernel version.
> 
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
> ---
>  qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
>  qga/commands-win32.c |  4 +++-
>  qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
>  3 files changed, 72 insertions(+), 18 deletions(-)
> 

> @@ -437,12 +461,14 @@
>  #       fragmented free space, although not all blocks will be discarded.
>  #       The default value is zero, meaning "discard every free block".
>  #
> -# Returns: Nothing.
> +# Returns: A @GuestFilesystemTrimResponse which contains the
> +#          status of all trimmed paths.

Maybe mention (since 2.4) as part of the Returns: line.

>  #
> -# Since: 1.2
> +# Since: 2.4

NACK to this hunk.  The command itself has existed since 1.2, it is only
the returns type that has changed in 2.4.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 0/2] Fix guest-fstrim behaviour

[Michael Roth]

Quoting Justin Ossevoort (2015-05-11 01:58:43)
> The qemu-ga 'guest-fstrim' command is currently not working properly.
> 
> There are 2 issues:
> - The current implementation reuses a struct between ioctl() calls without
>   reinitialising it's fields. This struct however is updated to reflect
>   the result of the trim operation.
>   Therefor only the first filesystem is thoroughly trimmed, the rest is only
>   trimmed up to the amount that was trimmed by the previous filesystem.
> - The current implementation will return an error if some filesystem returned
>   an unexpected error. The first issue consistently causes this issue when
>   the 'guest-fstrim' is performed multiple times in a row when multiple
>   filesystems are being trimmed, as this causes a trim request for at most
>   0 bytes, which is an error.

Applied, thanks.

> 
> The first patch fixes the first issue by explicitly resetting the struct used
> to perform the trim ioctl for each path. This is a pretty mundane change and
> fixes most use-cases.
> 
> The second patch fixes the second issue by changing the returned value to
> return a per-path result. This way all paths are always trimmed and dependening
> on the outcome of the ioctl an error or some details about the trim are
> returned. The returned values for error, minimum and trimmed are filesystem,
> storage stack and kernel version dependant.
> 
> There was an earlier request to mirror the fields from the 'guest-fsinfo'
> operation. The trim operation however need not happen at the mountpoint level.
> A logical future improvement would be to allow the caller to supply an optional
> list of paths they want to trim, without needing to have intimate details about
> the filesystem layout of the guest.
> 
> [Changes since v3]
> - Patch 2: Change return type of qmp_guest_fstrim in qga/command-win32.c
> - Patch 2: Change commit message on patch 2 to indicate returned values are
>            filesystem, storage stack and kernel version dependant
> 
> Justin Ossevoort (2):
>   qga/commands-posix: Fix bug in guest-fstrim
>   qga/commands-posix: Return per path fstrim result
> 
>  qga/commands-posix.c | 63 ++++++++++++++++++++++++++++++++++++----------------
>  qga/commands-win32.c |  4 +++-
>  qga/qapi-schema.json | 32 +++++++++++++++++++++++---
>  3 files changed, 76 insertions(+), 23 deletions(-)
> 
> -- 
> 2.1.4
> 
>