[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qga/commands-posix: Fix bug in guest-fstrim
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] qga/commands-posix: Fix bug in guest-fstrim |
Date: |
Tue, 31 Mar 2015 18:52:25 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
Paolo
> #
> # Since: 1.2
> ##
> { 'command': 'guest-fstrim',
> - 'data': { '*minimum': 'int' } }
> + 'data': { '*minimum': 'int' },
> + 'returns': 'int' }
>
> ##
> # @guest-suspend-disk
>