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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

Am 11.11.2016 um 03:37 hat Eric Blake geschrieben:
> 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.

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?

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

> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  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?

Hm, I would have to check the header files. I don't know one off the top
of my head. If you find it, let me know.

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

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]