Re: [PATCH 1/3] block: eliminate BDRV_REQ_NO_SERIALISING

[Kevin Wolf <kwolf@redhat.com>]

[ Fixing the qemu-block address ]

Am 18.12.2019 um 14:17 hat Paolo Bonzini geschrieben:
> It is unused since commit 00e30f0 ("block/backup: use backup-top instead
> of write notifiers", 2019-10-01), drop it to simplify the code.
> 
> While at it, drop redundant assertions on flags.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/io.c            | 18 ++++--------------
>  include/block/block.h | 12 ------------
>  2 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index f75777f..b3a67fe 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1445,8 +1445,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>       * potential fallback support, if we ever implement any read flags
>       * to pass through to drivers.  For now, there aren't any
>       * passthrough flags.  */
> -    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
> -                       BDRV_REQ_PREFETCH)));
> +    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));
>  
>      /* Handle Copy on Read and associated serialisation */
>      if (flags & BDRV_REQ_COPY_ON_READ) {
> @@ -1458,12 +1457,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>          bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
>      }
>  
> -    /* BDRV_REQ_SERIALISING is only for write operation */
> -    assert(!(flags & BDRV_REQ_SERIALISING));

I think we shoud still keep this assertion as long as read requests
don't mark themselves as serialising when BDRV_REQ_SERIALISING is given.
Otherwise, someone might add the flag to a read request and will later
be surprised that it didn't work.

> -    if (!(flags & BDRV_REQ_NO_SERIALISING)) {
> -        bdrv_wait_serialising_requests(req);
> -    }
> +    bdrv_wait_serialising_requests(req);
>  
>      if (flags & BDRV_REQ_COPY_ON_READ) {
>          int64_t pnum;
> @@ -1711,7 +1705,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
>      bdrv_inc_in_flight(bs);
>  
>      /* Don't do copy-on-read if we read data before write operation */
> -    if (atomic_read(&bs->copy_on_read) && !(flags & BDRV_REQ_NO_SERIALISING)) {
> +    if (atomic_read(&bs->copy_on_read)) {

The comment wants an update, too (or maybe a removal).

>          flags |= BDRV_REQ_COPY_ON_READ;
>      }
>  
> @@ -1852,8 +1846,6 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
>          return -EPERM;
>      }
>  
> -    /* BDRV_REQ_NO_SERIALISING is only for read operation */
> -    assert(!(flags & BDRV_REQ_NO_SERIALISING));
>      assert(!(bs->open_flags & BDRV_O_INACTIVE));
>      assert((bs->open_flags & BDRV_O_NO_IO) == 0);
>      assert(!(flags & ~BDRV_REQ_MASK));
> @@ -3222,9 +3214,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>  
>          /* BDRV_REQ_SERIALISING is only for write operation */
>          assert(!(read_flags & BDRV_REQ_SERIALISING));

Here you kept the assertion, so apart from making sense anyway, it would
also be more consistent to keep it above, too. :-)

> -        if (!(read_flags & BDRV_REQ_NO_SERIALISING)) {
> -            bdrv_wait_serialising_requests(&req);
> -        }
> +        bdrv_wait_serialising_requests(&req);
>  
>          ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,

Kevin