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

[Eric Blake]

On 11/10/2016 11:19 AM, Kevin Wolf wrote:
> This enables byte granularity requests on quorum nodes.
> 
> 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.

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

> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/quorum.c | 75 
> ++++++++++++++++++++++++++--------------------------------
>  1 file changed, 33 insertions(+), 42 deletions(-)
> 

> @@ -198,14 +198,17 @@ static void quorum_report_bad(QuorumOpType type, 
> uint64_t sector_num,
>      }
>  
>      qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name,
> -                                      sector_num, nb_sectors, &error_abort);
> +                                      offset / BDRV_SECTOR_SIZE,
> +                                      bytes / BDRV_SECTOR_SIZE, 
> &error_abort);

Rounding the offset down makes sense, but rounding the bytes down can
lead to weird messages.  Blindly rounding it up to a sector boundary can
also be wrong (example: writing 2 bytes at offset 511 really affects
1024 bytes when you consider that two sectors had to undergo
read-modify-write). Don't we have a helper routine for determining the
end boundary when we have to convert an offset and length to a courser
alignment?


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

Overall, I like that you are getting rid of more sector-based cruft.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature