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

[Justin Ossevoort]

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
returns a list of all trimmed filesystems with their status. If the
trime operation succeeded it returns the amount of bytes trimmed and
the effective minimum chunk size. On error it returns the value of
the errno for that filesystem operation (instead of aborting the
request).

Signed-off-by: Justin Ossevoort <address@hidden>
---
 qga/commands-posix.c | 106 +++++++++++++++++++++++++++++++++++++++++++--------
 qga/qapi-schema.json |  49 ++++++++++++++++++++++--
 2 files changed, 136 insertions(+), 19 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ba8de62..01ac801 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1322,21 +1322,52 @@ static void guest_fsfreeze_cleanup(void)
 #endif /* CONFIG_FSFREEZE */
 
 #if defined(CONFIG_FSTRIM)
+/* Add a fstrim result for a filesystem specified by @mount to @response */
+static GuestFilesystemTrimResult *
+qmp_guest_fstrim_add_result(GuestFilesystemTrimResponse *response,
+                            struct FsMount *mount, Error **errp)
+{
+    char *devpath, *syspath;
+    GuestFilesystemTrimResult *result = NULL;
+    GuestFilesystemTrimResultList *list;
+
+    devpath = g_strdup_printf("/sys/dev/block/%u:%u", mount->devmajor,
+                              mount->devminor);
+    syspath = realpath(devpath, NULL);
+    if (!syspath) {
+        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
+        goto out;
+    }
+
+    result = g_malloc0(sizeof(*result));
+    result->name = g_strdup(basename(syspath));
+    result->type = g_strdup(mount->devtype);
+    result->mountpoint = g_strdup(mount->dirname);
+
+    list = g_malloc0(sizeof(*list));
+    list->value = result;
+    list->next = response->trimmed;
+    response->trimmed = list;
+
+out:
+    free(devpath);
+    return result;
+}
+
 /*
  * 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)
 {
-    int ret = 0;
+    GuestFilesystemTrimResponse *response = NULL;
+    GuestFilesystemTrimResult *result;
+    int ret = 0, error;
     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;
 
     slog("guest-fstrim called");
 
@@ -1344,14 +1375,24 @@ 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) {
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
-            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
-            goto error;
+            error = errno;
+            result = qmp_guest_fstrim_add_result(response, mount, &local_err);
+            if (local_err) {
+                goto abort;
+            }
+            result->result =
+                GUEST_FILESYSTEM_TRIM_RESULT_TYPE_OPERATION_FAILED;
+            result->has_error_code = true;
+            result->error_code = error;
+            continue;
         }
 
         /* We try to cull filesytems we know won't work in advance, but other
@@ -1360,20 +1401,51 @@ 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) {
-                error_setg_errno(errp, errno, "failed to trim %s",
-                                 mount->dirname);
-                close(fd);
-                goto error;
+                error = errno;
+                result = qmp_guest_fstrim_add_result(response, mount,
+                                                     &local_err);
+                if (local_err) {
+                    goto abort;
+                }
+                result->result =
+                    GUEST_FILESYSTEM_TRIM_RESULT_TYPE_OPERATION_FAILED;
+                result->has_error_code = true;
+                result->error_code = error;
             }
+            close(fd);
+            continue;
         }
+
+        result = qmp_guest_fstrim_add_result(response, mount, &local_err);
+        if (local_err) {
+            goto abort;
+        }
+        result->result = GUEST_FILESYSTEM_TRIM_RESULT_TYPE_SUCCESS;
+        result->has_minimum = true;
+        result->minimum = r.minlen;
+        result->has_trimmed = true;
+        result->trimmed = r.len;
         close(fd);
     }
+    goto out;
 
-error:
+abort:
+    error_propagate(errp, local_err);
+    if (fd > 0) {
+        close(fd);
+    }
+    qapi_free_GuestFilesystemTrimResponse(response);
+    response = NULL;
+
+out:
     free_fs_mount_list(&mounts);
+    return response;
 }
 #endif /* CONFIG_FSTRIM */
 
@@ -2402,9 +2474,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/qapi-schema.json b/qga/qapi-schema.json
index 95f49e3..cc96742 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -425,6 +425,48 @@
   'returns': 'int' }
 
 ##
+# @GuestFilesystemTrimResultType
+#
+# An enumeration of filesystem trim operation result.
+#
+# @success: the operation of was succesful
+# @operation-failed: the filesystem reported a failure
+#
+# Since: 2.4
+##
+{ 'enum': 'GuestFilesystemTrimResultType',
+  'data': ['success', 'operation-failed'] }
+
+##
+# @GuestFilesystemTrimResult
+#
+# @name: disk name
+# @mountpoint: mount point path
+# @type: file system type string
+# @result: the result of the fstrim operation for this filesystem
+# @trimmed: bytes trimmed when result is success
+# @minimum: reported minimum considered for trimming when
+#           result is success
+# @error-code: the value of errno when response is not success
+#
+# Since: 2.4
+##
+{ 'type': 'GuestFilesystemTrimResult',
+  'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
+           'result': 'GuestFilesystemTrimResultType',
+           '*trimmed': 'int', '*minimum': 'int', '*error-code': 'int'} }
+
+##
+# @GuestFilesystemTrimResponse
+#
+# @trimmed: list of filesystem trim results
+#
+# Since: 2.4
+##
+{ 'type': 'GuestFilesystemTrimResponse',
+  'data': {'trimmed': ['GuestFilesystemTrimResult']} }
+
+##
 # @guest-fstrim:
 #
 # Discard (or "trim") blocks which are not in use by the filesystem.
@@ -437,12 +479,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
+# Since: 2.4
 ##
 { 'command': 'guest-fstrim',
-  'data': { '*minimum': 'int' } }
+  'data': { '*minimum': 'int' },
+  'returns': 'GuestFilesystemTrimResponse' }
 
 ##
 # @guest-suspend-disk
-- 
2.1.0