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

[Paolo Bonzini <pbonzini@redhat.com>]

On 31/03/2015 17:14, Justin Ossevoort 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. It
> also returns all bytes trimmed for all filesystems, providing a hint to
> the caller about how effective the guest-fstrim request was.
> 
> Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
> ---
>  qga/commands-posix.c | 19 +++++++++++--------
>  qga/qapi-schema.json |  5 +++--
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ba8de62..5b11ecf 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1325,18 +1325,15 @@ 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)
> +int64_t qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
>      int ret = 0;
>      FsMountList mounts;
>      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;
> +    int64_t trimmed = 0;
>  
>      slog("guest-fstrim called");
>  
> @@ -1344,7 +1341,7 @@ 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 0;
>      }
>  
>      QTAILQ_FOREACH(mount, &mounts, next) {
> @@ -1360,6 +1357,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) {
> @@ -1370,10 +1370,12 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>              }
>          }
>          close(fd);
> +        trimmed += r.len;
>      }
>  
>  error:
>      free_fs_mount_list(&mounts);
> +    return trimmed;
>  }
>  #endif /* CONFIG_FSTRIM */
>  
> @@ -2402,9 +2404,10 @@ 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)
> +int64_t qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
>      error_set(errp, QERR_UNSUPPORTED);
> +    return 0;
>  }
>  #endif
>  
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 95f49e3..48953f4 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -437,12 +437,13 @@
>  #       fragmented free space, although not all blocks will be discarded.
>  #       The default value is zero, meaning "discard every free block".
>  #
> -# Returns: Nothing.
> +# Returns: Number of bytes trimmed by this call.

It's better to add "(since 2.4)" here.

Apart from this, looks good.

I'm CCing the qemu-ga maintainer.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

>  #
>  # Since: 1.2
>  ##
>  { 'command': 'guest-fstrim',
> -  'data': { '*minimum': 'int' } }
> +  'data': { '*minimum': 'int' },
> +  'returns': 'int' }
>  
>  ##
>  # @guest-suspend-disk
>