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 <address@hidden> 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 <address@hidden>
> ---
>  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