Re: [Qemu-devel] [PATCH 06/16] quorum: allow ignoring child errors

[Wen Congyang]

On 09/03/2015 12:30 AM, Eric Blake wrote:
> On 09/02/2015 02:51 AM, Wen Congyang wrote:
>> If the child is not ready, read/write/getlength/flush will
>> return -errno. It is not critical error, and can be ignored:
>> 1. read/write:
>>    Just not report the error event.
> 
> What happens if all the children report an error?  Or is the threshold
> at play here?

Good question. What about this: if we have 5 children, only 4 children
can ignore errors.

> 
> For example, if you have a threshold of 3/5, then I'm assuming that if
> up to two children return an errno, then it is okay to ignore; but if
> three or more return an errno, you haven't met threshold, so the I/O
> must fail.

Do more check here, at least one child sucesses.

> 
> Are you ignoring ALL errors (including things like EACCES), or just EIO
> errors?

Does bdrv_xxx() always return -errno on error?

> 
> 
>> 2. getlength:
>>    just ignore it. If all children's getlength return -errno,
>>    and be ignored, return -EIO.
>> 3. flush:
>>    Just ignore it. If all children's getlength return -errno,
> 
> s/getlength/flush/
> 
>>    and be ignored, return 0.
> 
> Yuck - claiming success when all of the children fail feels dangerous.

Yes.

> 
>>
>> Usage: children.x.ignore-errors=true
>>
>> Signed-off-by: Wen Congyang <address@hidden>
>> Signed-off-by: zhanghailiang <address@hidden>
>> Signed-off-by: Gonglei <address@hidden>
>> Cc: Alberto Garcia <address@hidden>
>> ---
>>  block/quorum.c       | 94 
>> ++++++++++++++++++++++++++++++++++++++++++++++++----
> 
> Interface review only:
> 
>> +++ b/qapi/block-core.json
>> @@ -1411,6 +1411,8 @@
>>  # @allow-write-backing-file: #optional whether the backing file is opened in
>>  #                            read-write mode. It is only for backing file
>>  #                            (Since 2.5 default: false)
>> +# @ignore-errors: #options whether the child's I/O error should be ignored.
> 
> s/options/optional/
> s/error/errors/
> 
>> +#                 it is only for quorum's child.(Since 2.5 default: false)
> 
> Space after '.' in English sentences.
> 
> The fact that you are documenting that this option can only be specified
> for quorum children makes me wonder if it belongs better as an option in
> BlockdevOptionsQuorum rather than BlockdevOptionsBase.

Hmm, how to define it? It is quorum's child's option, not quorum's option.

> 
> Semantically, it sounds like you are trying to allow for a per-child
> decision of whether this particular child's errors matter to the overall
> quorum.  So, if we have a 3/5 quorum, we can decide that for children A,
> B, C, and D, errors cannot be ignored, but for child E, errors are not a
> problem.
> 
> As written, you are tying the semantics to each child BDS, and requiring
> special code to double-check that the property is only ever set if the
> BDS is used as the child of a quorum.  Furthermore, if the property is
> set, you are then changing what the child does in response to various
> operations.
> 
> What if you instead create a list property in the quorum parent?  Maybe
> along the lines of:
> 
> # @child-errors-okay: #optional an array of named-node children where
> errors will be ignored (Since 2.5, default empty)
> 
> { 'struct': 'BlockdevOptionsQuorum',
>   'data': { '*blkverify': 'bool',
>             'children': [ 'BlockdevRef' ],
>             'vote-threshold': 'int',
>             '*rewrite-corrupted': 'bool',
>             '*read-pattern': 'QuorumReadPattern',
>             '*child-errors-okay': ['str'] } }
> 
> The above example of a 3/5 quorum, where only child E can ignore errors,
> would then be represented as:
> 
> { "children": [ "A", "B", "C", "D", "E" ], 'vote-threshold':3,
> 'child-errors-okay': [ "E" ] }

OK, I will try it.

> 
> The code to ignore the errors is then done in the quorum itself (the BDS
> for E does not have to care about a special ignore-errors property, but
> just always returns the error as usual; and then the quorum is deciding
> how to handle the error), and you are not polluting the BDS state for
> something that is quorum-specific, because it is now the quorum itself
> that tracks the special casing.
> 
> Finally, why can't hot-plug/unplug of quorum members work?  If you are
> going to always ignore errors from a particular child, then why is that
> child even part of the quorum?  Isn't a better design to just not add
> the child to the quorum until it is ready and won't be reporting errors?
> 

Yes. In the early version, I don't use hot-plug/unplug of quorum members, so
the quorum member may be not ready. Now I use hot-plug/unplug of quorum members,
so the quorum's member is ready when it is hot added.  In such case, the quorum
member is not ready after failover. This error is expected, but I want this 
error
can be ignored, otherwise, there may be too error events...

Thanks
Wen Congyang