Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests

[Laszlo Ersek]

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 <address@hidden>
> Signed-off-by: Denis Plotnikov <address@hidden>
> CC: Michael Roth <address@hidden>
> CC: Stefan Weil <address@hidden>
> CC: Marc-André Lureau <address@hidden>
> ---
>  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) {
>