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

[Paolo Bonzini]

Il 01/08/2012 16:59, Kevin Wolf ha scritto:
> 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.

Yes, but then it makes sense (at least initially) to just block things
more than needed.  In this respect, in_use is doing its job decently;
there is always time to relax things later.

> 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?

What blocks what, for example (for which as you said we need a matrix in
order to model it correctly).  Or how the iostatus is reset if the
target has a iostatus at all; if not, that would be another difference.

>>>>>> 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."

Cool things you can do;
don't ask for two at a time
or QEMU eats your disk.

> 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.

So far we fared pretty well on the latter point though.

>> 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. :-)

It is, but it has the merit of decoupling new stuff from "not so
brilliant" old stuff.

> 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.

Or the iostatus for the target can just reside in the BlockJob... :)

As much as I hate to invoke shortcuts, management may proceed without
human help only in the ENOSPC case, and ENOSPC can only happens on the
target.  Humans usually look at dmesg to find the source.

Paolo