Re: [Qemu-block] [PATCH v2 4/9] block: Extract common write req handling

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> As a mechanical refactoring patch, this is the first step towards
> unified and more correct write code paths. This is helpful because
> multiple BlockDriverState fields need to be updated after modifying
> image data, and it's hard to maintain in multiple places such as copy
> offload, discard and truncate.
> 
> Suggested-by: Kevin Wolf <address@hidden>
> Signed-off-by: Fam Zheng <address@hidden>

Several lines are longer than 80 characters in this patch.

>  block/io.c | 70 ++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 443a8584c4..03d9eb0a65 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1538,6 +1538,48 @@ fail:
>      return ret;
>  }
>  
> +static inline int coroutine_fn
> +bdrv_co_write_req_prepare(BdrvChild *child, BdrvTrackedRequest *req, int 
> flags)
> +{
> +    BlockDriverState *bs = child->bs;
> +    bool waited;
> +    int64_t end_sector = DIV_ROUND_UP(req->offset + req->bytes, 
> BDRV_SECTOR_SIZE);

You can't simply replace offset/bytes with req->offset/bytes, they are
not necessarily the same thing with unaligned requests. (If they were
the same thing, bdrv_aligned_pwritev() wouldn't need the extra
parameters).

(Multiple instances throughout the patch, I'll mention it only once).

> +
> +    if (bs->read_only) {
> +        return -EPERM;
> +    }
> +    assert(!(bs->open_flags & BDRV_O_INACTIVE));
> +    assert((bs->open_flags & BDRV_O_NO_IO) == 0);
> +    assert(!(flags & ~BDRV_REQ_MASK));
> +    waited = wait_serialising_requests(req);
> +    assert(!waited || !req->serialising);
> +    assert(req->overlap_offset <= req->offset);
> +    assert(req->offset + req->bytes <= req->overlap_offset + 
> req->overlap_bytes);
> +    if (flags & BDRV_REQ_WRITE_UNCHANGED) {
> +        assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
> +    } else {
> +        assert(child->perm & BLK_PERM_WRITE);
> +    }
> +    assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
> +    return notifier_with_return_list_notify(&bs->before_write_notifiers, 
> req);
> +}

A few empty lines wouldn't hurt, to make it visually easier to find the
"real" code between the assertions.

Kevin