[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 7/8] quorum: Implement .bdrv_co_preadv/pwrit
Re: [Qemu-devel] [RFC PATCH 7/8] quorum: Implement .bdrv_co_preadv/pwritev()
Fri, 11 Nov 2016 10:58:57 +0100
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
If there is a user for the new fields, I can do that.
> > 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
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().
Description: PGP signature
Re: [Qemu-devel] [RFC PATCH 0/8] quorum: Implement .bdrv_co_preadv/pwritev(), Paolo Bonzini, 2016/11/11