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

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 02/01/11 17:50, Michael Roth wrote:
> On 02/01/2011 04:58 AM, Jes.Sorensen@redhat.com wrote:
>> +enum vs_fsfreeze_status {
>> +    FREEZE_ERROR = -1,
>> +    FREEZE_THAWED = 0,
>> +    FREEZE_INPROGRESS = 1,
>> +    FREEZE_FROZEN = 2,
>> +    FREEZE_THAWINPROGRESS = 3,
>> +};
> 
> Any reason for vs_* vs. va_*?

Hmmmm let me see if I can find a good excuse for that typo :)

>> diff --git a/virtagent-server.c b/virtagent-server.c
>> index 7bb35b2..cf2a3f0 100644
>> --- a/virtagent-server.c
>> +++ b/virtagent-server.c
>> @@ -14,6 +14,13 @@
>>   #include<syslog.h>
>>   #include "qemu_socket.h"
>>   #include "virtagent-common.h"
>> +#include<mntent.h>
>> +#include<sys/types.h>
>> +#include<sys/stat.h>
>> +#include<sys/errno.h>
>> +#include<sys/ioctl.h>
>> +#include<fcntl.h>
>> +#include<linux/fs.h>
> 
> Can probably clean these up a bit, I believe fcntl.h/errno.h/stat.h are
> already available at least.

Carry-over from writing the code outside of qemu. Would be much cleaner
than relying on the include everything and the kitchen sink in a global
header file, but thats how it is :(

>> +
>> +    fsfreeze_status = FREEZE_INPROGRESS;
>> +
>> +    entry = mount_list;
> 
> I think as we start adding more and more stateful RPCs, free-floating
> state variables can start getting a bit hairy to keep track of.
> Eventually I'd like to have state information that only applies to a
> subset of RPCs consolidated into a single object. I wouldn't focus on
> this too much because I'd like to have an interface to do this in the
> future (mainly so they can state objects can register themselves and
> provide a reset() function that can be called when, for instance, an
> agent disconnects/reconnects), but in the meantime I think it would be
> more readable to have a global va_fsfreeze_state object to track freeze
> status and mount points.

Urgh, what do you mean by object here? I have to admit the word object
always makes me cringe.... I changed the variables to have the va_ prefix.

>> +static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
>> +                                 xmlrpc_value *params,
>> +                                 void *user_data)
>> +{
>> +    xmlrpc_value *result = xmlrpc_build_value(env, "i",
>> fsfreeze_status);
>> +    SLOG("va_fsstatus()");
>> +    return result;
>> +}
> 
> Hmm, you mentioned before that these freezes may be long-running
> jobs...do the ioctl()'s not return until completion? There is global
> timeout in virtagent, currently under a minute, to prevent a virtagent
> monitor command from hanging the monitor session, so if it's unlikely
> you'll fit in this window we'll need to work on something to better
> support these this kinds of situations.

I think 1 minute is fine, but we should probably look at something a
little more flexible for handling commands over the longer term. Maybe
have virtagent spawn threads for executing some commands?

> The 3 main approaches would be:
> 
> 1) allow command-specific timeouts with values that are sane for the
> command in question, and potentially allow timeouts to be disabled
> 2) fork() long running jobs and provide a mechanism for them to provide
> asynchronous updates to us to we can query status
> 3) fork() long running jobs, have them provide status information
> elsewhere, and provide a polling function to check that status
> 
> 3) would likely require something like writing status to a file and then
> provide a polling function to check it, which doesn't work here so
> that's probably out.
> 
> I'd initially planned on doing 2) at some point, but I'm beginning to
> think 1) is the better approach, since qemu "opts in" on how long it's
> willing to hang for a particular command, so there's not really any
> surprises. At least not to qemu...users might get worried after a while,
> so there is a bit of a trade-off. But it's also more user-friendly....no
> need for polling or dealing with asynchronous updates to figure out when
> an RPC has actually finished. Seem reasonable?

I am not sure which is really the best solution. Basically we will need
to classify commands into two categories, so if you issue a certain type
of command, like agent_fsfreeze() (basically when the agent is in
FREEZE_FROZEN state) only status commands are allowed to execute in
parallel. Anything that tries to issue a write to the file system will
hang until agent_fsthaw is called. However it would be useful to be able
to call in for non-blocking status commands etc.

I'll post a v2 in a minute that addresses the issues pointed out by
Stefan and you. I think the threading/timeout aspect is something we
need to look at for the longer term.

Cheers,
Jes