From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb6QV-0003Lf-Q9 for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:17:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tb6QU-0005mR-8t for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:16:59 -0500 Received: from mail4.hitachi.co.jp ([133.145.228.5]:37352) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb6QT-0005m4-PK for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:16:58 -0500 Message-ID: <50AC9C2E.8080706@hitachi.com> Date: Wed, 21 Nov 2012 18:17:34 +0900 From: Tomoki Sekiyama MIME-Version: 1.0 References: <20121113045645.2645.43591.stgit@melchior2.sdl.hitachi.co.jp> <20121113045656.2645.88398.stgit@melchior2.sdl.hitachi.co.jp> <20121120210005.GC14974@vm> In-Reply-To: <20121120210005.GC14974@vm> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mdroth@linux.vnet.ibm.com Cc: qemu-devel@nongnu.org Hi Michael, Thanks for your review. On 2012/11/21 6:00, mdroth wrote: > On Tue, Nov 13, 2012 at 01:56:56PM +0900, Tomoki Sekiyama wrote: >> To use the online disk snapshot for online-backup, application-level >> consistency of the snapshot image is required. However, currently the >> guest agent can provide only filesystem-level consistency, and the >> snapshot may contain dirty data, for example, incomplete transactions. >> This patch provides the opportunity to quiesce applications before >> snapshot is taken. >> >> When the qemu-ga receives fsfreeze-freeze command, the hook script >> specified in --fsfreeze-hook option is executed with "freeze" argument >> before the filesystem is frozen. For fsfreeze-thaw command, the hook >> script is executed with "thaw" argument after the filesystem is thawed. >> >> Signed-off-by: Tomoki Sekiyama >> --- >> struct GAState *ga_state; >> @@ -153,6 +165,10 @@ static void usage(const char *cmd) >> " %s)\n" >> " -l, --logfile set logfile path, logs to stderr by default\n" >> " -f, --pidfile specify pidfile (default is %s)\n" >> +#ifdef CONFIG_FSFREEZE >> +" -F, --fsfreeze_hook\n" > > Small nit, but can we change this to --fsfreeze-hook? Ah, sure. >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c >> index 726930a..8c3e341 100644 >> --- a/qga/commands-posix.c >> +++ b/qga/commands-posix.c >> @@ -396,6 +396,45 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) >> return GUEST_FSFREEZE_STATUS_THAWED; >> } >> > > Rather than passing freeze/thaw in as a string, I think an enum > parameter of the form: > > typedef enum { > FSFREEZE_HOOK_THAW, > FSFREEZE_HOOK_FREEZE, > } FsfreezeHook; > > would be better in terms of documenting the interface. Okey, I will use enum as the argument. >> +static void execute_fsfreeze_hook(const char *arg) >> /* >> * Walk list of mounted file systems in the guest, and freeze the ones which >> * are real local file systems. >> @@ -410,6 +449,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err) >> >> slog("guest-fsfreeze called"); >> >> + execute_fsfreeze_hook("freeze"); >> + > > I think if a user configures a pre-freeze hook, it's failure should be > treated as a failure of the fsfreeze call in general, otherwise we risk > moving forward based on false assumptions that can lead to data loss or > other issues. I think the thaw hook is okay the way it is though, they > can review the logs for any issues arising after the VM is > unfrozen. I think that is reasonable. It would be better to notify the failure of fsfreeze hook to the host side for investigation of the issue. I will fix these in the next patchset. Thanks, -- Tomoki Sekiyama Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory