[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Michael Roth]

On 02/01/2011 04:58 AM, address@hidden wrote:
> From: Jes Sorensen<address@hidden>
>
> Implement freeze/thaw support in the guest, allowing the host to
> request the guest freezes all it's file systems before a live snapshot
> is performed.
>   - fsfreeze(): Walk the list of mounted local real file systems,
>                 and freeze them.
>   - fsthaw():   Walk the list of previously frozen file systems and
>                 thaw them.
>   - fsstatus(): Return the current status of freeze/thaw. The host must
>                 poll this function, in case fsfreeze() returned with a
>                timeout, to wait for the operation to finish.
>
> Signed-off-by: Jes Sorensen<address@hidden>
> ---
>   virtagent-common.h |    8 ++
>   virtagent-server.c |  196 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 204 insertions(+), 0 deletions(-)
>
> diff --git a/virtagent-common.h b/virtagent-common.h
> index 5d8f5c1..220a4b6 100644
> --- a/virtagent-common.h
> +++ b/virtagent-common.h
> @@ -61,6 +61,14 @@ typedef struct VAContext {
>       const char *channel_path;
>   } VAContext;
>
> +enum vs_fsfreeze_status {
> +    FREEZE_ERROR = -1,
> +    FREEZE_THAWED = 0,
> +    FREEZE_INPROGRESS = 1,
> +    FREEZE_FROZEN = 2,
> +    FREEZE_THAWINPROGRESS = 3,
> +};

Any reason for vs_* vs. va_*?

> +
>   enum va_job_status {
>       VA_JOB_STATUS_PENDING = 0,
>       VA_JOB_STATUS_OK,
> diff --git a/virtagent-server.c b/virtagent-server.c
> index 7bb35b2..cf2a3f0 100644
> --- a/virtagent-server.c
> +++ b/virtagent-server.c
> @@ -14,6 +14,13 @@
>   #include<syslog.h>
>   #include "qemu_socket.h"
>   #include "virtagent-common.h"
> +#include<mntent.h>
> +#include<sys/types.h>
> +#include<sys/stat.h>
> +#include<sys/errno.h>
> +#include<sys/ioctl.h>
> +#include<fcntl.h>
> +#include<linux/fs.h>

Can probably clean these up a bit, I believe fcntl.h/errno.h/stat.h are 
already available at least.

>   static VAServerData *va_server_data;
>   static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
> @@ -217,6 +224,189 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
>       return result;
>   }
>
> +
> +/*
> + * Walk the mount table and build a list of local file systems
> + */
> +
> +struct direntry {
> +    char *dirname;
> +    char *devtype;
> +    struct direntry *next;
> +};
> +
> +static struct direntry *mount_list;
> +static int fsfreeze_status;
> +
> +static int build_mount_list(void)
> +{
> +    struct mntent *mnt;
> +    struct direntry *entry;
> +    struct direntry *next;
> +    char const *mtab = MOUNTED;
> +    FILE *fp;
> +
> +    fp = setmntent(mtab, "r");
> +    if (!fp) {
> +       fprintf(stderr, "unable to read mtab\n");
> +       goto fail;
> +    }
> +
> +    while ((mnt = getmntent(fp))) {
> +       /*
> +        * An entry which device name doesn't start with a '/' is
> +        * either a dummy file system or a network file system.
> +        * Add special handling for smbfs and cifs as is done by
> +        * coreutils as well.
> +        */
> +       if ((mnt->mnt_fsname[0] != '/') ||
> +           (strcmp(mnt->mnt_type, "smbfs") == 0) ||
> +           (strcmp(mnt->mnt_type, "cifs") == 0)) {
> +           continue;
> +       }
> +
> +       entry = qemu_malloc(sizeof(struct direntry));
> +       if (!entry) {
> +           goto fail;
> +       }
> +       entry->dirname = qemu_strdup(mnt->mnt_dir);
> +       entry->devtype = qemu_strdup(mnt->mnt_type);
> +       entry->next = mount_list;
> +
> +       mount_list = entry;
> +    }
> +
> +    endmntent(fp);
> +
> +    return 0;
> +
> +fail:
> +    while(mount_list) {
> +       next = mount_list->next;
> +       qemu_free(mount_list->dirname);
> +       qemu_free(mount_list->devtype);
> +       qemu_free(mount_list);
> +       mount_list = next;
> +    }

should be spaces instead of tabs

> +
> +    return -1;
> +}
> +
> +/*
> + * va_fsfreeze(): Walk list of mounted file systems in the guest, and
> + *   freeze the ones which are real local file systems.
> + * rpc return values: Number of file systems frozen, -1 on error.
> + */
> +static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_int32 ret = 0, i = 0;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd;
> +    SLOG("va_fsfreeze()");
> +
> +    if (fsfreeze_status == FREEZE_FROZEN) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    ret = build_mount_list();
> +    if (ret<  0) {
> +        goto out;
> +    }
> +
> +    fsfreeze_status = FREEZE_INPROGRESS;
> +
> +    entry = mount_list;

I think as we start adding more and more stateful RPCs, free-floating 
state variables can start getting a bit hairy to keep track of. 
Eventually I'd like to have state information that only applies to a 
subset of RPCs consolidated into a single object. I wouldn't focus on 
this too much because I'd like to have an interface to do this in the 
future (mainly so they can state objects can register themselves and 
provide a reset() function that can be called when, for instance, an 
agent disconnects/reconnects), but in the meantime I think it would be 
more readable to have a global va_fsfreeze_state object to track freeze 
status and mount points.

> +    while(entry) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = errno;
> +            goto error;
> +        }
> +        ret = ioctl(fd, FIFREEZE);
> +        if (ret<  0&&  ret != EOPNOTSUPP) {
> +            goto error;
> +        }
> +
> +        close(fd);
> +        entry = entry->next;
> +        i++;
> +    }
> +
> +    fsfreeze_status = FREEZE_FROZEN;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +error:
> +    if (i>  0) {
> +        fsfreeze_status = FREEZE_ERROR;
> +    }
> +    goto out;
> +}
> +
> +/*
> + * va_fsthaw(): Walk list of frozen file systems in the guest, and
> + *   thaw them.
> + * rpc return values: Number of file systems thawed on success, -1 on error.
> + */
> +static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
> +                               xmlrpc_value *params,
> +                               void *user_data)
> +{
> +    xmlrpc_int32 ret;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd, i = 0;
> +    SLOG("va_fsthaw()");
> +
> +    if (fsfreeze_status == FREEZE_THAWED) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    while((entry = mount_list)) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = -1;
> +            goto out;
> +        }
> +        ret = ioctl(fd, FITHAW);
> +        if (ret<  0&&  ret != EOPNOTSUPP) {
> +            ret = -1;
> +            goto out;
> +       }

whitespace issues

> +        close(fd);
> +
> +        mount_list = entry->next;
> +        qemu_free(entry->dirname);
> +        qemu_free(entry->devtype);
> +        qemu_free(entry);
> +        i++;
> +    }
> +
> +    fsfreeze_status = FREEZE_THAWED;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +}
> +
> +/* va_fsstatus(): Return status of freeze/thaw
> + * rpc return values: fsfreeze_status
> + */
> +static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_value *result = xmlrpc_build_value(env, "i", fsfreeze_status);
> +    SLOG("va_fsstatus()");
> +    return result;
> +}

Hmm, you mentioned before that these freezes may be long-running 
jobs...do the ioctl()'s not return until completion? There is global 
timeout in virtagent, currently under a minute, to prevent a virtagent 
monitor command from hanging the monitor session, so if it's unlikely 
you'll fit in this window we'll need to work on something to better 
support these this kinds of situations.

The 3 main approaches would be:

1) allow command-specific timeouts with values that are sane for the 
command in question, and potentially allow timeouts to be disabled
2) fork() long running jobs and provide a mechanism for them to provide 
asynchronous updates to us to we can query status
3) fork() long running jobs, have them provide status information 
elsewhere, and provide a polling function to check that status

3) would likely require something like writing status to a file and then 
provide a polling function to check it, which doesn't work here so 
that's probably out.

I'd initially planned on doing 2) at some point, but I'm beginning to 
think 1) is the better approach, since qemu "opts in" on how long it's 
willing to hang for a particular command, so there's not really any 
surprises. At least not to qemu...users might get worried after a while, 
so there is a bit of a trade-off. But it's also more user-friendly....no 
need for polling or dealing with asynchronous updates to figure out when 
an RPC has actually finished. Seem reasonable?

> +
>   typedef struct RPCFunction {
>       xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
>       const char *func_name;
> @@ -237,6 +427,12 @@ static RPCFunction guest_functions[] = {
>         .func_name = "va.ping" },
>       { .func = va_capabilities,
>         .func_name = "va.capabilities" },
> +    { .func = va_fsfreeze,
> +      .func_name = "va.fsfreeze" },
> +    { .func = va_fsthaw,
> +      .func_name = "va.fsthaw" },
> +    { .func = va_fsstatus,
> +      .func_name = "va.fsstatus" },
>       { NULL, NULL }
>   };
>   static RPCFunction host_functions[] = {