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

[Kevin Wolf <kwolf@redhat.com>]

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

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

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