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

[Qemu-devel] [PATCH v3 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.

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.

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/qapi-schema.json | 32 +++++++++++++++++++++++---
 2 files changed, 73 insertions(+), 22 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v3 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

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

[Thomas Huth]

On Thu, 30 Apr 2015 16:29:57 +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) {

Reviewed-by: Thomas Huth <thuth@redhat.com>

(I've also put qemu-stable on cc: since I think that is a bug that
should be fixed in the stable branches, too).

[Qemu-devel] [PATCH v3 2/2] qga/commands-posix: 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.

Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
---
 qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
 qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 17 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/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 v3 2/2] qga/commands-posix: Return per path fstrim result

[Thomas Huth]

On Thu, 30 Apr 2015 16:29:58 +0200
Justin Ossevoort <justin@quarantainenet.nl> 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.
> 
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
> ---
>  qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
>  qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
>  2 files changed, 69 insertions(+), 17 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;

I'm not sure, but does this "minimum" result make sense at all? What's
the kernel supposed to return in this field? I had a quick look at some
file system implementations in the kernel, but to me it seems like only
the .len field is updated with a return value.

> +        result->has_trimmed = true;
> +        result->trimmed = r.len;
>          close(fd);
>      }
>  
> -error:
>      free_fs_mount_list(&mounts);
> +    return response;
>  }
>  #endif /* CONFIG_FSTRIM */

I just also had a quick test of this patch and got this behaviour:

{"execute":"guest-fstrim"}            
{"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 2040348672}, {"minimum": 4096, "path": "/mnt2", "trimmed": 2040348672}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699807232}]}}
{"execute":"guest-fstrim"}            
{"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
{"execute":"guest-fstrim"}            
{"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
{"execute":"guest-fstrim"}            
{"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
{"execute":"guest-fstrim"}            
{"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}

/mnt and /mnt2 got successfully trimmed and consecutive calls then
reported "trimmed: 0". But the values for "/boot" and "/" do not make
sense to me, why does it claim to have always trimmed the same amount
of bytes here? (I only touched the /mnt and /mnt2 file systems before
doing the trim calls, so I wonder why there are bytes trimmed on /
and /boot at all?)

Also, I think you need to adjust the stub of qmp_guest_fstrim() in
commands-win32.c, too, so that you don't break the compilation of the
windows target.

 Thomas

Re: [Qemu-devel] [PATCH v3 2/2] qga/commands-posix: Return per path fstrim result

[Justin Ossevoort]

On 30-04-15 18:35, Thomas Huth wrote:
> On Thu, 30 Apr 2015 16:29:58 +0200
> Justin Ossevoort <justin@quarantainenet.nl> 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.
>>
>> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
>> ---
>>   qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++--------------
>>   qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++---
>>   2 files changed, 69 insertions(+), 17 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;
>
> I'm not sure, but does this "minimum" result make sense at all? What's
> the kernel supposed to return in this field? I had a quick look at some
> file system implementations in the kernel, but to me it seems like only
> the .len field is updated with a return value.

Without having looked at the actual implementation in the kernel, but 
based on discussions on the linux-kernel mailinglist, should the minimum 
reflect the actual minimum size it was able to trim.

I'm not certain if it's the minimum size supported by all parts of the 
storage stack or if it was the size of the smallest consecutive area 
trimmed.

It is only filled if something actually got trimmed. But that is still 
logical for both implementations. The reason I'm returning it, is 
because it get's updated by the kernel, so it might mean something to 
someone.

For my use-cases 'trimmed' is meaningful for monitoring, and per file 
system errors are useful in discovering why some filesystem might not 
get trimmed (and ensuring all filesystems actually get trimmed). I 
personally have no use for this 'minimum', so dropping it is fine by me.

>> +        result->has_trimmed = true;
>> +        result->trimmed = r.len;
>>           close(fd);
>>       }
>>
>> -error:
>>       free_fs_mount_list(&mounts);
>> +    return response;
>>   }
>>   #endif /* CONFIG_FSTRIM */
>
> I just also had a quick test of this patch and got this behaviour:
>
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 2040348672}, {"minimum": 4096, "path": "/mnt2", "trimmed": 2040348672}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699807232}]}}
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}}
>
> /mnt and /mnt2 got successfully trimmed and consecutive calls then
> reported "trimmed: 0". But the values for "/boot" and "/" do not make
> sense to me, why does it claim to have always trimmed the same amount
> of bytes here? (I only touched the /mnt and /mnt2 file systems before
> doing the trim calls, so I wonder why there are bytes trimmed on /
> and /boot at all?)

It will probably have trimmed the entire free space map the first time. 
I don't know if filesystem trim information is persistent or in-memory 
only. I suspect the response is filesystem / storage stack specific.

> Also, I think you need to adjust the stub of qmp_guest_fstrim() in
> commands-win32.c, too, so that you don't break the compilation of the
> windows target.

Good call, will fix.

>   Thomas

Thanks for ther review and tests.

Regards,

     Justin