From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ywq9P-0000ht-1a for qemu-devel@nongnu.org; Mon, 25 May 2015 07:02:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ywq9J-0006SV-2f for qemu-devel@nongnu.org; Mon, 25 May 2015 07:02:30 -0400 Received: from mx2.parallels.com ([199.115.105.18]:44077) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ywq9I-0006Rx-R5 for qemu-devel@nongnu.org; Mon, 25 May 2015 07:02:25 -0400 Message-ID: <5562FC52.80908@parallels.com> Date: Mon, 25 May 2015 13:41:22 +0300 From: Olga Krishtal MIME-Version: 1.0 References: <1431327525-25625-1-git-send-email-justin@quarantainenet.nl> <1431327525-25625-3-git-send-email-justin@quarantainenet.nl> In-Reply-To: <1431327525-25625-3-git-send-email-justin@quarantainenet.nl> Content-Type: multipart/alternative; boundary="------------060403060902080909000809" Subject: Re: [Qemu-devel] [PATCH v4 2/2] qga/qmp_guest_fstrim: Return per path fstrim result List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Justin Ossevoort , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com --------------060403060902080909000809 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit On 11/05/15 09:58, Justin Ossevoort 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. > > The returned values for errors, minimum and trimmed are dependant on the > filesystem, storage stacks and kernel version. > > Signed-off-by: Justin Ossevoort > --- > 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 The path means the mount point of fs? > +# @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'} } Actually i am not quite sure about the whole structure, because when someone decides to implement this command for Windows they may face some difficulties. For Win 8 we have only ioctl FSCTL_FILE_LEVEL_TRIM, that allows as to specify https://msdn.microsoft.com/en-us/library/windows/desktop/hh447300(v=vs.85).aspx - Offset, in bytes, from the start of the file for the range to be trimmed. - Length, in bytes, for the range to be trimmed. And as output we have Contains the number of ranges that were successfully processed. If as path fs mountpoint will be used, then what should be done with other variables? > +# > +## > +# @GuestFilesystemTrimResponse > +# > +# @paths: list of @GuestFilesystemTrimResult per path that was trimmed > +# > +# Since: 2.4 > +## > +{ 'type': 'GuestFilesystemTrimResponse', > + 'data': {'paths': ['GuestFilesystemTrimResult']} } Afaik instead of type, struct should be used > +## > # @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 --------------060403060902080909000809 Content-Type: text/html; charset="windows-1252" Content-Transfer-Encoding: 8bit
On 11/05/15 09:58, Justin Ossevoort 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.

The returned values for errors, minimum and trimmed are dependant on the
filesystem, storage stacks and kernel version.

Signed-off-by: Justin Ossevoort <justin@quarantainenet.nl>
---
 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
The path means the mount point of fs? 
+# @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'} }
Actually i am not quite sure about the whole structure, because when someone decides to implement
this command for Windows they may face some difficulties.
For Win 8 we have only ioctl  FSCTL_FILE_LEVEL_TRIM, that allows as to specify
https://msdn.microsoft.com/en-us/library/windows/desktop/hh447300(v=vs.85).aspx
- Offset, in bytes, from the start of the file for the range to be trimmed.
- Length, in bytes, for the range to be trimmed.

And as output we have
Contains the number of ranges that were successfully processed.
If as path fs mountpoint will be used, then what should be done with other variables?
 
+#
+##
+# @GuestFilesystemTrimResponse
+#
+# @paths: list of @GuestFilesystemTrimResult per path that was trimmed
+#
+# Since: 2.4
+##
+{ 'type': 'GuestFilesystemTrimResponse',
+  'data': {'paths': ['GuestFilesystemTrimResult']} }
Afaik instead of type, struct should be used
+##
 # @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

--------------060403060902080909000809--