From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37445 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkJRC-0008AJ-94 for qemu-devel@nongnu.org; Tue, 01 Feb 2011 11:50:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PkJQy-0006Ss-Qf for qemu-devel@nongnu.org; Tue, 01 Feb 2011 11:50:30 -0500 Received: from e6.ny.us.ibm.com ([32.97.182.146]:57658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PkJQy-0006Rl-M3 for qemu-devel@nongnu.org; Tue, 01 Feb 2011 11:50:28 -0500 Received: from d01dlp02.pok.ibm.com (d01dlp02.pok.ibm.com [9.56.224.85]) by e6.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p11Ga4tP003552 for ; Tue, 1 Feb 2011 11:36:09 -0500 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 65C9D4DE803F for ; Tue, 1 Feb 2011 11:49:51 -0500 (EST) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p11GoOYr154610 for ; Tue, 1 Feb 2011 11:50:24 -0500 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p11GoNrs010520 for ; Tue, 1 Feb 2011 09:50:23 -0700 Message-ID: <4D4839CC.8000109@linux.vnet.ibm.com> Date: Tue, 01 Feb 2011 10:50:20 -0600 From: Michael Roth MIME-Version: 1.0 References: <1296557928-30019-1-git-send-email-Jes.Sorensen@redhat.com> <1296557928-30019-2-git-send-email-Jes.Sorensen@redhat.com> In-Reply-To: <1296557928-30019-2-git-send-email-Jes.Sorensen@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes.Sorensen@redhat.com Cc: qemu-devel@nongnu.org, agl@us.ibm.com On 02/01/2011 04:58 AM, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen > > 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 > --- > 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 > #include "qemu_socket.h" > #include "virtagent-common.h" > +#include > +#include > +#include > +#include > +#include > +#include > +#include 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[] = {