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