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