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

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
> 
>