Re: [Qemu-devel] [PATCH 13/47] block: introduce block job error

[Kevin Wolf]

Am 01.08.2012 16:34, schrieb Paolo Bonzini:
> Il 01/08/2012 16:01, Kevin Wolf ha scritto:
>> Well, the thing that disturbs me is having a BlockJob attached to a
>> single block device. Jobs can and do operate on multiple images and
>> should probably affect both BDSs the same. For example, bs->in_use
>> should probably be set for both (Haven't checked in detail yet, do we
>> have bugs here? Patch 27 doesn't call bdrv_set_in_use at least. It
>> starts to matter when we get named targets.)
> 
> Makes sense, but what does bs->in_use mean if being a target is "just
> another way" to use a named block device?  A target cannot be attached
> to a guest disk

For the mirror, probably yes. Live commit writes to a BDS that is part
of a BlockBackend used by a guest. So it depends on the exact job type
which involved images can be used in which other cases.

Maybe we should first get an overview of which uses exclude which other
uses of a BDS. Because honestly, at this point I have no idea how to
model it correctly.

> but a source that is used by the guest can become part
> of a block job.  So, you could block in_use devices from being used in
> the drive property.  But you need a separate mechanism to mark devices
> as used by the guest... in_use does not cut it.
> 
> Stuff like this is what makes me nervous about naming the target
> devices.  Name something, and people start using it in ways you haven't
> anticipated.  It also makes me wonder if the target is indeed a
> BlockBackend or there is a common superclass hiding.

Hm...Don't know, what would the difference be?

>>>>>>> +    if (action == BDRV_ACTION_STOP) {
>>>>>>> +        block_job_pause(job);
>>>>>>> +        block_job_iostatus_set_err(job, error);
>>>>>>> +        if (bs != job->bs) {
>>>>>>> +            bdrv_iostatus_set_err(bs, error);
>>>>>>> +        }
>>>>>>> +    }
>>>>>>>
>>>>>>> where the bdrv_iostatus_set_err is mostly to "prepare for the future"
>>>>>>> usage of named block devices.
>>>>>>
>>>>>> Again, I'd make it unconditional to get symmetric behaviour.
>>>>>
>>>>> Not possible, because existing clients may expect "iostatus:
>>>>> {nospace,failed}" to imply a runstate of "not running (i/o error)".
>>>>
>>>> Did we make such guarantees? Does libvirt actually make use of it?
>>>
>>> I'm not sure libvirt relies on it, but I think it's a reasonable
>>> expectation.
>>
>> Possibly, but I don't think anyone should rely on it. You first get the
>> event, or notice that the VM is stopped, and then check query-block.
>> Doing it the other way round and then inferring the VM state from
>> query-block sounds highly unlikely.
> 
> Yeah, makes sense.  The other thing is that the iostatus is very much
> tied to the running state.  A stop+cont happening for unrelated reasons
> would clear the iostatus for example.

Yes, clearing it automatically may be convenient in the common case, but
is probably not exactly the best idea we've ever had.

>>>>>>> Hmm, but I'm a bit wary of introducing such a big change now.  We know
>>>>>>> what it makes nicer, but we don't know of anything irremediably broken
>>>>>>> without them, and we haven't thought enough of any warts it introduces.
>>>>>>
>>>>>> On the one hand, I can understand your concerns, but on the other hand,
>>>>>> introducing an API now and then making such a big change afterwards
>>>>>> scares me much more.
>>>>>
>>>>> One example of the doubts I have: is iostatus a BlockBackend or a
>>>>> BlockDriverState thing?  If it a BlockBackend thing, does the target
>>>>> device have an iostatus at all?
>>>>
>>>> I think it's better to have it in BlockDriverState, but in my
>>>> imagination the target is a BlockBackend anyway.
>>>
>>> Great, 0/2. :)  My rhetorical questions didn't have the outcome I hoped for.
>>
>> Heh. :-)
>>
>> I was thinking that when you do it for backing files, you automatically
>> have to use the BDS, but the iostatus/pointer model works as well, so
>> yeah, maybe BB is better.
>>
>> But why wouldn't you use a BlockBackend for the target?
> 
> Probably just because at the moment I'm not sure how much of BDS would
> percolate there.  In_use is tricky, of course, but perhaps we can or
> need to get rid of in_use altogether.

in_use has never been properly designed, it is a band-aid hack that has
spread more or more. Can you explain its exact semantics? The best I can
come up with is something like: "Someone does something with the device
and thought that doing one specific other thing, that happens to use
in_use as well, at the same time might be a bad idea."

I'm pretty sure that in_use forbids more cases than is really necessary,
and probably many other cases that should be forbidden are missing.

> I'm not sure of iostatus because we currently do not have a way to reset
> it.  "cont" and "block-job-resume" do that, but only implicitly and
> that's too tied to the current usage of iostatus.  If the target device
> is not a BB we solve the problem of "cont" resetting its iostatus...

This is an awful reason. :-)

Block jobs aren't really different from guests in that respect. Maybe
the BB needs a second iostatus field that must explicitly be reset, and
the old one keeps doing the stupid thing for compatibility's sake.

Kevin