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

[Kevin Wolf]

Am 01.08.2012 14:30, schrieb Paolo Bonzini:
> Il 01/08/2012 14:23, Kevin Wolf ha scritto:
>> Am 01.08.2012 14:09, schrieb Paolo Bonzini:
>>> Il 01/08/2012 13:49, Kevin Wolf ha scritto:
>>>>> The real question is: if I remove the possibility to inspect the (so far
>>>>> anonymous) target device with query-block-jobs, how do you read the
>>>>> status of the target device?...
>>>>
>>>> You don't? :-)
>>>
>>> That's a possibility. :)
>>>
>>> You can just report it in the block job.  It's more consistent with the
>>> fact that you got a BLOCK_JOB_ERROR and not a BLOCK_IO_ERROR.  So I
>>> would do:
>>>
>>> +    bdrv_emit_qmp_error_event(job->bs, QEVENT_BLOCK_JOB_ERROR,
>>> +                              action, is_read);
>>
>> Isn't job->bs the source?
> 
> It's not about source vs. target, it's about what block device the _job_
> is attached too.  The source is always going to be special because you
> must do "block-job-resume source".

This whole concept of a block job being attached to a single bs is
flawed. Because in the general case it isn't. And probably it should
have been a BackgroundJob rather than a BlockJob from the beginning,
because I'm sure there are use cases outside the block layer as well.

We really manage to get every single point messed up. :-(

> is_read tells you where the error was.

Rather indirect, but yes, for the mirror it works.

>> Also, if you miss the event (e.g. libvirt crashed), can you still tell
>> which bs caused the error?
> 
> No, but how is it different from "can you still tell which bs in the
> chain caused the error"?  Which you cannot tell anyway even from the
> event parameters we have had so far.

It isn't different. We really should report the exact BDS. In practice,
management tools care about ENOSPC which is always the top-level BDS,
and call the admin for help in other cases. The admin can try manually
which file is at fault, but I suppose he would be very glad to be told
by the error message which file it is.

>> Do we need another BlockJobInfo field for the
>> name (*cough* ;-)) of the failed bs?
>>
>> If I understand it right, error reporting on the source and on the
>> target would be completely symmetrical then, which I think is a good thing.
> 
> Yeah, it would, but job->bs _is_ special anyway.
> 
>> job->bs makes one image special, which isn't really useful for a generic
>> interface. So we should keep the fact that it exists internal and get
>> rid of it sometime.
>>
>>> +    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?

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

Kevin