From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60657) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YoAGN-00089C-6Z for qemu-devel@nongnu.org; Fri, 01 May 2015 08:41:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YoAGH-0007WS-T0 for qemu-devel@nongnu.org; Fri, 01 May 2015 08:41:51 -0400 Received: from [2001:1540:801:3:21e:67ff:fe7a:3e14] (port=38673 helo=champignon.inhetmidden.nl) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YoAGH-0007W7-JH for qemu-devel@nongnu.org; Fri, 01 May 2015 08:41:45 -0400 Message-ID: <554369D9.4030808@internetionals.nl> Date: Fri, 01 May 2015 13:56:09 +0200 From: Justin Ossevoort MIME-Version: 1.0 References: <1430404198-10324-1-git-send-email-justin@quarantainenet.nl> <1430404198-10324-3-git-send-email-justin@quarantainenet.nl> <20150430183540.3c78a19b@thh440s> In-Reply-To: <20150430183540.3c78a19b@thh440s> Content-Type: text/plain; charset=utf-8; format=flowed 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 On 30-04-15 18:35, Thomas Huth wrote: > 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. Without having looked at the actual implementation in the kernel, but based on discussions on the linux-kernel mailinglist, should the minimum reflect the actual minimum size it was able to trim. I'm not certain if it's the minimum size supported by all parts of the storage stack or if it was the size of the smallest consecutive area trimmed. It is only filled if something actually got trimmed. But that is still logical for both implementations. The reason I'm returning it, is because it get's updated by the kernel, so it might mean something to someone. For my use-cases 'trimmed' is meaningful for monitoring, and per file system errors are useful in discovering why some filesystem might not get trimmed (and ensuring all filesystems actually get trimmed). I personally have no use for this 'minimum', so dropping it is fine by me. >> + 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?) It will probably have trimmed the entire free space map the first time. I don't know if filesystem trim information is persistent or in-memory only. I suspect the response is filesystem / storage stack specific. > 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. Good call, will fix. > Thomas Thanks for ther review and tests. Regards, Justin