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

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

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

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

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

Paolo