Re: [Qemu-block] [Qemu-devel] [RFC PATCH 7/8] quorum: Implement .bdrv_co_preadv/pwritev()

[Alberto Garcia]

On Fri 11 Nov 2016 10:58:57 AM CET, Kevin Wolf wrote:

>> > Note that the QMP events emitted by the driver are an external API
>> > that we were careless enough to define as sector based. The offset
>> > and length of requests reported in events are rounded down
>> > therefore.
>> 
>> Would it be better to round offset down and length up?  A length of 0
>> looks odd.
>
> To be honest, I don't know what these events are used for and what the
> most useful rounding would be. Maybe Berto can tell?

What makes sense if we don't want to break the API is to round as Eric
suggests. Or, more specifically:

sector-num = ROUND_SECTOR_DOWN(offset)
sectors-count = ROUND_SECTOR_UP(offset + length) - sector-num

>> Should we add more fields to the two affected events (QUORUM_FAILURE
>> and QUORUM_REPORT_BAD)? We have to keep the existing fields for
>> back-compat, but we could add new fields that give byte-based
>> locations for management apps smart enough to use the new instead of
>> the old (particularly since the old fields are named 'sector-num' and
>> 'sectors-count').
>
> If there is a user for the new fields, I can do that.

I wouldn't do it until we have a use case.

>> > @@ -462,8 +461,8 @@ static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB 
>> > *acb,
>> >      va_list ap;
>> >  
>> >      va_start(ap, fmt);
>> > -    fprintf(stderr, "quorum: sector_num=%" PRId64 " nb_sectors=%d ",
>> > -            acb->sector_num, acb->nb_sectors);
>> > +    fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 " ",
>> > +            acb->offset, acb->bytes);
>> 
>> Might be worth a separate patch to get rid of fprintf and use correct
>> error reporting.  But not the work for this patch.
>
> What would correct error reporting be in this case? A QMP event?
> Because other than that and stderr, I don't think we have any channels
> for error messages for I/O requests. We could use error_report(), but
> it would be effectively the same thing as fprintf().

I'm not sure what was the original use case of this, but the
functionality should be equivalente to the blkverify driver.

I think it should actually be possible to replace blkverify completely
with quorum:

https://lists.gnu.org/archive/html/qemu-devel/2012-08/msg02817.html

Berto