From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnrRM-00023n-Kx for qemu-devel@nongnu.org; Thu, 30 Apr 2015 12:35:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnrRI-0002ze-5L for qemu-devel@nongnu.org; Thu, 30 Apr 2015 12:35:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnrRH-0002zK-Tz for qemu-devel@nongnu.org; Thu, 30 Apr 2015 12:35:52 -0400 Date: Thu, 30 Apr 2015 18:35:40 +0200 From: Thomas Huth Message-ID: <20150430183540.3c78a19b@thh440s> In-Reply-To: <1430404198-10324-3-git-send-email-justin@quarantainenet.nl> References: <1430404198-10324-1-git-send-email-justin@quarantainenet.nl> <1430404198-10324-3-git-send-email-justin@quarantainenet.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/2] qga/commands-posix: Return per path fstrim result List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, Justin Ossevoort On Thu, 30 Apr 2015 16:29:58 +0200 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. > > Signed-off-by: Justin Ossevoort > --- > qga/commands-posix.c | 54 ++++++++++++++++++++++++++++++++++++++-------------- > qga/qapi-schema.json | 32 ++++++++++++++++++++++++++++--- > 2 files changed, 69 insertions(+), 17 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; I'm not sure, but does this "minimum" result make sense at all? What's the kernel supposed to return in this field? I had a quick look at some file system implementations in the kernel, but to me it seems like only the .len field is updated with a return value. > + result->has_trimmed = true; > + result->trimmed = r.len; > close(fd); > } > > -error: > free_fs_mount_list(&mounts); > + return response; > } > #endif /* CONFIG_FSTRIM */ I just also had a quick test of this patch and got this behaviour: {"execute":"guest-fstrim"} {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 2040348672}, {"minimum": 4096, "path": "/mnt2", "trimmed": 2040348672}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699807232}]}} {"execute":"guest-fstrim"} {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}} {"execute":"guest-fstrim"} {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}} {"execute":"guest-fstrim"} {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}} {"execute":"guest-fstrim"} {"return": {"paths": [{"minimum": 4096, "path": "/mnt", "trimmed": 0}, {"minimum": 4096, "path": "/mnt2", "trimmed": 0}, {"minimum": 0, "path": "/boot", "trimmed": 388968448}, {"minimum": 0, "path": "/", "trimmed": 17699799040}]}} /mnt and /mnt2 got successfully trimmed and consecutive calls then reported "trimmed: 0". But the values for "/boot" and "/" do not make sense to me, why does it claim to have always trimmed the same amount of bytes here? (I only touched the /mnt and /mnt2 file systems before doing the trim calls, so I wonder why there are bytes trimmed on / and /boot at all?) Also, I think you need to adjust the stub of qmp_guest_fstrim() in commands-win32.c, too, so that you don't break the compilation of the windows target. Thomas