| Message ID | 1475503285-9021-1-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Oct 3, 2016 at 6:01 PM Denis V. Lunev <den@openvz.org> wrote: > Unfortunately, there is no public Windows API to start trimming the > filesystem. The only viable way here is to call 'defrag.exe /L' for > each volume. > > This is working since Win8 and Win2k12. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > CC: Michael Roth <mdroth@linux.vnet.ibm.com> > CC: Stefan Weil <sw@weilnetz.de> > CC: Marc-André Lureau <marcandre.lureau@gmail.com> > overall looks good to me, few remarks below: > --- > qga/commands-win32.c | 97 > ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 94 insertions(+), 3 deletions(-) > > Changes from v3: > - fixed memory leak on error path for FindFirstVolumeW > - replaced g_malloc0 with g_malloc for uc_path. g_malloc is better as we > are > allocating string, not an object > > Changes from v1, v2: > - next attempt to fix error handling on error in FindFirstVolumeW > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 9c9be12..cebf4cc 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void) > GuestFilesystemTrimResponse * > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp) > { > - error_setg(errp, QERR_UNSUPPORTED); > - return NULL; > + GuestFilesystemTrimResponse *resp; > + HANDLE handle; > + WCHAR guid[MAX_PATH] = L""; > + > + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid)); > + if (handle == INVALID_HANDLE_VALUE) { > + error_setg_win32(errp, GetLastError(), "failed to find any > volume"); > + return NULL; > + } > + > + resp = g_new0(GuestFilesystemTrimResponse, 1); > + > + do { > + GuestFilesystemTrimResult *res; > + GuestFilesystemTrimResultList *list; > + PWCHAR uc_path; > + DWORD char_count = 0; > + char *path, *out; > + GError *gerr = NULL; > + gchar * argv[4]; > + > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count); > + > It assumes GetVolumePathNamesForVolumeNameW() == 0, perhaps better be explicit about it with an assert() or a warning()? + if (GetLastError() != ERROR_MORE_DATA) { > Would it be useful to log the error in this case? > + continue; > + } > + if (GetDriveTypeW(guid) != DRIVE_FIXED) { > + continue; > + } > + > + uc_path = g_malloc(sizeof(WCHAR) * char_count); > + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, char_count, > + &char_count) || !*uc_path) { > + /* strange, but this condition could be faced even with size > == 2 */ > What size? Same remark regarding logging error. + g_free(uc_path); > + continue; > + } > + > + res = g_new0(GuestFilesystemTrimResult, 1); > + > + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, &gerr); > + > + g_free(uc_path); > + > + if (gerr != NULL && gerr->code) { > Why check gerr->code? To be consistent with error checking code, I would check if path == NULL instead, which by glib doc says that gerr will be set in this case. > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > + break; > + } > + > + res->path = path; > + > + list = g_new0(GuestFilesystemTrimResultList, 1); > + list->value = res; > + list->next = resp->paths; > + > + resp->paths = list; > + > + memset(argv, 0, sizeof(argv)); > + argv[0] = (gchar *)"defrag.exe"; > + argv[1] = (gchar *)"/L"; > + argv[2] = path; > + > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, NULL, > NULL, > + &out /* stdout */, NULL /* stdin */, > + NULL, &gerr)) { > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > It could use continue; here, like the other error code paths, to avoid the else indent? > + } else { > + /* defrag.exe is UGLY. Exit code is ALWAYS zero. > + Error is reported in the output with something like > + (x89000020) etc code in the stdout */ > + > + int i; > + gchar **lines = g_strsplit(out, "\r\n", 0); > + g_free(out); > + > + for (i = 0; lines[i] != NULL; i++) { > + if (g_strstr_len(lines[i], -1, "(0x") == NULL) { > + continue; > + } > + res->has_error = true; > + res->error = g_strdup(lines[i]); > + break; > + } > + g_strfreev(lines); > + } > + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid))); > + > + FindVolumeClose(handle); > + return resp; > } > > typedef enum { > @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList *blacklist) > "guest-get-memory-blocks", "guest-set-memory-blocks", > "guest-get-memory-block-size", > "guest-fsfreeze-freeze-list", > - "guest-fstrim", NULL}; > + NULL}; > char **p = (char **)list_unsupported; > > while (*p) { > -- > 2.7.4 > > -- Marc-André Lureau
On 10/04/2016 04:43 PM, Marc-André Lureau wrote: > Hi > > On Mon, Oct 3, 2016 at 6:01 PM Denis V. Lunev <den@openvz.org > <mailto:den@openvz.org>> wrote: > > Unfortunately, there is no public Windows API to start trimming the > filesystem. The only viable way here is to call 'defrag.exe /L' for > each volume. > > This is working since Win8 and Win2k12. > > Signed-off-by: Denis V. Lunev <den@openvz.org <mailto:den@openvz.org>> > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com > <mailto:dplotnikov@virtuozzo.com>> > CC: Michael Roth <mdroth@linux.vnet.ibm.com > <mailto:mdroth@linux.vnet.ibm.com>> > CC: Stefan Weil <sw@weilnetz.de <mailto:sw@weilnetz.de>> > CC: Marc-André Lureau <marcandre.lureau@gmail.com > <mailto:marcandre.lureau@gmail.com>> > > > overall looks good to me, few remarks below: > > > --- > qga/commands-win32.c | 97 > ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 94 insertions(+), 3 deletions(-) > > Changes from v3: > - fixed memory leak on error path for FindFirstVolumeW > - replaced g_malloc0 with g_malloc for uc_path. g_malloc is better > as we are > allocating string, not an object > > Changes from v1, v2: > - next attempt to fix error handling on error in FindFirstVolumeW > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 9c9be12..cebf4cc 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void) > GuestFilesystemTrimResponse * > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp) > { > - error_setg(errp, QERR_UNSUPPORTED); > - return NULL; > + GuestFilesystemTrimResponse *resp; > + HANDLE handle; > + WCHAR guid[MAX_PATH] = L""; > + > + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid)); > + if (handle == INVALID_HANDLE_VALUE) { > + error_setg_win32(errp, GetLastError(), "failed to find > any volume"); > + return NULL; > + } > + > + resp = g_new0(GuestFilesystemTrimResponse, 1); > + > + do { > + GuestFilesystemTrimResult *res; > + GuestFilesystemTrimResultList *list; > + PWCHAR uc_path; > + DWORD char_count = 0; > + char *path, *out; > + GError *gerr = NULL; > + gchar * argv[4]; > + > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count); > + > > > It assumes GetVolumePathNamesForVolumeNameW() == 0, perhaps better be > explicit about it with an assert() or a warning()? original assumption was that in this case we'll call GetVolumePathNamesForVolumeNameW() with the exactly the same parameter set and fail there. > > + if (GetLastError() != ERROR_MORE_DATA) { > > > Would it be useful to log the error in this case? > > > + continue; > + } > + if (GetDriveTypeW(guid) != DRIVE_FIXED) { > + continue; > + } > + > + uc_path = g_malloc(sizeof(WCHAR) * char_count); > > + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, > char_count, > + &char_count) || > !*uc_path) { > + /* strange, but this condition could be faced even > with size == 2 */ > > > What size? > with char_count == 2 > Same remark regarding logging error. > > + g_free(uc_path); > + continue; > + } > + > + res = g_new0(GuestFilesystemTrimResult, 1); > + > + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, > &gerr); > + > + g_free(uc_path); > + > + if (gerr != NULL && gerr->code) { > > > Why check gerr->code? To be consistent with error checking code, I > would check if path == NULL instead, which by glib doc says that gerr > will be set in this case. > ok > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > + break; > + } > + > + res->path = path; > + > + list = g_new0(GuestFilesystemTrimResultList, 1); > + list->value = res; > + list->next = resp->paths; > + > + resp->paths = list; > + > + memset(argv, 0, sizeof(argv)); > + argv[0] = (gchar *)"defrag.exe"; > + argv[1] = (gchar *)"/L"; > + argv[2] = path; > + > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, > NULL, NULL, > + &out /* stdout */, NULL /* stdin */, > + NULL, &gerr)) { > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > > > It could use continue; here, like the other error code paths, to avoid > the else indent? I need indent for local variable > > > + } else { > + /* defrag.exe is UGLY. Exit code is ALWAYS zero. > + Error is reported in the output with something like > + (x89000020) etc code in the stdout */ > + > + int i; > + gchar **lines = g_strsplit(out, "\r\n", 0); > + g_free(out); > + > + for (i = 0; lines[i] != NULL; i++) { > + if (g_strstr_len(lines[i], -1, "(0x") == NULL) { > + continue; > + } > + res->has_error = true; > + res->error = g_strdup(lines[i]); > + break; > + } > + g_strfreev(lines); > + } > + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid))); > + > + FindVolumeClose(handle); > + return resp; > } > > typedef enum { > @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList > *blacklist) > "guest-get-memory-blocks", "guest-set-memory-blocks", > "guest-get-memory-block-size", > "guest-fsfreeze-freeze-list", > - "guest-fstrim", NULL}; > + NULL}; > char **p = (char **)list_unsupported; > > while (*p) { > -- > 2.7.4 > > -- > Marc-André Lureau
On Mon, Oct 03, 2016 at 05:01:25PM +0300, Denis V. Lunev wrote: > Unfortunately, there is no public Windows API to start trimming the > filesystem. The only viable way here is to call 'defrag.exe /L' for > each volume. It's good to know that at least in one way, ntfs-3g is more featureful than Windows :-) > This is working since Win8 and Win2k12. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > CC: Michael Roth <mdroth@linux.vnet.ibm.com> > CC: Stefan Weil <sw@weilnetz.de> > CC: Marc-André Lureau <marcandre.lureau@gmail.com> > --- > qga/commands-win32.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 94 insertions(+), 3 deletions(-) > > Changes from v3: > - fixed memory leak on error path for FindFirstVolumeW > - replaced g_malloc0 with g_malloc for uc_path. g_malloc is better as we are > allocating string, not an object > > Changes from v1, v2: > - next attempt to fix error handling on error in FindFirstVolumeW > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 9c9be12..cebf4cc 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void) > GuestFilesystemTrimResponse * > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp) > { > - error_setg(errp, QERR_UNSUPPORTED); > - return NULL; > + GuestFilesystemTrimResponse *resp; > + HANDLE handle; > + WCHAR guid[MAX_PATH] = L""; > + > + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid)); > + if (handle == INVALID_HANDLE_VALUE) { > + error_setg_win32(errp, GetLastError(), "failed to find any volume"); > + return NULL; > + } > + > + resp = g_new0(GuestFilesystemTrimResponse, 1); > + > + do { > + GuestFilesystemTrimResult *res; > + GuestFilesystemTrimResultList *list; > + PWCHAR uc_path; > + DWORD char_count = 0; > + char *path, *out; > + GError *gerr = NULL; > + gchar * argv[4]; > + > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count); > + > + if (GetLastError() != ERROR_MORE_DATA) { > + continue; > + } > + if (GetDriveTypeW(guid) != DRIVE_FIXED) { > + continue; > + } > + > + uc_path = g_malloc(sizeof(WCHAR) * char_count); > + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, char_count, > + &char_count) || !*uc_path) { > + /* strange, but this condition could be faced even with size == 2 */ > + g_free(uc_path); > + continue; > + } > + > + res = g_new0(GuestFilesystemTrimResult, 1); > + > + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, &gerr); > + > + g_free(uc_path); > + > + if (gerr != NULL && gerr->code) { > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > + break; > + } > + > + res->path = path; > + > + list = g_new0(GuestFilesystemTrimResultList, 1); > + list->value = res; > + list->next = resp->paths; > + > + resp->paths = list; > + > + memset(argv, 0, sizeof(argv)); > + argv[0] = (gchar *)"defrag.exe"; > + argv[1] = (gchar *)"/L"; > + argv[2] = path; > + > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, > + &out /* stdout */, NULL /* stdin */, > + NULL, &gerr)) { > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > + } else { > + /* defrag.exe is UGLY. Exit code is ALWAYS zero. > + Error is reported in the output with something like > + (x89000020) etc code in the stdout */ > + > + int i; > + gchar **lines = g_strsplit(out, "\r\n", 0); > + g_free(out); > + > + for (i = 0; lines[i] != NULL; i++) { > + if (g_strstr_len(lines[i], -1, "(0x") == NULL) { > + continue; > + } > + res->has_error = true; > + res->error = g_strdup(lines[i]); > + break; > + } > + g_strfreev(lines); > + } > + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid))); > + > + FindVolumeClose(handle); > + return resp; > } > > typedef enum { > @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList *blacklist) > "guest-get-memory-blocks", "guest-set-memory-blocks", > "guest-get-memory-block-size", > "guest-fsfreeze-freeze-list", > - "guest-fstrim", NULL}; > + NULL}; > char **p = (char **)list_unsupported; > > while (*p) { The patch looks good to me. It's a bit of a shame that we have to grep the output for "(0x" and hope that that is the only way that error can be reported, but not much else we can do. Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich.
On 10/03/16 16:01, Denis V. Lunev wrote: > Unfortunately, there is no public Windows API to start trimming the > filesystem. The only viable way here is to call 'defrag.exe /L' for > each volume. > > This is working since Win8 and Win2k12. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > CC: Michael Roth <mdroth@linux.vnet.ibm.com> > CC: Stefan Weil <sw@weilnetz.de> > CC: Marc-André Lureau <marcandre.lureau@gmail.com> > --- > qga/commands-win32.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 94 insertions(+), 3 deletions(-) Just to educate myself (really, you can ignore my question as "review comment", because it's not one): why is this necessary? In Windows 8 and Windows Server 2012 R2, simply putting your NTFS filesystem on a SCSI disk (such as virtio-scsi-pci / scsi-hd) or AHCI, and specifying discard=on for the -drive, will result in the guest automatically trimming the NTFS (with a little delay) after deleting files, and the host getting those blocks back. Thanks Laszlo > Changes from v3: > - fixed memory leak on error path for FindFirstVolumeW > - replaced g_malloc0 with g_malloc for uc_path. g_malloc is better as we are > allocating string, not an object > > Changes from v1, v2: > - next attempt to fix error handling on error in FindFirstVolumeW > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 9c9be12..cebf4cc 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void) > GuestFilesystemTrimResponse * > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp) > { > - error_setg(errp, QERR_UNSUPPORTED); > - return NULL; > + GuestFilesystemTrimResponse *resp; > + HANDLE handle; > + WCHAR guid[MAX_PATH] = L""; > + > + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid)); > + if (handle == INVALID_HANDLE_VALUE) { > + error_setg_win32(errp, GetLastError(), "failed to find any volume"); > + return NULL; > + } > + > + resp = g_new0(GuestFilesystemTrimResponse, 1); > + > + do { > + GuestFilesystemTrimResult *res; > + GuestFilesystemTrimResultList *list; > + PWCHAR uc_path; > + DWORD char_count = 0; > + char *path, *out; > + GError *gerr = NULL; > + gchar * argv[4]; > + > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count); > + > + if (GetLastError() != ERROR_MORE_DATA) { > + continue; > + } > + if (GetDriveTypeW(guid) != DRIVE_FIXED) { > + continue; > + } > + > + uc_path = g_malloc(sizeof(WCHAR) * char_count); > + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, char_count, > + &char_count) || !*uc_path) { > + /* strange, but this condition could be faced even with size == 2 */ > + g_free(uc_path); > + continue; > + } > + > + res = g_new0(GuestFilesystemTrimResult, 1); > + > + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, &gerr); > + > + g_free(uc_path); > + > + if (gerr != NULL && gerr->code) { > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > + break; > + } > + > + res->path = path; > + > + list = g_new0(GuestFilesystemTrimResultList, 1); > + list->value = res; > + list->next = resp->paths; > + > + resp->paths = list; > + > + memset(argv, 0, sizeof(argv)); > + argv[0] = (gchar *)"defrag.exe"; > + argv[1] = (gchar *)"/L"; > + argv[2] = path; > + > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, > + &out /* stdout */, NULL /* stdin */, > + NULL, &gerr)) { > + res->has_error = true; > + res->error = g_strdup(gerr->message); > + g_error_free(gerr); > + } else { > + /* defrag.exe is UGLY. Exit code is ALWAYS zero. > + Error is reported in the output with something like > + (x89000020) etc code in the stdout */ > + > + int i; > + gchar **lines = g_strsplit(out, "\r\n", 0); > + g_free(out); > + > + for (i = 0; lines[i] != NULL; i++) { > + if (g_strstr_len(lines[i], -1, "(0x") == NULL) { > + continue; > + } > + res->has_error = true; > + res->error = g_strdup(lines[i]); > + break; > + } > + g_strfreev(lines); > + } > + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid))); > + > + FindVolumeClose(handle); > + return resp; > } > > typedef enum { > @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList *blacklist) > "guest-get-memory-blocks", "guest-set-memory-blocks", > "guest-get-memory-block-size", > "guest-fsfreeze-freeze-list", > - "guest-fstrim", NULL}; > + NULL}; > char **p = (char **)list_unsupported; > > while (*p) { >
On 10/05/2016 09:55 PM, Laszlo Ersek wrote: > On 10/03/16 16:01, Denis V. Lunev wrote: >> Unfortunately, there is no public Windows API to start trimming the >> filesystem. The only viable way here is to call 'defrag.exe /L' for >> each volume. >> >> This is working since Win8 and Win2k12. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> CC: Michael Roth <mdroth@linux.vnet.ibm.com> >> CC: Stefan Weil <sw@weilnetz.de> >> CC: Marc-André Lureau <marcandre.lureau@gmail.com> >> --- >> qga/commands-win32.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 94 insertions(+), 3 deletions(-) > Just to educate myself (really, you can ignore my question as "review > comment", because it's not one): why is this necessary? In Windows 8 and > Windows Server 2012 R2, simply putting your NTFS filesystem on a SCSI > disk (such as virtio-scsi-pci / scsi-hd) or AHCI, and specifying > discard=on for the -drive, will result in the guest automatically > trimming the NTFS (with a little delay) after deleting files, and the > host getting those blocks back. The same as for Linux. But if the one exact block has been freed by half at one operation and by another half in the different operation as far as I could understand it will not be freed. This patch implements the ability to trim all the disc as done for Linux to go over all the disc and discard all possible areas. I think that this would be useful f.e. to prepare template images. Den
On 10/05/16 21:47, Denis V. Lunev wrote: > On 10/05/2016 09:55 PM, Laszlo Ersek wrote: >> On 10/03/16 16:01, Denis V. Lunev wrote: >>> Unfortunately, there is no public Windows API to start trimming the >>> filesystem. The only viable way here is to call 'defrag.exe /L' for >>> each volume. >>> >>> This is working since Win8 and Win2k12. >>> >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >>> CC: Michael Roth <mdroth@linux.vnet.ibm.com> >>> CC: Stefan Weil <sw@weilnetz.de> >>> CC: Marc-André Lureau <marcandre.lureau@gmail.com> >>> --- >>> qga/commands-win32.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 94 insertions(+), 3 deletions(-) >> Just to educate myself (really, you can ignore my question as "review >> comment", because it's not one): why is this necessary? In Windows 8 and >> Windows Server 2012 R2, simply putting your NTFS filesystem on a SCSI >> disk (such as virtio-scsi-pci / scsi-hd) or AHCI, and specifying >> discard=on for the -drive, will result in the guest automatically >> trimming the NTFS (with a little delay) after deleting files, and the >> host getting those blocks back. > The same as for Linux. But if the one exact block has been freed > by half at one operation and by another half in the different operation > as far as I could understand it will not be freed. > > This patch implements the ability to trim all the disc as done for Linux > to go over all the disc and discard all possible areas. I think that this > would be useful f.e. to prepare template images. Thank you for explaining. And, now I've learned about "defrag /L" too :) Cheers Laszlo
Quoting Denis V. Lunev (2016-10-05 06:13:12) > On 10/04/2016 04:43 PM, Marc-André Lureau wrote: > > Hi > > > > On Mon, Oct 3, 2016 at 6:01 PM Denis V. Lunev <den@openvz.org > > <mailto:den@openvz.org>> wrote: > > > > Unfortunately, there is no public Windows API to start trimming the > > filesystem. The only viable way here is to call 'defrag.exe /L' for > > each volume. > > > > This is working since Win8 and Win2k12. > > > > Signed-off-by: Denis V. Lunev <den@openvz.org <mailto:den@openvz.org>> > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com > > <mailto:dplotnikov@virtuozzo.com>> > > CC: Michael Roth <mdroth@linux.vnet.ibm.com > > <mailto:mdroth@linux.vnet.ibm.com>> > > CC: Stefan Weil <sw@weilnetz.de <mailto:sw@weilnetz.de>> > > CC: Marc-André Lureau <marcandre.lureau@gmail.com > > <mailto:marcandre.lureau@gmail.com>> > > > > > > overall looks good to me, few remarks below: > > > > > > --- > > qga/commands-win32.c | 97 > > ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 94 insertions(+), 3 deletions(-) > > > > Changes from v3: > > - fixed memory leak on error path for FindFirstVolumeW > > - replaced g_malloc0 with g_malloc for uc_path. g_malloc is better > > as we are > > allocating string, not an object > > > > Changes from v1, v2: > > - next attempt to fix error handling on error in FindFirstVolumeW > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index 9c9be12..cebf4cc 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void) > > GuestFilesystemTrimResponse * > > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp) > > { > > - error_setg(errp, QERR_UNSUPPORTED); > > - return NULL; > > + GuestFilesystemTrimResponse *resp; > > + HANDLE handle; > > + WCHAR guid[MAX_PATH] = L""; > > + > > + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid)); > > + if (handle == INVALID_HANDLE_VALUE) { > > + error_setg_win32(errp, GetLastError(), "failed to find > > any volume"); > > + return NULL; > > + } > > + > > + resp = g_new0(GuestFilesystemTrimResponse, 1); > > + > > + do { > > + GuestFilesystemTrimResult *res; > > + GuestFilesystemTrimResultList *list; > > + PWCHAR uc_path; > > + DWORD char_count = 0; > > + char *path, *out; > > + GError *gerr = NULL; > > + gchar * argv[4]; > > + > > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count); > > + > > > > > > It assumes GetVolumePathNamesForVolumeNameW() == 0, perhaps better be > > explicit about it with an assert() or a warning()? > original assumption was that in this case we'll call > GetVolumePathNamesForVolumeNameW() > with the exactly the same parameter set and fail there. > > > > > > + if (GetLastError() != ERROR_MORE_DATA) { > > > > > > Would it be useful to log the error in this case? > > > > > > + continue; > > + } > > + if (GetDriveTypeW(guid) != DRIVE_FIXED) { > > + continue; > > + } > > + > > + uc_path = g_malloc(sizeof(WCHAR) * char_count); > > > > + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, > > char_count, > > + &char_count) || > > !*uc_path) { > > + /* strange, but this condition could be faced even > > with size == 2 */ > > > > > > What size? > > > with char_count == 2 > > > Same remark regarding logging error. > > > > + g_free(uc_path); > > + continue; > > + } > > + > > + res = g_new0(GuestFilesystemTrimResult, 1); > > + > > + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, > > &gerr); > > + > > + g_free(uc_path); > > + > > + if (gerr != NULL && gerr->code) { > > > > > > Why check gerr->code? To be consistent with error checking code, I > > would check if path == NULL instead, which by glib doc says that gerr > > will be set in this case. > > > ok Thanks, applied to qga tree with the above suggestion squashed in: https://github.com/mdroth/qemu/commits/qga > > > + res->has_error = true; > > + res->error = g_strdup(gerr->message); > > + g_error_free(gerr); > > + break; > > + } > > + > > + res->path = path; > > + > > + list = g_new0(GuestFilesystemTrimResultList, 1); > > + list->value = res; > > + list->next = resp->paths; > > + > > + resp->paths = list; > > + > > + memset(argv, 0, sizeof(argv)); > > + argv[0] = (gchar *)"defrag.exe"; > > + argv[1] = (gchar *)"/L"; > > + argv[2] = path; > > + > > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, > > NULL, NULL, > > + &out /* stdout */, NULL /* stdin */, > > + NULL, &gerr)) { > > + res->has_error = true; > > + res->error = g_strdup(gerr->message); > > + g_error_free(gerr); > > > > > > It could use continue; here, like the other error code paths, to avoid > > the else indent? > I need indent for local variable > > > > > > > + } else { > > + /* defrag.exe is UGLY. Exit code is ALWAYS zero. > > + Error is reported in the output with something like > > + (x89000020) etc code in the stdout */ > > + > > + int i; > > + gchar **lines = g_strsplit(out, "\r\n", 0); > > + g_free(out); > > + > > + for (i = 0; lines[i] != NULL; i++) { > > + if (g_strstr_len(lines[i], -1, "(0x") == NULL) { > > + continue; > > + } > > + res->has_error = true; > > + res->error = g_strdup(lines[i]); > > + break; > > + } > > + g_strfreev(lines); > > + } > > + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid))); > > + > > + FindVolumeClose(handle); > > + return resp; > > } > > > > typedef enum { > > @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList > > *blacklist) > > "guest-get-memory-blocks", "guest-set-memory-blocks", > > "guest-get-memory-block-size", > > "guest-fsfreeze-freeze-list", > > - "guest-fstrim", NULL}; > > + NULL}; > > char **p = (char **)list_unsupported; > > > > while (*p) { > > -- > > 2.7.4 > > > > -- > > Marc-André Lureau > >
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 9c9be12..cebf4cc 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void) GuestFilesystemTrimResponse * qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp) { - error_setg(errp, QERR_UNSUPPORTED); - return NULL; + GuestFilesystemTrimResponse *resp; + HANDLE handle; + WCHAR guid[MAX_PATH] = L""; + + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid)); + if (handle == INVALID_HANDLE_VALUE) { + error_setg_win32(errp, GetLastError(), "failed to find any volume"); + return NULL; + } + + resp = g_new0(GuestFilesystemTrimResponse, 1); + + do { + GuestFilesystemTrimResult *res; + GuestFilesystemTrimResultList *list; + PWCHAR uc_path; + DWORD char_count = 0; + char *path, *out; + GError *gerr = NULL; + gchar * argv[4]; + + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count); + + if (GetLastError() != ERROR_MORE_DATA) { + continue; + } + if (GetDriveTypeW(guid) != DRIVE_FIXED) { + continue; + } + + uc_path = g_malloc(sizeof(WCHAR) * char_count); + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, char_count, + &char_count) || !*uc_path) { + /* strange, but this condition could be faced even with size == 2 */ + g_free(uc_path); + continue; + } + + res = g_new0(GuestFilesystemTrimResult, 1); + + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, &gerr); + + g_free(uc_path); + + if (gerr != NULL && gerr->code) { + res->has_error = true; + res->error = g_strdup(gerr->message); + g_error_free(gerr); + break; + } + + res->path = path; + + list = g_new0(GuestFilesystemTrimResultList, 1); + list->value = res; + list->next = resp->paths; + + resp->paths = list; + + memset(argv, 0, sizeof(argv)); + argv[0] = (gchar *)"defrag.exe"; + argv[1] = (gchar *)"/L"; + argv[2] = path; + + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, + &out /* stdout */, NULL /* stdin */, + NULL, &gerr)) { + res->has_error = true; + res->error = g_strdup(gerr->message); + g_error_free(gerr); + } else { + /* defrag.exe is UGLY. Exit code is ALWAYS zero. + Error is reported in the output with something like + (x89000020) etc code in the stdout */ + + int i; + gchar **lines = g_strsplit(out, "\r\n", 0); + g_free(out); + + for (i = 0; lines[i] != NULL; i++) { + if (g_strstr_len(lines[i], -1, "(0x") == NULL) { + continue; + } + res->has_error = true; + res->error = g_strdup(lines[i]); + break; + } + g_strfreev(lines); + } + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid))); + + FindVolumeClose(handle); + return resp; } typedef enum { @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList *blacklist) "guest-get-memory-blocks", "guest-set-memory-blocks", "guest-get-memory-block-size", "guest-fsfreeze-freeze-list", - "guest-fstrim", NULL}; + NULL}; char **p = (char **)list_unsupported; while (*p) {