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

[Eric Blake]

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 <address@hidden>
> ---
>  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.


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

>      BDRVQuorumState *s = bs->opaque;
> -    QuorumAIOCB *acb = quorum_aio_get(bs, qiov, sector_num, nb_sectors);
> +    QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes);
>      int ret;
>  
>      acb->is_read = true;
> @@ -726,9 +722,7 @@ static void write_quorum_entry(void *opaque)
>      QuorumChildRequest *sacb = &acb->qcrs[i];
>  
>      sacb->bs = s->children[i]->bs;
> -    sacb->ret = bdrv_co_pwritev(s->children[i],
> -                                acb->sector_num * BDRV_SECTOR_SIZE,
> -                                acb->nb_sectors * BDRV_SECTOR_SIZE,
> +    sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
>                                  acb->qiov, 0);

A followup patch that supports non-zero flags such as BDRV_REQUEST_FUA
might be nice (presumably, we would advertise supported_write_flags of
of BDRV_REQUEST_FUA only if all children support it).  I agree that this
patch is not the place to do it, though, so hard-coding to 0 may be
okay, if...

>      if (sacb->ret == 0) {
>          acb->success_count++;
> @@ -745,12 +739,11 @@ static void write_quorum_entry(void *opaque)
>      }
>  }
>  
> -static int quorum_co_writev(BlockDriverState *bs,
> -                            int64_t sector_num, int nb_sectors,
> -                            QEMUIOVector *qiov)
> +static int quorum_co_pwritev(BlockDriverState *bs, uint64_t offset,
> +                             uint64_t bytes, QEMUIOVector *qiov, int flags)
>  {

...you make a decision on whether asserting that flags is zero is best
for now.

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

Due to my question about rounding in the event code, I'm omitting R-b
for now.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature