[Qemu-devel] [PATCH v2 0/9] block: Fix dst reading after tail copy offloading

[Qemu-devel] [PATCH v2 0/9] block: Fix dst reading after tail copy offloading

[Fam Zheng]

Qcow2 allocates new clusters after the end of the file. If it is the destinaton
of copy offloading, we must adjust dst->bs->total_sectors. Otherwise, further
reads will drop to the "beyond EOF" code path and return zeroes, which problem
is caught by iotests 222.

Follow the logic in the normal write code and update bs->total_sectors after
I/O is done.

While at it, add a few convenient trace points to aid future debug experiences
in the topic.

Fam Zheng (9):
  block: Add copy offloading trace points
  block: Use BdrvChild to discard
  block: Use uint64_t for BdrvTrackedRequest byte fields
  block: Extract common write req handling
  block: Fix handling of image enlarging write
  block: Use common req handling for discard
  block: Use common req handling in copy offloading
  block: Fix bdrv_co_truncate overlap check
  block: Use common write req handling in truncate

 block/blkdebug.c          |   2 +-
 block/blkreplay.c         |   2 +-
 block/block-backend.c     |   2 +-
 block/copy-on-read.c      |   2 +-
 block/file-posix.c        |   2 +
 block/io.c                | 163 +++++++++++++++++++++++++-------------
 block/iscsi.c             |   3 +
 block/mirror.c            |   2 +-
 block/qcow2-refcount.c    |   2 +-
 block/raw-format.c        |   2 +-
 block/throttle.c          |   2 +-
 block/trace-events        |   6 ++
 include/block/block.h     |   4 +-
 include/block/block_int.h |   4 +-
 14 files changed, 130 insertions(+), 68 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/9] block: Add copy offloading trace points

[Fam Zheng]

A few trace points that can help reveal what is happening in a copy
offloading I/O path.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 2 ++
 block/io.c         | 2 ++
 block/iscsi.c      | 3 +++
 block/trace-events | 6 ++++++
 4 files changed, 13 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 829ee538d8..d3b1609410 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1488,6 +1488,8 @@ static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
         ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
                                       aiocb->aio_fd2, &out_off,
                                       bytes, 0);
+        trace_copy_file_range(aiocb->bs, aiocb->aio_fildes, in_off,
+                              aiocb->aio_fd2, out_off, bytes, 0, ret);
         if (ret == 0) {
             /* No progress (e.g. when beyond EOF), let the caller fall back to
              * buffer I/O. */
diff --git a/block/io.c b/block/io.c
index 1a2272fad3..8cc5ee661d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2960,6 +2960,7 @@ int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
                                          BdrvChild *dst, uint64_t dst_offset,
                                          uint64_t bytes, BdrvRequestFlags flags)
 {
+    trace_bdrv_co_copy_range_from(src, src_offset, dst, dst_offset, bytes, flags);
     return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
                                        bytes, flags, true);
 }
@@ -2972,6 +2973,7 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
                                        BdrvChild *dst, uint64_t dst_offset,
                                        uint64_t bytes, BdrvRequestFlags flags)
 {
+    trace_bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes, flags);
     return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
                                        bytes, flags, false);
 }
diff --git a/block/iscsi.c b/block/iscsi.c
index ead2bd5aa7..118555d051 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -44,6 +44,7 @@
 #include "qapi/qmp/qstring.h"
 #include "crypto/secret.h"
 #include "scsi/utils.h"
+#include "trace.h"
 
 /* Conflict between scsi/utils.h and libiscsi! :( */
 #define SCSI_XFER_NONE ISCSI_XFER_NONE
@@ -2396,6 +2397,8 @@ retry:
     }
 
 out_unlock:
+
+    trace_iscsi_xcopy(src_lun, src_offset, dst_lun, dst_offset, bytes, r);
     g_free(iscsi_task.task);
     qemu_mutex_unlock(&dst_lun->mutex);
     g_free(iscsi_task.err_str);
diff --git a/block/trace-events b/block/trace-events
index c35287b48a..1a25a997f2 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -15,6 +15,8 @@ bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs
 bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
 bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64
+bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" flags 0x%x"
+bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" flags 0x%x"
 
 # block/stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
@@ -57,6 +59,7 @@ qmp_block_stream(void *bs, void *job) "bs %p job %p"
 # block/file-posix.c
 paio_submit_co(int64_t offset, int count, int type) "offset %"PRId64" count %d type %d"
 paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "acb %p opaque %p offset %"PRId64" count %d type %d"
+copy_file_range(void *bs, int src, int64_t src_off, int dst, int64_t dst_off, int64_t bytes, int flags, int64_t ret) "bs %p src_fd %d offset %"PRIu64" dst_fd %d offset %"PRIu64" bytes %"PRIu64" flags %d ret %"PRId64
 
 # block/qcow2.c
 qcow2_writev_start_req(void *co, int64_t offset, int bytes) "co %p offset 0x%" PRIx64 " bytes %d"
@@ -150,3 +153,6 @@ nvme_free_req_queue_wait(void *q) "q %p"
 nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
 nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
 nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pages %d"
+
+# block/iscsi.c
+iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, uint64_t bytes, int ret) "src_lun %p offset %"PRIu64" dst_lun %p offset %"PRIu64" bytes %"PRIu64" ret %d"
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 1/9] block: Add copy offloading trace points

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> A few trace points that can help reveal what is happening in a copy
> offloading I/O path.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 2 ++
>  block/io.c         | 2 ++
>  block/iscsi.c      | 3 +++
>  block/trace-events | 6 ++++++
>  4 files changed, 13 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 829ee538d8..d3b1609410 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1488,6 +1488,8 @@ static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
>          ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
>                                        aiocb->aio_fd2, &out_off,
>                                        bytes, 0);
> +        trace_copy_file_range(aiocb->bs, aiocb->aio_fildes, in_off,
> +                              aiocb->aio_fd2, out_off, bytes, 0, ret);

I think it's preferable to have a common prefix for all trace points in
a driver, so they can be enabled with a glob.

paio_* is the existing one for thread pool based file-posix trace
points. Not sure if we like it or want to replace it with something
else.

Kevin

Re: [Qemu-devel] [PATCH v2 1/9] block: Add copy offloading trace points

[Fam Zheng]

On Thu, 07/05 13:08, Kevin Wolf wrote:
> Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> > A few trace points that can help reveal what is happening in a copy
> > offloading I/O path.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 2 ++
> >  block/io.c         | 2 ++
> >  block/iscsi.c      | 3 +++
> >  block/trace-events | 6 ++++++
> >  4 files changed, 13 insertions(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 829ee538d8..d3b1609410 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1488,6 +1488,8 @@ static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
> >          ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
> >                                        aiocb->aio_fd2, &out_off,
> >                                        bytes, 0);
> > +        trace_copy_file_range(aiocb->bs, aiocb->aio_fildes, in_off,
> > +                              aiocb->aio_fd2, out_off, bytes, 0, ret);
> 
> I think it's preferable to have a common prefix for all trace points in
> a driver, so they can be enabled with a glob.
> 
> paio_* is the existing one for thread pool based file-posix trace
> points. Not sure if we like it or want to replace it with something
> else.

Yes, I think it makes sense to add a "file_" prefix.

Fam

[Qemu-devel] [PATCH v2 2/9] block: Use BdrvChild to discard

[Fam Zheng]

Other I/O functions are already using a BdrvChild pointer in the API, so
make discard do the same. It makes it possible to initiate the same
permission checks before doing I/O, and much easier to share the
helper functions for this, which will be added and used by write,
truncate and copy range paths.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/blkdebug.c       |  2 +-
 block/blkreplay.c      |  2 +-
 block/block-backend.c  |  2 +-
 block/copy-on-read.c   |  2 +-
 block/io.c             | 18 +++++++++---------
 block/mirror.c         |  2 +-
 block/qcow2-refcount.c |  2 +-
 block/raw-format.c     |  2 +-
 block/throttle.c       |  2 +-
 include/block/block.h  |  4 ++--
 10 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 526af2a808..0457bf5b66 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -625,7 +625,7 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
         return err;
     }
 
-    return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes);
 }
 
 static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
diff --git a/block/blkreplay.c b/block/blkreplay.c
index b016dbeee7..766150ade6 100755
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -113,7 +113,7 @@ static int coroutine_fn blkreplay_co_pdiscard(BlockDriverState *bs,
                                               int64_t offset, int bytes)
 {
     uint64_t reqid = blkreplay_next_id();
-    int ret = bdrv_co_pdiscard(bs->file->bs, offset, bytes);
+    int ret = bdrv_co_pdiscard(bs->file, offset, bytes);
     block_request_create(reqid, bs, qemu_coroutine_self());
     qemu_coroutine_yield();
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 6b75bca317..dd037b40a2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1559,7 +1559,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
         return ret;
     }
 
-    return bdrv_co_pdiscard(blk_bs(blk), offset, bytes);
+    return bdrv_co_pdiscard(blk->root, offset, bytes);
 }
 
 int blk_co_flush(BlockBackend *blk)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 1dcdaeed69..a19164f9eb 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -116,7 +116,7 @@ static int coroutine_fn cor_co_pwrite_zeroes(BlockDriverState *bs,
 static int coroutine_fn cor_co_pdiscard(BlockDriverState *bs,
                                         int64_t offset, int bytes)
 {
-    return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes);
 }
 
 
diff --git a/block/io.c b/block/io.c
index 8cc5ee661d..3e00667a2a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2590,7 +2590,7 @@ int bdrv_flush(BlockDriverState *bs)
 }
 
 typedef struct DiscardCo {
-    BlockDriverState *bs;
+    BdrvChild *child;
     int64_t offset;
     int bytes;
     int ret;
@@ -2599,17 +2599,17 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque)
 {
     DiscardCo *rwco = opaque;
 
-    rwco->ret = bdrv_co_pdiscard(rwco->bs, rwco->offset, rwco->bytes);
+    rwco->ret = bdrv_co_pdiscard(rwco->child, rwco->offset, rwco->bytes);
 }
 
-int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
-                                  int bytes)
+int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
 {
     BdrvTrackedRequest req;
     int max_pdiscard, ret;
     int head, tail, align;
+    BlockDriverState *bs = child->bs;
 
-    if (!bs->drv) {
+    if (!bs || !bs->drv) {
         return -ENOMEDIUM;
     }
 
@@ -2720,11 +2720,11 @@ out:
     return ret;
 }
 
-int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
+int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes)
 {
     Coroutine *co;
     DiscardCo rwco = {
-        .bs = bs,
+        .child = child,
         .offset = offset,
         .bytes = bytes,
         .ret = NOT_DONE,
@@ -2735,8 +2735,8 @@ int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
         bdrv_pdiscard_co_entry(&rwco);
     } else {
         co = qemu_coroutine_create(bdrv_pdiscard_co_entry, &rwco);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, rwco.ret == NOT_DONE);
+        bdrv_coroutine_enter(child->bs, co);
+        BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
     }
 
     return rwco.ret;
diff --git a/block/mirror.c b/block/mirror.c
index 61bd9f3cf1..b48c3f8cf5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1333,7 +1333,7 @@ static int coroutine_fn bdrv_mirror_top_do_write(BlockDriverState *bs,
         break;
 
     case MIRROR_METHOD_DISCARD:
-        ret = bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
+        ret = bdrv_co_pdiscard(bs->backing, offset, bytes);
         break;
 
     default:
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 18c729aa27..4e1589ad7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -734,7 +734,7 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
 
         /* Discard is optional, ignore the return value */
         if (ret >= 0) {
-            bdrv_pdiscard(bs->file->bs, d->offset, d->bytes);
+            bdrv_pdiscard(bs->file, d->offset, d->bytes);
         }
 
         g_free(d);
diff --git a/block/raw-format.c b/block/raw-format.c
index b78da564d4..d2c95a0ec6 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -297,7 +297,7 @@ static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
     if (ret) {
         return ret;
     }
-    return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes);
 }
 
 static int64_t raw_getlength(BlockDriverState *bs)
diff --git a/block/throttle.c b/block/throttle.c
index f617f23a12..636c9764aa 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -149,7 +149,7 @@ static int coroutine_fn throttle_co_pdiscard(BlockDriverState *bs,
     ThrottleGroupMember *tgm = bs->opaque;
     throttle_group_co_io_limits_intercept(tgm, bytes, true);
 
-    return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
+    return bdrv_co_pdiscard(bs->file, offset, bytes);
 }
 
 static int throttle_co_flush(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index e5c7759a0c..901f5faaeb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -387,8 +387,8 @@ AioWait *bdrv_get_aio_wait(BlockDriverState *bs);
                    bdrv_get_aio_context(bs_),              \
                    cond); })
 
-int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int bytes);
-int bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes);
+int bdrv_pdiscard(BdrvChild *child, int64_t offset, int bytes);
+int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/9] block: Use BdrvChild to discard

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> Other I/O functions are already using a BdrvChild pointer in the API, so
> make discard do the same. It makes it possible to initiate the same
> permission checks before doing I/O, and much easier to share the
> helper functions for this, which will be added and used by write,
> truncate and copy range paths.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

This conflicts with my block branch (because the new blklogwrites driver
needs to be changed to the new interface, too).

Kevin

[Qemu-devel] [PATCH v2 3/9] block: Use uint64_t for BdrvTrackedRequest byte fields

[Fam Zheng]

This matches the types used for bytes in the rest parts of block layer.
In the case of bdrv_co_truncate, new_bytes can be the image size which
probably doesn't fit in a 32 bit int.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c                | 2 +-
 include/block/block_int.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3e00667a2a..443a8584c4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -587,7 +587,7 @@ static void tracked_request_end(BdrvTrackedRequest *req)
 static void tracked_request_begin(BdrvTrackedRequest *req,
                                   BlockDriverState *bs,
                                   int64_t offset,
-                                  unsigned int bytes,
+                                  uint64_t bytes,
                                   enum BdrvTrackedRequestType type)
 {
     *req = (BdrvTrackedRequest){
diff --git a/include/block/block_int.h b/include/block/block_int.h
index af71b414be..66c0e50d82 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -69,12 +69,12 @@ enum BdrvTrackedRequestType {
 typedef struct BdrvTrackedRequest {
     BlockDriverState *bs;
     int64_t offset;
-    unsigned int bytes;
+    uint64_t bytes;
     enum BdrvTrackedRequestType type;
 
     bool serialising;
     int64_t overlap_offset;
-    unsigned int overlap_bytes;
+    uint64_t overlap_bytes;
 
     QLIST_ENTRY(BdrvTrackedRequest) list;
     Coroutine *co; /* owner, used for deadlock detection */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/9] block: Use uint64_t for BdrvTrackedRequest byte fields

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> This matches the types used for bytes in the rest parts of block layer.
> In the case of bdrv_co_truncate, new_bytes can be the image size which
> probably doesn't fit in a 32 bit int.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

mark_request_serialising() has this:

    unsigned int overlap_bytes = ROUND_UP(req->offset + req->bytes, align)
                               - overlap_offset;


There is also:

static bool tracked_request_overlaps(BdrvTrackedRequest *req,
                                     int64_t offset, unsigned int bytes)

Don't these need to be uint64_t now as well?

>  block/io.c                | 2 +-
>  include/block/block_int.h | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 3e00667a2a..443a8584c4 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -587,7 +587,7 @@ static void tracked_request_end(BdrvTrackedRequest *req)
>  static void tracked_request_begin(BdrvTrackedRequest *req,
>                                    BlockDriverState *bs,
>                                    int64_t offset,
> -                                  unsigned int bytes,
> +                                  uint64_t bytes,
>                                    enum BdrvTrackedRequestType type)
>  {
>      *req = (BdrvTrackedRequest){

Should we assert that offset + bytes <= INT64_MAX? We make this
assumption in basically all of the calculations.

Kevin

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

[Fam Zheng]

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

Re: [Qemu-devel] [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 <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

[Qemu-devel] [PATCH v2 5/9] block: Fix handling of image enlarging write

[Fam Zheng]

Two problems exist when a write request that enlarges the image (i.e.
write beyond EOF) finishes:

1) parent is not notified about size change;
2) dirty bitmap is not resized although we try to set the dirty bits;

Fix them just like how bdrv_co_truncate works.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index 03d9eb0a65..f06978dda0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -40,6 +40,7 @@
 
 static AioWait drain_all_aio_wait;
 
+static void bdrv_parent_cb_resize(BlockDriverState *bs);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags);
 
@@ -1571,13 +1572,17 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
     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);
+    if (req->type != BDRV_TRACKED_DISCARD &&
+        ret == 0 &&
+        end_sector > bs->total_sectors) {
+        bs->total_sectors = end_sector;
+        bdrv_parent_cb_resize(bs);
+        bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
     }
+    bdrv_set_dirty(bs, req->offset, req->bytes);
 }
 
 /*
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 5/9] block: Fix handling of image enlarging write

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> Two problems exist when a write request that enlarges the image (i.e.
> write beyond EOF) finishes:
> 
> 1) parent is not notified about size change;
> 2) dirty bitmap is not resized although we try to set the dirty bits;
> 
> Fix them just like how bdrv_co_truncate works.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 03d9eb0a65..f06978dda0 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -40,6 +40,7 @@
>  
>  static AioWait drain_all_aio_wait;
>  
> +static void bdrv_parent_cb_resize(BlockDriverState *bs);
>  static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>      int64_t offset, int bytes, BdrvRequestFlags flags);
>  
> @@ -1571,13 +1572,17 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
>      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);
> +    if (req->type != BDRV_TRACKED_DISCARD &&

This part of the condition belongs in the next patch really because
currently BDRV_TRACKED_DISCARD doesn't happen.

But then, I think we can only skip this part for BDRV_TRACKED_DISCARD
because discards can't extend the image. We already check this with
end_sector > bs->total_sectors, so that condition should be enough.

Maybe what we should do (in the next patch) is assert that this
condition is never true for discards, and then rely on it for the rest
of the code.

> +        ret == 0 &&
> +        end_sector > bs->total_sectors) {
> +        bs->total_sectors = end_sector;
> +        bdrv_parent_cb_resize(bs);
> +        bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
>      }
> +    bdrv_set_dirty(bs, req->offset, req->bytes);
>  }

Kevin

[Qemu-devel] [PATCH v2 6/9] block: Use common req handling for discard

[Fam Zheng]

Reuse the new bdrv_co_write_req_prepare/finish helpers. The variation
here is that discard requests don't affect bs->wr_highest_offset.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index f06978dda0..912fcb962a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1582,7 +1582,18 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
         bdrv_parent_cb_resize(bs);
         bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
     }
-    bdrv_set_dirty(bs, req->offset, req->bytes);
+    if (req->bytes) {
+        switch (req->type) {
+        case BDRV_TRACKED_WRITE:
+            stat64_max(&bs->wr_highest_offset, req->offset + req->bytes);
+            /* fall through, to set dirty bits */
+        case BDRV_TRACKED_DISCARD:
+            bdrv_set_dirty(bs, req->offset, req->bytes);
+            break;
+        default:
+            break;
+        }
+    }
 }
 
 /*
@@ -2643,10 +2654,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
     ret = bdrv_check_byte_request(bs, offset, bytes);
     if (ret < 0) {
         return ret;
-    } else if (bs->read_only) {
-        return -EPERM;
     }
-    assert(!(bs->open_flags & BDRV_O_INACTIVE));
 
     /* Do nothing if disabled.  */
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
@@ -2670,7 +2678,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
     bdrv_inc_in_flight(bs);
     tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_DISCARD);
 
-    ret = notifier_with_return_list_notify(&bs->before_write_notifiers, &req);
+    ret = bdrv_co_write_req_prepare(child, &req, 0);
     if (ret < 0) {
         goto out;
     }
@@ -2736,8 +2744,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
     }
     ret = 0;
 out:
-    atomic_inc(&bs->write_gen);
-    bdrv_set_dirty(bs, req.offset, req.bytes);
+    bdrv_co_write_req_finish(child, &req, ret);
     tracked_request_end(&req);
     bdrv_dec_in_flight(bs);
     return ret;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 6/9] block: Use common req handling for discard

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> Reuse the new bdrv_co_write_req_prepare/finish helpers. The variation
> here is that discard requests don't affect bs->wr_highest_offset.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index f06978dda0..912fcb962a 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1582,7 +1582,18 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
>          bdrv_parent_cb_resize(bs);
>          bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
>      }
> -    bdrv_set_dirty(bs, req->offset, req->bytes);
> +    if (req->bytes) {
> +        switch (req->type) {
> +        case BDRV_TRACKED_WRITE:
> +            stat64_max(&bs->wr_highest_offset, req->offset + req->bytes);

You forgot to remove this line above, so now this one is redundant and
we still execute it always.

Apart from that, why shouldn't discard be included in
bs->wr_highest_offset? It's an access to an area in the image that must
be present, so it indicates a larger file size, doesn't it?

> +            /* fall through, to set dirty bits */
> +        case BDRV_TRACKED_DISCARD:
> +            bdrv_set_dirty(bs, req->offset, req->bytes);
> +            break;
> +        default:
> +            break;
> +        }
> +    }
>  }

Kevin

Re: [Qemu-devel] [PATCH v2 6/9] block: Use common req handling for discard

[Fam Zheng]

On Thu, 07/05 14:44, Kevin Wolf wrote:
> Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> > Reuse the new bdrv_co_write_req_prepare/finish helpers. The variation
> > here is that discard requests don't affect bs->wr_highest_offset.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/io.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index f06978dda0..912fcb962a 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1582,7 +1582,18 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
> >          bdrv_parent_cb_resize(bs);
> >          bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
> >      }
> > -    bdrv_set_dirty(bs, req->offset, req->bytes);
> > +    if (req->bytes) {
> > +        switch (req->type) {
> > +        case BDRV_TRACKED_WRITE:
> > +            stat64_max(&bs->wr_highest_offset, req->offset + req->bytes);
> 
> You forgot to remove this line above, so now this one is redundant and
> we still execute it always.
> 
> Apart from that, why shouldn't discard be included in
> bs->wr_highest_offset? It's an access to an area in the image that must
> be present, so it indicates a larger file size, doesn't it?

I'm not sure. wr_highest_offset is used for management to allocate disk space. A
discard request is on the contrary for releasing disk space. Since guest is
allowed to discard unallocated sectors even though it should be nop in the
backend, such an operation shouldn't cause a user visible change in
@wr_highest_offset in QMP.

Fam

> 
> > +            /* fall through, to set dirty bits */
> > +        case BDRV_TRACKED_DISCARD:
> > +            bdrv_set_dirty(bs, req->offset, req->bytes);
> > +            break;
> > +        default:
> > +            break;
> > +        }
> > +    }
> >  }
> 
> Kevin

Re: [Qemu-devel] [PATCH v2 6/9] block: Use common req handling for discard

[Kevin Wolf]

Am 06.07.2018 um 08:51 hat Fam Zheng geschrieben:
> On Thu, 07/05 14:44, Kevin Wolf wrote:
> > Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> > > Reuse the new bdrv_co_write_req_prepare/finish helpers. The variation
> > > here is that discard requests don't affect bs->wr_highest_offset.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  block/io.c | 21 ++++++++++++++-------
> > >  1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/block/io.c b/block/io.c
> > > index f06978dda0..912fcb962a 100644
> > > --- a/block/io.c
> > > +++ b/block/io.c
> > > @@ -1582,7 +1582,18 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
> > >          bdrv_parent_cb_resize(bs);
> > >          bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
> > >      }
> > > -    bdrv_set_dirty(bs, req->offset, req->bytes);
> > > +    if (req->bytes) {
> > > +        switch (req->type) {
> > > +        case BDRV_TRACKED_WRITE:
> > > +            stat64_max(&bs->wr_highest_offset, req->offset + req->bytes);
> > 
> > You forgot to remove this line above, so now this one is redundant and
> > we still execute it always.
> > 
> > Apart from that, why shouldn't discard be included in
> > bs->wr_highest_offset? It's an access to an area in the image that must
> > be present, so it indicates a larger file size, doesn't it?
> 
> I'm not sure. wr_highest_offset is used for management to allocate disk space. A
> discard request is on the contrary for releasing disk space. Since guest is
> allowed to discard unallocated sectors even though it should be nop in the
> backend, such an operation shouldn't cause a user visible change in
> @wr_highest_offset in QMP.

It's a tough call. I think wr_highest_offset is more about the file size
than about allocation status. Though as long as discard can't grow the
image if you discard beyond EOF, not increasing wr_highest_offset is
indeed more consistent in a way.

Kevin

[Qemu-devel] [PATCH v2 7/9] block: Use common req handling in copy offloading

[Fam Zheng]

This brings the request handling logic inline with write and discard,
fixing write_gen, resize_cb, dirty bitmaps and image size refreshing.
The last of these issues broke iotest case 222, which is now fixed.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 912fcb962a..d07849fa96 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2962,7 +2962,11 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
 
     if (!(flags & BDRV_REQ_NO_SERIALISING)) {
         wait_serialising_requests(&src_req);
-        wait_serialising_requests(&dst_req);
+    }
+
+    ret = bdrv_co_write_req_prepare(dst, &dst_req, flags);
+    if (ret) {
+        goto out;
     }
     if (recurse_src) {
         ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
@@ -2975,6 +2979,8 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
                                                   dst, dst_offset,
                                                   bytes, flags);
     }
+out:
+    bdrv_co_write_req_finish(dst, &dst_req, ret);
     tracked_request_end(&src_req);
     tracked_request_end(&dst_req);
     bdrv_dec_in_flight(src->bs);
-- 
2.17.1

[Qemu-devel] [PATCH v2 8/9] block: Fix bdrv_co_truncate overlap check

[Fam Zheng]

If we are growing the image and potentially using preallocation for the
new area, we need to make sure that no write requests are made to the
"preallocated" area which [@old_size, @offset), not [@offset, offset * 2
- @old_size).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index d07849fa96..ed18eb0ca3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3070,7 +3070,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
     }
 
     bdrv_inc_in_flight(bs);
-    tracked_request_begin(&req, bs, offset, new_bytes, BDRV_TRACKED_TRUNCATE);
+    tracked_request_begin(&req, bs, offset - new_bytes, new_bytes,
+                          BDRV_TRACKED_TRUNCATE);
 
     /* If we are growing the image and potentially using preallocation for the
      * new area, we need to make sure that no write requests are made to it
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 8/9] block: Fix bdrv_co_truncate overlap check

[Eric Blake]

On 07/05/2018 02:37 AM, Fam Zheng wrote:
> If we are growing the image and potentially using preallocation for the
> new area, we need to make sure that no write requests are made to the
> "preallocated" area which [@old_size, @offset), not [@offset, offset * 2

s/which/which is/

> - @old_size).
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/io.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/io.c b/block/io.c
> index d07849fa96..ed18eb0ca3 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3070,7 +3070,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
>       }
>   
>       bdrv_inc_in_flight(bs);
> -    tracked_request_begin(&req, bs, offset, new_bytes, BDRV_TRACKED_TRUNCATE);
> +    tracked_request_begin(&req, bs, offset - new_bytes, new_bytes,
> +                          BDRV_TRACKED_TRUNCATE);

Is it any more legible to do s/offset - new_bytes/old_size/, since those 
are equivalent?

>   
>       /* If we are growing the image and potentially using preallocation for the
>        * new area, we need to make sure that no write requests are made to it
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 8/9] block: Fix bdrv_co_truncate overlap check

[Fam Zheng]

On Fri, 07/06 17:09, Eric Blake wrote:
> On 07/05/2018 02:37 AM, Fam Zheng wrote:
> > If we are growing the image and potentially using preallocation for the
> > new area, we need to make sure that no write requests are made to the
> > "preallocated" area which [@old_size, @offset), not [@offset, offset * 2
> 
> s/which/which is/
> 
> > - @old_size).
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >   block/io.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/block/io.c b/block/io.c
> > index d07849fa96..ed18eb0ca3 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -3070,7 +3070,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
> >       }
> >       bdrv_inc_in_flight(bs);
> > -    tracked_request_begin(&req, bs, offset, new_bytes, BDRV_TRACKED_TRUNCATE);
> > +    tracked_request_begin(&req, bs, offset - new_bytes, new_bytes,
> > +                          BDRV_TRACKED_TRUNCATE);
> 
> Is it any more legible to do s/offset - new_bytes/old_size/, since those are
> equivalent?

No they are not. offset - new_bytes is either old_size (if expanding), or
smaller than old_size (if shrinking).

Fam

> 
> >       /* If we are growing the image and potentially using preallocation for the
> >        * new area, we need to make sure that no write requests are made to it
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH v2 9/9] block: Use common write req handling in truncate

[Fam Zheng]

Truncation is the last to convert from open coded req handling to
reusing helpers. This time the permission check in prepare has to adapt
to the new caller: it checks a different permission bit, and don't
trigger the before write notifier.

Also, truncation should always trigger a bs->total_sectors update and in
turn call parent resize_cb. Update the condition in finish helper, too.

It's intended to do a duplicated bs->read_only check before calling
bdrv_co_write_req_prepare() so that we can be more informative with the
error message, as bdrv_co_write_req_prepare() doesn't have Error
parameter.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 54 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/block/io.c b/block/io.c
index ed18eb0ca3..53f2bf4103 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1556,13 +1556,22 @@ bdrv_co_write_req_prepare(BdrvChild *child, BdrvTrackedRequest *req, int flags)
     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);
+    switch (req->type) {
+    case BDRV_TRACKED_WRITE:
+    case BDRV_TRACKED_DISCARD:
+        if (flags & BDRV_REQ_WRITE_UNCHANGED) {
+            assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
+        } else {
+            assert(child->perm & BLK_PERM_WRITE);
+        }
+        return notifier_with_return_list_notify(&bs->before_write_notifiers, req);
+    case BDRV_TRACKED_TRUNCATE:
+        assert(child->perm & BLK_PERM_RESIZE);
+        return 0;
+    default:
+        abort();
+    }
 }
 
 static inline void coroutine_fn
@@ -1575,9 +1584,10 @@ bdrv_co_write_req_finish(BdrvChild *child, BdrvTrackedRequest *req, int ret)
 
     stat64_max(&bs->wr_highest_offset, req->offset + req->bytes);
 
-    if (req->type != BDRV_TRACKED_DISCARD &&
-        ret == 0 &&
-        end_sector > bs->total_sectors) {
+    if (ret == 0 &&
+        (req->type == BDRV_TRACKED_TRUNCATE ||
+         (req->type != BDRV_TRACKED_DISCARD &&
+          end_sector > bs->total_sectors))) {
         bs->total_sectors = end_sector;
         bdrv_parent_cb_resize(bs);
         bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
@@ -3045,7 +3055,6 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
     int64_t old_size, new_bytes;
     int ret;
 
-    assert(child->perm & BLK_PERM_RESIZE);
 
     /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
     if (!drv) {
@@ -3078,7 +3087,16 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
      * concurrently or they might be overwritten by preallocation. */
     if (new_bytes) {
         mark_request_serialising(&req, 1);
-        wait_serialising_requests(&req);
+    }
+    if (bs->read_only) {
+        error_setg(errp, "Image is read-only");
+        ret = -EACCES;
+        goto out;
+    }
+    ret = bdrv_co_write_req_prepare(child, &req, 0);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to prepare request for truncation");
+        goto out;
     }
 
     if (!drv->bdrv_co_truncate) {
@@ -3090,13 +3108,6 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
         ret = -ENOTSUP;
         goto out;
     }
-    if (bs->read_only) {
-        error_setg(errp, "Image is read-only");
-        ret = -EACCES;
-        goto out;
-    }
-
-    assert(!(bs->open_flags & BDRV_O_INACTIVE));
 
     ret = drv->bdrv_co_truncate(bs, offset, prealloc, errp);
     if (ret < 0) {
@@ -3108,9 +3119,10 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset,
     } else {
         offset = bs->total_sectors * BDRV_SECTOR_SIZE;
     }
-    bdrv_dirty_bitmap_truncate(bs, offset);
-    bdrv_parent_cb_resize(bs);
-    atomic_inc(&bs->write_gen);
+    /* It's possible that truncation succeeded but refresh_total_sectors
+     * failed, but the latter doesn't affect how we should finish the request.
+     * Pass 0 as the last parameter so that dirty bitmaps etc. are handled. */
+    bdrv_co_write_req_finish(child, &req, 0);
 
 out:
     tracked_request_end(&req);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 9/9] block: Use common write req handling in truncate

[Eric Blake]

On 07/05/2018 02:37 AM, Fam Zheng wrote:
> Truncation is the last to convert from open coded req handling to
> reusing helpers. This time the permission check in prepare has to adapt
> to the new caller: it checks a different permission bit, and don't

did you mean "won't" or "doesn't"?

> trigger the before write notifier.
> 
> Also, truncation should always trigger a bs->total_sectors update and in
> turn call parent resize_cb. Update the condition in finish helper, too.
> 
> It's intended to do a duplicated bs->read_only check before calling
> bdrv_co_write_req_prepare() so that we can be more informative with the
> error message, as bdrv_co_write_req_prepare() doesn't have Error
> parameter.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/io.c | 54 +++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 33 insertions(+), 21 deletions(-)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 9/9] block: Use common write req handling in truncate

[Fam Zheng]

On Fri, 07/06 17:12, Eric Blake wrote:
> On 07/05/2018 02:37 AM, Fam Zheng wrote:
> > Truncation is the last to convert from open coded req handling to
> > reusing helpers. This time the permission check in prepare has to adapt
> > to the new caller: it checks a different permission bit, and don't
> 
> did you mean "won't" or "doesn't"?

Yeah, "doesn't'. Thanks!

Fam

Re: [Qemu-devel] [PATCH v2 0/9] block: Fix dst reading after tail copy offloading

[Kevin Wolf]

Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben:
> Qcow2 allocates new clusters after the end of the file. If it is the destinaton
> of copy offloading, we must adjust dst->bs->total_sectors. Otherwise, further
> reads will drop to the "beyond EOF" code path and return zeroes, which problem
> is caught by iotests 222.
> 
> Follow the logic in the normal write code and update bs->total_sectors after
> I/O is done.
> 
> While at it, add a few convenient trace points to aid future debug experiences
> in the topic.

Apart from the comments I made, this series also conflicts with
Vladimir's "[PATCH v3 0/4] fix image fleecing". Can you please rebase on
top of his series for the next version?

Kevin