Am 21.11.2016 um 21:04 hat Eric Blake geschrieben:
> On 11/21/2016 11:31 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 therefore.
> >
> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
> > ---
> > block/quorum.c | 79
> > ++++++++++++++++++++++++++--------------------------------
> > 1 file changed, 36 insertions(+), 43 deletions(-)
> >
>
> > -static void quorum_report_bad(QuorumOpType type, uint64_t sector_num,
> > - int nb_sectors, char *node_name, int ret)
> > +static void quorum_report_bad(QuorumOpType type, uint64_t offset,
> > + uint64_t bytes, char *node_name, int ret)
> > {
> > const char *msg = NULL;
> > + int64_t start_sector = offset / BDRV_SECTOR_SIZE;
> > + int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
>
> This one looks correct,
>
> > +
> > if (ret < 0) {
> > msg = strerror(-ret);
> > }
> >
> > - qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name,
> > - sector_num, nb_sectors,
> > &error_abort);
> > + qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name,
> > start_sector,
> > + end_sector - start_sector,
> > &error_abort);
> > }
> >
> > static void quorum_report_failure(QuorumAIOCB *acb)
> > {
> > const char *reference = bdrv_get_device_or_node_name(acb->bs);
> > - qapi_event_send_quorum_failure(reference, acb->sector_num,
> > - acb->nb_sectors, &error_abort);
> > + qapi_event_send_quorum_failure(reference,
> > + acb->offset / BDRV_SECTOR_SIZE,
> > + acb->bytes / BDRV_SECTOR_SIZE,
>
> but this one still looks like it could give unexpected results for
> acb->bytes < BDRV_SECTOR_SIZE.
Thanks, I missed this one. I'll send a v2.
> > -static int quorum_co_readv(BlockDriverState *bs,
> > - int64_t sector_num, int nb_sectors,
> > - QEMUIOVector *qiov)
> > +static int quorum_co_preadv(BlockDriverState *bs, uint64_t offset,
> > + uint64_t bytes, QEMUIOVector *qiov, int flags)
> > {
>
> Is it worth adding assert(!flags)? For now, the block layer doesn't
> have any defined flags (and if it did, we'd probably want to add a
> .supported_read_flags to parallel the existing .supported_write_flags).
I don't think we need to assert this, no other driver does that. We have
.supported_write_flags and I would indeed add .supported_read_flags
if/when we start using flags for read requests, so we can be reasonably
sure that only those flags are set even without asserting it.
> [Huh - side thought: right now, we don't have any defined semantics for
> BDRV_REQUEST_FUA on reads (although we modeled it in part after SCSI,
> which does have it defined for reads). But quorum rewrites on read
> might be an interesting application of where we can trigger a write
> during reads, and where we may want to guarantee FUA semantics on those
> writes, thus making a potentially plausible use of the flag on read]
Makes sense to me, but that's something for a different series. And
actually, I'm not sure who would even send a read with FUA set today.
Can this even happen yet?
Kevin
pgpBRkIhRIAaX.pgp
Description: PGP signature
