Re: [Qemu-devel] [PATCH v3 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

[Tomoki Sekiyama]

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 <address@hidden>
>> ---
<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 <address@hidden>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory