Re: [Qemu-devel] [PATCH 1/6] block: Support byte-based aio callbacks

[Kevin Wolf <kwolf@redhat.com>]

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

Am 24.04.2018 um 19:06 hat Eric Blake geschrieben:
> On 04/24/2018 10:40 AM, Kevin Wolf wrote:
> > Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
> >> We are gradually moving away from sector-based interfaces, towards
> >> byte-based.  Add new sector-based aio callbacks for read and write,
> >> to match the fact that bdrv_aio_pdiscard is already byte-based.
> >>
> >> Ideally, drivers should be converted to use coroutine callbacks
> >> rather than aio; but that is not quite as trivial (if we do that
> >> conversion, the null-aio driver will disappear), so for the
> >> short term, converting the signature but keeping things with aio
> >> is easier.  Once all drivers are converted, the sector-based aio
> >> callbacks will be removed.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  include/block/block_int.h |  6 ++++++
> >>  block/io.c                | 37 +++++++++++++++++++++++++++----------
> >>  2 files changed, 33 insertions(+), 10 deletions(-)
> >>
> 
> >> +++ b/block/io.c
> >> @@ -934,9 +934,11 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
> >>      sector_num = offset >> BDRV_SECTOR_BITS;
> >>      nb_sectors = bytes >> BDRV_SECTOR_BITS;
> >>
> >> -    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> >> -    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> >> -    assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS);
> >> +    if (!drv->bdrv_aio_preadv) {
> >> +        assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> >> +        assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> >> +        assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS);
> >> +    }
> > 
> > Hm, this is kind of ugly. Previously, we handled everything byte-aligned
> > in the first section, now we mix both in the second section.
> > 
> > I can see that you do this so you don't have to duplicate the acb and
> > coroutine yielding code below, but can we move things into the right
> > place in the final patch at least? That is, calculate sector_num and
> > nb_sectors only if all the byte-based interfaces weren't available.
> 
> Yeah, that's easy enough to squash into patch 6:
> 
> diff --git i/block/io.c w/block/io.c
> index ba767612931..49fabe8eeb1 100644
> --- i/block/io.c
> +++ w/block/io.c
> @@ -924,16 +924,13 @@ static int coroutine_fn
> bdrv_driver_preadv(BlockDriverState *bs,
>          return drv->bdrv_co_preadv(bs, offset, bytes, qiov, flags);
>      }
> 
> -    sector_num = offset >> BDRV_SECTOR_BITS;
> -    nb_sectors = bytes >> BDRV_SECTOR_BITS;
> -
> -    if (!drv->bdrv_aio_preadv) {
> -        assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -        assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> -        assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS);
> -    }
> -
>      if (drv->bdrv_co_readv) {
> +        sector_num = offset >> BDRV_SECTOR_BITS;
> +        nb_sectors = bytes >> BDRV_SECTOR_BITS;
> +
> +        assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> +        assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +        assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS);
>          return drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
>      } else {
>          BlockAIOCB *acb;

Ah, yes. I thought of moving the code in the else block, but this works,
too. Maybe it's even a bit nicer.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]