From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34854 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkHC8-0002md-IF for qemu-devel@nongnu.org; Tue, 01 Feb 2011 09:27:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PkHC7-0006AC-JT for qemu-devel@nongnu.org; Tue, 01 Feb 2011 09:27:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:11581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PkHC7-0006A7-CN for qemu-devel@nongnu.org; Tue, 01 Feb 2011 09:26:59 -0500 Message-ID: <4D48182E.9030201@redhat.com> Date: Tue, 01 Feb 2011 15:26:54 +0100 From: Jes Sorensen MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw References: <1296557928-30019-1-git-send-email-Jes.Sorensen@redhat.com> <1296557928-30019-2-git-send-email-Jes.Sorensen@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: agl@us.ibm.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com On 02/01/11 15:12, Stefan Hajnoczi wrote: > On Tue, Feb 1, 2011 at 10:58 AM, 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. > > It is desirable to minimize the freeze time, which may interrupt or > degrade the service that applications inside the VM can provide. > Polling means we have to choose a fixed value (500 ms?) at which to > check for freeze completion. In this example we could have up to 500 > ms extra time spent in freeze because it completed right after we > polled. Any thoughts on this? I have to admit you lost me here, where do you get that 500ms time from? Is that the XMLRPC polling time or? I just used the example code from other agent calls. > In terms of the fsfreeze(), fsthaw(), fsstatus() API, are you looking > at Windows Volume Shadow Copy Services and does this API fit that > model (I haven't looked at it in detail yet)? > http://msdn.microsoft.com/en-us/library/bb968832(v=vs.85).aspx I haven't looked at it, I designed the calls based on how they fit with the Linux ioctls. >> + entry = qemu_malloc(sizeof(struct direntry)); >> + if (!entry) { >> + goto fail; >> + } > > qemu_malloc() never fails. Good point, we have ugly malloc in qemu :( I wrote the code to handle this outside QEMU first, to make sure it worked correctly and trying to see how many times I could crash my laptop in the process. I'll fix it. >> +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; >> + } > > The only valid status here is FREEZE_THAWED? Perhaps we should test > for that specifically. Good point, I'll fix this. >> + >> + ret = build_mount_list(); >> + if (ret < 0) { >> + goto out; >> + } >> + >> + fsfreeze_status = FREEZE_INPROGRESS; >> + >> + entry = mount_list; >> + while(entry) { >> + fd = qemu_open(entry->dirname, O_RDONLY); >> + if (fd == -1) { >> + ret = errno; >> + goto error; >> + } >> + ret = ioctl(fd, FIFREEZE); > > If you close(fd) here then it won't leak or need extra code in the error path. Good point, will fix. >> +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; >> + } > > A stricter check would be status FREEZE_FROZEN. Yep, will fix >> + >> + while((entry = mount_list)) { >> + fd = qemu_open(entry->dirname, O_RDONLY); >> + if (fd == -1) { >> + ret = -1; >> + goto out; >> + } >> + ret = ioctl(fd, FITHAW); > > Same thing about close(fd) here. Thanks for the review, all valid points! Cheers, Jes