Re: [Qemu-devel] [PATCH v2 06/45] block: add support for job pause/resume

[Kevin Wolf]

Am 27.09.2012 14:27, schrieb Paolo Bonzini:
> Il 27/09/2012 14:18, Kevin Wolf ha scritto:
>>>>
>>>> Signed-off-by: Paolo Bonzini <address@hidden>
>> I think there's a problem with terminology at least. What does "paused"
>> really mean? Is it that the job has been requested to pause, or that it
>> has actually yielded and is inactive?
>>
>> The commit message seems to use the latter semantics (which I would
>> consider the intuitive one),
> 
> You mean this: "Paused jobs cannot be canceled without first resuming
> them".  I can add a specification, like "(even if the job actually has
> not reached the sleeping point and thus is still running)".

I actually meant "pause happens at the next sleeping point", which isn't
unspecific at all.

>> the QMP documentation leaves it unclear,
>> but the code actually implements the former semantics.
> 
> This code comment is clear:
> 
>     /**
>      * Set to true if the job is either paused, or will pause itself
>      * as soon as possible (if busy == true).
>      */
>     bool paused;

Yes, this one is a good and clear comment (and possibly I wouldn't even
have noticed without this comment)

> but this one can indeed use some improvement.
> 
> /**
>  * block_job_is_paused:
>  * @job: The job being queried.
>  *
>  * Returns whether the job is currently paused.
>  */
> bool block_job_is_paused(BlockJob *job);
> 
> 
> From the QMP client's point of view it doesn't really matter, does it?
> 
> - even after a job that writes to disk X has "really" paused, you cannot
> read or write disk X.  It's still owned by QEMU, it hasn't been flushed,
> it may play games like lazy refcounts.

I'm not sure about this one. Consider things like a built-in NBD server.
Probably we'll find more cases in the future, where some monitor command
might seem to be safe while a job is paused.

It makes me nervous that clients could make assumptions based on the
paused state despite having no way to make sure that a job is actually
stopped - the documentation doesn't even tell them about the fact that
"paused" doesn't really mean what they think it means.

> - what matters is that a resume undoes a pause, even if it is still
> pending (which it does).

Agreed, this part looks okay.

Kevin