| Message ID | 1427814871-1936-1-git-send-email-justin@quarantainenet.nl |
|---|---|
| State | New |
| Headers | show |
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 >
On 03/31/2015 09:14 AM, 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> > --- > -# Returns: Nothing. > +# Returns: Number of bytes trimmed by this call. > # > # Since: 1.2 > ## > { 'command': 'guest-fstrim', > - 'data': { '*minimum': 'int' } } > + 'data': { '*minimum': 'int' }, > + 'returns': 'int' } No. Please don't add any new non-dictionary returns (see my pending patch that would flag this as not being on the whitelist: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05046.html ). This is particularly true for an already existing command (clients might not be prepared for older guest agent returning an empty dictionary in place of an int; but SHOULD be prepared to see if the returned dictionary is empty). Better would be to add a key to the returned dictionary that reports either total amount trimmed, or even better, an array reporting stats for each file system trimmed; something like: { "return": { "trimmed": [ { "name":"abc", "mountpoint":"xyz", "trimmed":123 }, { "name":"def", "mountpoint":"pqr", "trimmed":456 } ] } } where the 'name' and 'mountpoint' keys of each array element correspond to the data given by the 'GuestFilesystemInfo' type in 'guest-get-fsinfo' (so that the two commands have correlated information).
On 03/31/2015 10:52 AM, Paolo Bonzini wrote: >> -# Returns: Nothing. >> +# Returns: Number of bytes trimmed by this call. > > It's better to add "(since 2.4)" here. > > Apart from this, looks good. Changing a "return":{} to "return":0 is not backwards-compatible. > > I'm CCing the qemu-ga maintainer. I failed to do that in my beefier reply, so I'm responding here just to say that this needs a v2.
On 31/03/2015 19:03, Eric Blake wrote: >>> >>> Apart from this, looks good. > Changing a "return":{} to "return":0 is not backwards-compatible. Why not? Paolo >> I'm CCing the qemu-ga maintainer. > I failed to do that in my beefier reply, so I'm responding here > just to say that this needs a v2.
On 04/01/2015 01:54 AM, Paolo Bonzini wrote: > > > On 31/03/2015 19:03, Eric Blake wrote: >>>> >>>> Apart from this, looks good. >> Changing a "return":{} to "return":0 is not backwards-compatible. > > Why not? It's only a minor incompatibility, but a client that hard-codes itself to parsing "returns":0 (that is, expecting a json-number) will fail when talking to an older qemu that provided a json-object instead; while a client that expects a json-object always and can search for a "key":0 integer pair within that object that may or may not be present (we already document that clients MUST be prepared for dictionary members to be optional, but do NOT advise that clients must be prepared for a change between fundamental JSON types). Yes, the backwards-incompatibility is a weak argument, but as my pending series will be requiring all new commands to whitelist a non-dict return, we might as well avoid making this another case for that whitelist. My stronger argument in my other email is that returning a single integer is not much information. Since we have FULL statistics (how much did each file system trim), why not return it all, for the end user to take advantage of if desired?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 01/04/2015 14:33, Eric Blake wrote: > It's only a minor incompatibility, but a client that hard-codes > itself to parsing "returns":0 (that is, expecting a json-number) > will fail when talking to an older qemu that provided a json-object > instead; while a client that expects a json-object always and can > search for a "key":0 integer pair within that object that may or > may not be present (we already document that clients MUST be > prepared for dictionary members to be optional, but do NOT advise > that clients must be prepared for a change between fundamental JSON > types). Yes, the backwards-incompatibility is a weak argument, but > as my pending series will be requiring all new commands to > whitelist a non-dict return, we might as well avoid making this > another case for that whitelist. That's true, and we might take the opportunity to return the number of trimmed bytes separately for each filesystem. Justin, would you like to take a shot at that, or just submit a new version that doesn't include the change to the return value? Thanks, Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVHCGwAAoJEL/70l94x66DuMcH/1Sb013BWNwot+I0jhMEr7jF WgaXQgIAVlxtrzspuE4cXF/TszCie/G1cGlF2oLP+GkJVivbAJFqJYNZcNDfE2Ti OdfhyUCgYMhOYUG81IXIbwXlGca/RhuDW74+B+tEL9dnoissI4l5JWS+k4bWK3On Pgu3gmIcLtQKTxUeDi93K25OAZNJJ9mLmf8CF4FAOUv3C5T4SN5tSkSEqQSB52by zKcM4+cXg5fzYH5OLo6d2SEUqsUHoEVh75VKLb38vAYm3GuNcuVoMEnlSELc3nTo A8Zrmc7swjZXqaZ6iRaXz7seiJ69XrfNWGqY+s0rGZoUK3z8/LwGSpDgmAY3pVQ= =Goct -----END PGP SIGNATURE-----
Hello, I posted a PATCH v2 yesterday that does this. It returns the result per filesystem, with the name, mountpoint and filesystem type. Eric requested output similar to fsinfo (that's why name and type is included), but I'm not really convinced it is meaningful. Especially 'name' is something that has no real meaning, but determining it is one of the most likely failure cases for this operation. I included type because to complement the 3 properties of fsinfo, and since it at least conveys some information about a possible returned error for that mountpoint and it is trivially available. But a FITRIM operation may be performed on any directory (not necessarily a mountpoint, though that is the sensible path to use when trimming all filesystems). And I can imagine someone extending the 'guest-fstrim' operation to take an optional path(s) argument, but in that case you don't want to report name, mountpoint and type but only a "path" (which in the current implementation would be the mountpoint). Another issue I ran into is that this is a batch operation. I changed the result to include a "result" enum (success / operation-failed) and if it is a "success" than the "trimmid" and "minimum" fields for that mountpoint are returned, if it is an "operation-failed" than the "error" (with the specific errno value) field is returned. But I'm unsure if this is how one would want to use this return type. Pherhaps it would be better to drop the "result" field and mirror the basic return logic: "error" attribute on error and a "return" attribute with a dictionary containing the per mountpoint result? Regards, Justin On 01-04-15 18:49, Paolo Bonzini wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > > > On 01/04/2015 14:33, Eric Blake wrote: >> It's only a minor incompatibility, but a client that hard-codes >> itself to parsing "returns":0 (that is, expecting a json-number) >> will fail when talking to an older qemu that provided a json-object >> instead; while a client that expects a json-object always and can >> search for a "key":0 integer pair within that object that may or >> may not be present (we already document that clients MUST be >> prepared for dictionary members to be optional, but do NOT advise >> that clients must be prepared for a change between fundamental JSON >> types). Yes, the backwards-incompatibility is a weak argument, but >> as my pending series will be requiring all new commands to >> whitelist a non-dict return, we might as well avoid making this >> another case for that whitelist. > > That's true, and we might take the opportunity to return the number of > trimmed bytes separately for each filesystem. > > Justin, would you like to take a shot at that, or just submit a new > version that doesn't include the change to the return value? > > Thanks, > > Paolo > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2 > > iQEcBAEBCAAGBQJVHCGwAAoJEL/70l94x66DuMcH/1Sb013BWNwot+I0jhMEr7jF > WgaXQgIAVlxtrzspuE4cXF/TszCie/G1cGlF2oLP+GkJVivbAJFqJYNZcNDfE2Ti > OdfhyUCgYMhOYUG81IXIbwXlGca/RhuDW74+B+tEL9dnoissI4l5JWS+k4bWK3On > Pgu3gmIcLtQKTxUeDi93K25OAZNJJ9mLmf8CF4FAOUv3C5T4SN5tSkSEqQSB52by > zKcM4+cXg5fzYH5OLo6d2SEUqsUHoEVh75VKLb38vAYm3GuNcuVoMEnlSELc3nTo > A8Zrmc7swjZXqaZ6iRaXz7seiJ69XrfNWGqY+s0rGZoUK3z8/LwGSpDgmAY3pVQ= > =Goct > -----END PGP SIGNATURE----- > >
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. # # Since: 1.2 ## { 'command': 'guest-fstrim', - 'data': { '*minimum': 'int' } } + 'data': { '*minimum': 'int' }, + 'returns': 'int' } ## # @guest-suspend-disk
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(-)