[Qemu-devel] [PATCH v6 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Qemu-devel] [PATCH v6 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Fam Zheng]

An unaligned zero write causes NULL deferencing in bdrv_co_do_pwritev. That
path is reachable from bdrv_co_write_zeroes and bdrv_aio_write_zeroes.

You can easily trigger through the former with qemu-io, as the test case added
by 61815d6e0aa. For bdrv_aio_write_zeroes, in common cases there's always a
format driver (which uses 512 alignment), so it would be much rarer to have
unaligned requests (only concerning top level here, when the request goes down
to bs->file, where for example the alignment is 4k, it would then be calling
bdrv_co_write_zeroes because it's in a coroutine).

fc3959e4669a1c fixed bdrv_co_write_zeroes but not bdrv_aio_write_zeroes.  The
lattern is the actually used one by device model. Revert the previous fix, do
it in bdrv_co_do_pwritev, to cover both paths.

v6: Split the function to make it easier to reason. (Stefan)
    Add rev-by in 1, 3. (Stefan)

v5: Rebase on to the block/io.c split. (Kevin)

v4: "if (!qiov)" -> "if (!qiov && bytes >= align)". (Paolo)

v3: Fix the case where the unaligned request is contained within the first
    block. (Paolo)
    Also update iotests 033 to cover the code path with qemu-io.

v2: Split to three aligned pwritev.


Fam Zheng (3):
  Revert "block: Fix unaligned zero write"
  block: Fix NULL deference for unaligned write if qiov is NULL
  qemu-iotests: Test unaligned sub-block zero write

 block/io.c                 | 140 ++++++++++++++++++++++++++++++++-------------
 tests/qemu-iotests/033     |  13 +++++
 tests/qemu-iotests/033.out |  30 ++++++++++
 3 files changed, 144 insertions(+), 39 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v6 1/3] Revert "block: Fix unaligned zero write"

[Fam Zheng]

This reverts commit fc3959e4669a1c2149b91ccb05101cfc7ae1fc05.

The core write code already handles the case, so remove this
duplication.

Because commit 61007b316 moved the touched code from block.c to
block/io.c, the change is manually reverted.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 45 ++++++---------------------------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/block/io.c b/block/io.c
index 1ce62c4..4e5a92e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -929,19 +929,6 @@ out:
     return ret;
 }
 
-static inline uint64_t bdrv_get_align(BlockDriverState *bs)
-{
-    /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
-    return MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
-}
-
-static inline bool bdrv_req_is_aligned(BlockDriverState *bs,
-                                       int64_t offset, size_t bytes)
-{
-    int64_t align = bdrv_get_align(bs);
-    return !(offset & (align - 1) || (bytes & (align - 1)));
-}
-
 /*
  * Handle a read request in coroutine context
  */
@@ -952,7 +939,8 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
     BlockDriver *drv = bs->drv;
     BdrvTrackedRequest req;
 
-    uint64_t align = bdrv_get_align(bs);
+    /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
     uint8_t *head_buf = NULL;
     uint8_t *tail_buf = NULL;
     QEMUIOVector local_qiov;
@@ -1194,7 +1182,8 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
     BdrvRequestFlags flags)
 {
     BdrvTrackedRequest req;
-    uint64_t align = bdrv_get_align(bs);
+    /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
     uint8_t *head_buf = NULL;
     uint8_t *tail_buf = NULL;
     QEMUIOVector local_qiov;
@@ -1293,10 +1282,6 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
         bytes = ROUND_UP(bytes, align);
     }
 
-    if (use_local_qiov) {
-        /* Local buffer may have non-zero data. */
-        flags &= ~BDRV_REQ_ZERO_WRITE;
-    }
     ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
                                use_local_qiov ? &local_qiov : qiov,
                                flags);
@@ -1337,32 +1322,14 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
                                       int64_t sector_num, int nb_sectors,
                                       BdrvRequestFlags flags)
 {
-    int ret;
-
     trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
 
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
         flags &= ~BDRV_REQ_MAY_UNMAP;
     }
-    if (bdrv_req_is_aligned(bs, sector_num << BDRV_SECTOR_BITS,
-                            nb_sectors << BDRV_SECTOR_BITS)) {
-        ret = bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
-                                BDRV_REQ_ZERO_WRITE | flags);
-    } else {
-        uint8_t *buf;
-        QEMUIOVector local_qiov;
-        size_t bytes = nb_sectors << BDRV_SECTOR_BITS;
 
-        buf = qemu_memalign(bdrv_opt_mem_align(bs), bytes);
-        memset(buf, 0, bytes);
-        qemu_iovec_init(&local_qiov, 1);
-        qemu_iovec_add(&local_qiov, buf, bytes);
-
-        ret = bdrv_co_do_writev(bs, sector_num, nb_sectors, &local_qiov,
-                                BDRV_REQ_ZERO_WRITE | flags);
-        qemu_vfree(buf);
-    }
-    return ret;
+    return bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
+                             BDRV_REQ_ZERO_WRITE | flags);
 }
 
 int bdrv_flush_all(void)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v6 1/3] Revert "block: Fix unaligned zero write"

[Kevin Wolf]

Am 12.05.2015 um 08:09 hat Fam Zheng geschrieben:
> This reverts commit fc3959e4669a1c2149b91ccb05101cfc7ae1fc05.
> 
> The core write code already handles the case, so remove this
> duplication.
> 
> Because commit 61007b316 moved the touched code from block.c to
> block/io.c, the change is manually reverted.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Fam Zheng]

For zero write, callers pass in NULL qiov (qemu-io "write -z" or
scsi-disk "write same").

Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case
for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler
fix would be in bdrv_co_do_pwritev which is the NULL dereference point
and covers both cases.

So don't access it in bdrv_co_do_pwritev in this case, use three aligned
writes.

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

diff --git a/block/io.c b/block/io.c
index 4e5a92e..d766220 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1174,6 +1174,97 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
     return ret;
 }
 
+static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
+                                                int64_t offset,
+                                                unsigned int bytes,
+                                                BdrvRequestFlags flags)
+{
+    BdrvTrackedRequest req;
+    uint8_t *buf = NULL;
+    QEMUIOVector local_qiov;
+    struct iovec iov;
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+    unsigned int head_padding_bytes, tail_padding_bytes;
+    int ret;
+
+    head_padding_bytes = offset & (align - 1);
+    tail_padding_bytes = align - ((offset + bytes) & (align - 1));
+    tracked_request_begin(&req, bs, offset, bytes, true);
+
+    mark_request_serialising(&req, align);
+    wait_serialising_requests(&req);
+
+    assert(flags & BDRV_REQ_ZERO_WRITE);
+    if (head_padding_bytes || tail_padding_bytes) {
+        buf = qemu_blockalign(bs, align);
+        iov = (struct iovec) {
+            .iov_base   = buf,
+            .iov_len    = align,
+        };
+        qemu_iovec_init_external(&local_qiov, &iov, 1);
+    }
+    if (head_padding_bytes) {
+        uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
+
+        /* RMW the unaligned part before head. */
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD);
+        ret = bdrv_aligned_preadv(bs, &req, offset & ~(align - 1), align,
+                                  align, &local_qiov, 0);
+        if (ret < 0) {
+            goto fail;
+        }
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
+
+        memset(buf + head_padding_bytes, 0, zero_bytes);
+        ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
+                                   &local_qiov,
+                                   flags & ~BDRV_REQ_ZERO_WRITE);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += zero_bytes;
+        bytes -= zero_bytes;
+    }
+
+    assert((offset & (align - 1)) == 0);
+    if (bytes >= align) {
+        /* Write the aligned part in the middle. */
+        uint64_t aligned_bytes = bytes & ~(align - 1);
+        ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes,
+                                   NULL, flags);
+        if (ret < 0) {
+            goto fail;
+        }
+        bytes -= aligned_bytes;
+        offset += aligned_bytes;
+    }
+
+    assert((offset & (align - 1)) == 0);
+    if (bytes) {
+        assert(align == tail_padding_bytes + bytes);
+        /* RMW the unaligned part after tail. */
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL);
+        ret = bdrv_aligned_preadv(bs, &req, offset, align,
+                                  align, &local_qiov, 0);
+        if (ret < 0) {
+            goto fail;
+        }
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
+
+        memset(buf, 0, bytes);
+        printf("tail part %ld %d\n", offset, bytes);
+        ret = bdrv_aligned_pwritev(bs, &req, offset, align,
+                                   &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE);
+    }
+fail:
+    tracked_request_end(&req);
+    if (buf) {
+        qemu_vfree(buf);
+    }
+    return ret;
+
+}
+
 /*
  * Handle a write request in coroutine context
  */
@@ -1207,6 +1298,10 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
         bdrv_io_limits_intercept(bs, bytes, true);
     }
 
+    if (!qiov) {
+        return bdrv_co_do_zero_pwritev(bs, offset, bytes, flags);
+    }
+
     /*
      * Align write if necessary by performing a read-modify-write cycle.
      * Pad qiov with the read parts and be sure to have a tracked request not
-- 
2.1.0

Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Kevin Wolf]

Am 12.05.2015 um 08:09 hat Fam Zheng geschrieben:
> For zero write, callers pass in NULL qiov (qemu-io "write -z" or
> scsi-disk "write same").
> 
> Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case
> for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler
> fix would be in bdrv_co_do_pwritev which is the NULL dereference point
> and covers both cases.
> 
> So don't access it in bdrv_co_do_pwritev in this case, use three aligned
> writes.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 4e5a92e..d766220 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1174,6 +1174,97 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>      return ret;
>  }
>  
> +static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
> +                                                int64_t offset,
> +                                                unsigned int bytes,
> +                                                BdrvRequestFlags flags)
> +{
> +    BdrvTrackedRequest req;
> +    uint8_t *buf = NULL;
> +    QEMUIOVector local_qiov;
> +    struct iovec iov;
> +    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
> +    unsigned int head_padding_bytes, tail_padding_bytes;
> +    int ret;
> +
> +    head_padding_bytes = offset & (align - 1);
> +    tail_padding_bytes = align - ((offset + bytes) & (align - 1));

Don't we have macros for these calculations?

> +    tracked_request_begin(&req, bs, offset, bytes, true);

Why duplicate this when it would already be the next line in
bdrv_co_do_pwritev()?

> +    mark_request_serialising(&req, align);
> +    wait_serialising_requests(&req);

So this patch serialises all zero writes, even if they are perfectly
aligned? Why?

Actually, even for misaligned requests, I think the part in the middle
doesn't require any serialisation, only the RMW parts do.

> +    assert(flags & BDRV_REQ_ZERO_WRITE);
> +    if (head_padding_bytes || tail_padding_bytes) {
> +        buf = qemu_blockalign(bs, align);
> +        iov = (struct iovec) {
> +            .iov_base   = buf,
> +            .iov_len    = align,
> +        };
> +        qemu_iovec_init_external(&local_qiov, &iov, 1);
> +    }
> +    if (head_padding_bytes) {
> +        uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
> +
> +        /* RMW the unaligned part before head. */
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD);
> +        ret = bdrv_aligned_preadv(bs, &req, offset & ~(align - 1), align,
> +                                  align, &local_qiov, 0);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> +
> +        memset(buf + head_padding_bytes, 0, zero_bytes);
> +        ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
> +                                   &local_qiov,
> +                                   flags & ~BDRV_REQ_ZERO_WRITE);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        offset += zero_bytes;
> +        bytes -= zero_bytes;
> +    }
> +
> +    assert((offset & (align - 1)) == 0);
> +    if (bytes >= align) {
> +        /* Write the aligned part in the middle. */
> +        uint64_t aligned_bytes = bytes & ~(align - 1);
> +        ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes,
> +                                   NULL, flags);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        bytes -= aligned_bytes;
> +        offset += aligned_bytes;
> +    }
> +
> +    assert((offset & (align - 1)) == 0);
> +    if (bytes) {
> +        assert(align == tail_padding_bytes + bytes);
> +        /* RMW the unaligned part after tail. */
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL);
> +        ret = bdrv_aligned_preadv(bs, &req, offset, align,
> +                                  align, &local_qiov, 0);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> +
> +        memset(buf, 0, bytes);
> +        printf("tail part %ld %d\n", offset, bytes);
> +        ret = bdrv_aligned_pwritev(bs, &req, offset, align,
> +                                   &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE);
> +    }
> +fail:
> +    tracked_request_end(&req);
> +    if (buf) {
> +        qemu_vfree(buf);
> +    }
> +    return ret;
> +
> +}
> +
>  /*
>   * Handle a write request in coroutine context
>   */
> @@ -1207,6 +1298,10 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
>          bdrv_io_limits_intercept(bs, bytes, true);
>      }
>  
> +    if (!qiov) {
> +        return bdrv_co_do_zero_pwritev(bs, offset, bytes, flags);
> +    }
> +
>      /*
>       * Align write if necessary by performing a read-modify-write cycle.
>       * Pad qiov with the read parts and be sure to have a tracked request not

Kevin

Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Fam Zheng]

On Tue, 05/12 13:52, Kevin Wolf wrote:
> Am 12.05.2015 um 08:09 hat Fam Zheng geschrieben:
> > For zero write, callers pass in NULL qiov (qemu-io "write -z" or
> > scsi-disk "write same").
> > 
> > Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case
> > for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler
> > fix would be in bdrv_co_do_pwritev which is the NULL dereference point
> > and covers both cases.
> > 
> > So don't access it in bdrv_co_do_pwritev in this case, use three aligned
> > writes.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/io.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 4e5a92e..d766220 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1174,6 +1174,97 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
> >      return ret;
> >  }
> >  
> > +static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
> > +                                                int64_t offset,
> > +                                                unsigned int bytes,
> > +                                                BdrvRequestFlags flags)
> > +{
> > +    BdrvTrackedRequest req;
> > +    uint8_t *buf = NULL;
> > +    QEMUIOVector local_qiov;
> > +    struct iovec iov;
> > +    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
> > +    unsigned int head_padding_bytes, tail_padding_bytes;
> > +    int ret;
> > +
> > +    head_padding_bytes = offset & (align - 1);
> > +    tail_padding_bytes = align - ((offset + bytes) & (align - 1));
> 
> Don't we have macros for these calculations?

No, I don't see any.

> > +    tracked_request_begin(&req, bs, offset, bytes, true);
> 
> Why duplicate this when it would already be the next line in
> bdrv_co_do_pwritev()?

I'll remove the duplication.

> 
> > +    mark_request_serialising(&req, align);
> > +    wait_serialising_requests(&req);
> 
> So this patch serialises all zero writes, even if they are perfectly
> aligned? Why?
> 
> Actually, even for misaligned requests, I think the part in the middle
> doesn't require any serialisation, only the RMW parts do.

I'll move to branches.

Thanks,
Fam

Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3617 bytes --]

On Tue, May 12, 2015 at 02:09:31PM +0800, Fam Zheng wrote:
> +static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
> +                                                int64_t offset,
> +                                                unsigned int bytes,
> +                                                BdrvRequestFlags flags)
> +{
> +    BdrvTrackedRequest req;
> +    uint8_t *buf = NULL;
> +    QEMUIOVector local_qiov;
> +    struct iovec iov;
> +    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
> +    unsigned int head_padding_bytes, tail_padding_bytes;
> +    int ret;
> +
> +    head_padding_bytes = offset & (align - 1);
> +    tail_padding_bytes = align - ((offset + bytes) & (align - 1));
> +    tracked_request_begin(&req, bs, offset, bytes, true);
> +
> +    mark_request_serialising(&req, align);

This is only necessary if (head_padding_bytes || tail_padding_bytes).
Serialized requests are more expensive so let's only do it when
necessary.

> +    wait_serialising_requests(&req);
> +
> +    assert(flags & BDRV_REQ_ZERO_WRITE);
> +    if (head_padding_bytes || tail_padding_bytes) {
> +        buf = qemu_blockalign(bs, align);
> +        iov = (struct iovec) {
> +            .iov_base   = buf,
> +            .iov_len    = align,
> +        };
> +        qemu_iovec_init_external(&local_qiov, &iov, 1);
> +    }
> +    if (head_padding_bytes) {
> +        uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
> +
> +        /* RMW the unaligned part before head. */
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD);
> +        ret = bdrv_aligned_preadv(bs, &req, offset & ~(align - 1), align,
> +                                  align, &local_qiov, 0);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> +
> +        memset(buf + head_padding_bytes, 0, zero_bytes);
> +        ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
> +                                   &local_qiov,
> +                                   flags & ~BDRV_REQ_ZERO_WRITE);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        offset += zero_bytes;
> +        bytes -= zero_bytes;
> +    }
> +
> +    assert((offset & (align - 1)) == 0);
> +    if (bytes >= align) {
> +        /* Write the aligned part in the middle. */
> +        uint64_t aligned_bytes = bytes & ~(align - 1);
> +        ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes,
> +                                   NULL, flags);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        bytes -= aligned_bytes;
> +        offset += aligned_bytes;
> +    }
> +
> +    assert((offset & (align - 1)) == 0);
> +    if (bytes) {
> +        assert(align == tail_padding_bytes + bytes);
> +        /* RMW the unaligned part after tail. */
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL);
> +        ret = bdrv_aligned_preadv(bs, &req, offset, align,
> +                                  align, &local_qiov, 0);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> +
> +        memset(buf, 0, bytes);
> +        printf("tail part %ld %d\n", offset, bytes);

Please drop the debug printf.

> +        ret = bdrv_aligned_pwritev(bs, &req, offset, align,
> +                                   &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE);
> +    }
> +fail:
> +    tracked_request_end(&req);
> +    if (buf) {
> +        qemu_vfree(buf);
> +    }

if (buf) is unnecessary.  qemu_vfree(NULL) is a nop.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Fam Zheng]

On Tue, 05/12 13:18, Stefan Hajnoczi wrote:
> On Tue, May 12, 2015 at 02:09:31PM +0800, Fam Zheng wrote:
> > +static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
> > +                                                int64_t offset,
> > +                                                unsigned int bytes,
> > +                                                BdrvRequestFlags flags)
> > +{
> > +    BdrvTrackedRequest req;
> > +    uint8_t *buf = NULL;
> > +    QEMUIOVector local_qiov;
> > +    struct iovec iov;
> > +    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
> > +    unsigned int head_padding_bytes, tail_padding_bytes;
> > +    int ret;
> > +
> > +    head_padding_bytes = offset & (align - 1);
> > +    tail_padding_bytes = align - ((offset + bytes) & (align - 1));
> > +    tracked_request_begin(&req, bs, offset, bytes, true);
> > +
> > +    mark_request_serialising(&req, align);
> 
> This is only necessary if (head_padding_bytes || tail_padding_bytes).
> Serialized requests are more expensive so let's only do it when
> necessary.
> 
> > +    wait_serialising_requests(&req);
> > +
> > +    assert(flags & BDRV_REQ_ZERO_WRITE);
> > +    if (head_padding_bytes || tail_padding_bytes) {
> > +        buf = qemu_blockalign(bs, align);
> > +        iov = (struct iovec) {
> > +            .iov_base   = buf,
> > +            .iov_len    = align,
> > +        };
> > +        qemu_iovec_init_external(&local_qiov, &iov, 1);
> > +    }
> > +    if (head_padding_bytes) {
> > +        uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
> > +
> > +        /* RMW the unaligned part before head. */
> > +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD);
> > +        ret = bdrv_aligned_preadv(bs, &req, offset & ~(align - 1), align,
> > +                                  align, &local_qiov, 0);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> > +
> > +        memset(buf + head_padding_bytes, 0, zero_bytes);
> > +        ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
> > +                                   &local_qiov,
> > +                                   flags & ~BDRV_REQ_ZERO_WRITE);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +        offset += zero_bytes;
> > +        bytes -= zero_bytes;
> > +    }
> > +
> > +    assert((offset & (align - 1)) == 0);
> > +    if (bytes >= align) {
> > +        /* Write the aligned part in the middle. */
> > +        uint64_t aligned_bytes = bytes & ~(align - 1);
> > +        ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes,
> > +                                   NULL, flags);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +        bytes -= aligned_bytes;
> > +        offset += aligned_bytes;
> > +    }
> > +
> > +    assert((offset & (align - 1)) == 0);
> > +    if (bytes) {
> > +        assert(align == tail_padding_bytes + bytes);
> > +        /* RMW the unaligned part after tail. */
> > +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL);
> > +        ret = bdrv_aligned_preadv(bs, &req, offset, align,
> > +                                  align, &local_qiov, 0);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> > +
> > +        memset(buf, 0, bytes);
> > +        printf("tail part %ld %d\n", offset, bytes);
> 
> Please drop the debug printf.
> 
> > +        ret = bdrv_aligned_pwritev(bs, &req, offset, align,
> > +                                   &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE);
> > +    }
> > +fail:
> > +    tracked_request_end(&req);
> > +    if (buf) {
> > +        qemu_vfree(buf);
> > +    }
> 
> if (buf) is unnecessary.  qemu_vfree(NULL) is a nop.

OK, will fix them.

Thanks,
Fam

[Qemu-devel] [PATCH v6 3/3] qemu-iotests: Test unaligned sub-block zero write

[Fam Zheng]

Test zero write in byte range 512~1024 for 4k alignment.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/033     | 13 +++++++++++++
 tests/qemu-iotests/033.out | 30 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index 4008f10..a61d8ce 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -78,6 +78,19 @@ for align in 512 4k; do
 	echo
 	echo "== verifying patterns (2) =="
 	do_test $align "read -P 0x0 0x400 0x20000" "$TEST_IMG" | _filter_qemu_io
+
+	echo
+	echo "== rewriting unaligned zeroes =="
+	do_test $align "write -P 0xb 0x0 0x1000" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "write -z 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
+
+	echo
+	echo "== verifying patterns (3) =="
+	do_test $align "read -P 0xb 0x0 0x200" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "read -P 0x0 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "read -P 0xb 0x400 0xc00" "$TEST_IMG" | _filter_qemu_io
+
+	echo
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/033.out b/tests/qemu-iotests/033.out
index 305949f..c3d18aa 100644
--- a/tests/qemu-iotests/033.out
+++ b/tests/qemu-iotests/033.out
@@ -27,6 +27,21 @@ wrote 65536/65536 bytes at offset 65536
 read 131072/131072 bytes at offset 1024
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+== rewriting unaligned zeroes ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (3) ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 3072/3072 bytes at offset 1024
+3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
 == preparing image ==
 wrote 1024/1024 bytes at offset 512
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -52,4 +67,19 @@ wrote 65536/65536 bytes at offset 65536
 == verifying patterns (2) ==
 read 131072/131072 bytes at offset 1024
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting unaligned zeroes ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (3) ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 3072/3072 bytes at offset 1024
+3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 *** done
-- 
2.1.0

Re: [Qemu-devel] [PATCH v6 3/3] qemu-iotests: Test unaligned sub-block zero write

[Kevin Wolf]

Am 12.05.2015 um 08:09 hat Fam Zheng geschrieben:
> Test zero write in byte range 512~1024 for 4k alignment.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>