[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/9] block: Extract common write req handling
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/9] block: Extract common write req handling |
Date: |
Thu, 5 Jul 2018 13:31:24 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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
- [Qemu-devel] [PATCH v2 0/9] block: Fix dst reading after tail copy offloading, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 1/9] block: Add copy offloading trace points, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 2/9] block: Use BdrvChild to discard, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 3/9] block: Use uint64_t for BdrvTrackedRequest byte fields, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 4/9] block: Extract common write req handling, Fam Zheng, 2018/07/05
- Re: [Qemu-devel] [PATCH v2 4/9] block: Extract common write req handling,
Kevin Wolf <=
- [Qemu-devel] [PATCH v2 5/9] block: Fix handling of image enlarging write, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 6/9] block: Use common req handling for discard, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 7/9] block: Use common req handling in copy offloading, Fam Zheng, 2018/07/05
- [Qemu-devel] [PATCH v2 8/9] block: Fix bdrv_co_truncate overlap check, Fam Zheng, 2018/07/05