Re: [Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw

[Jes Sorensen]

On 02/01/11 15:12, Stefan Hajnoczi wrote:
> On Tue, Feb 1, 2011 at 10:58 AM,  <address@hidden> wrote:
>> From: Jes Sorensen <address@hidden>
>>
>> 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