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 <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 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);
+
+    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);
+}
+
+static inline void coroutine_fn
+bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
+{
+    int64_t end_sector = DIV_ROUND_UP(req->offset + req->bytes, BDRV_SECTOR_SIZE);
+    BlockDriverState *bs = child->bs;
+
+    atomic_inc(&bs->write_gen);
+    bdrv_set_dirty(bs, req->offset, req->bytes);
+
+    stat64_max(&bs->wr_highest_offset, req->offset + req->bytes);
+
+    if (ret == 0) {
+        bs->total_sectors = MAX(bs->total_sectors, end_sector);
+    }
+}
+
 /*
  * Forwards an already correctly aligned write request to the BlockDriver,
  * after possibly fragmenting it.
@@ -1548,10 +1590,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
 {
     BlockDriverState *bs = child->bs;
     BlockDriver *drv = bs->drv;
-    bool waited;
     int ret;
 
-    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
     uint64_t bytes_remaining = bytes;
     int max_transfer;
 
@@ -1567,23 +1607,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     assert((offset & (align - 1)) == 0);
     assert((bytes & (align - 1)) == 0);
     assert(!qiov || bytes == qiov->size);
-    assert((bs->open_flags & BDRV_O_NO_IO) == 0);
-    assert(!(flags & ~BDRV_REQ_MASK));
     max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer, INT_MAX),
                                    align);
 
-    waited = wait_serialising_requests(req);
-    assert(!waited || !req->serialising);
-    assert(req->overlap_offset <= offset);
-    assert(offset + 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);
-
-    ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
+    ret = bdrv_co_write_req_prepare(child, req, flags);
 
     if (!ret && bs->detect_zeroes != BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF &&
         !(flags & BDRV_REQ_ZERO_WRITE) && drv->bdrv_co_pwrite_zeroes &&
@@ -1632,15 +1659,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     }
     bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);
 
-    atomic_inc(&bs->write_gen);
-    bdrv_set_dirty(bs, offset, bytes);
-
-    stat64_max(&bs->wr_highest_offset, offset + bytes);
-
     if (ret >= 0) {
-        bs->total_sectors = MAX(bs->total_sectors, end_sector);
         ret = 0;
     }
+    bdrv_co_write_req_finish(child, req, ret);
 
     return ret;
 }
@@ -1755,10 +1777,6 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
     if (!bs->drv) {
         return -ENOMEDIUM;
     }
-    if (bs->read_only) {
-        return -EPERM;
-    }
-    assert(!(bs->open_flags & BDRV_O_INACTIVE));
 
     ret = bdrv_check_byte_request(bs, offset, bytes);
     if (ret < 0) {
-- 
2.17.1

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 <kwolf@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>

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