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 <tomoki.sekiyama...@hitachi.com>
>> ---
<snip>
>> 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)
<snip>
>> /*
>> * 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 <tomoki.sekiyama...@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
