On 24/04/2015 12:33, Fam Zheng wrote: > For zero write, qiov passed by callers (qemu-io "write -z" and > scsi-disk "write same") is NULL. > > 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 <f...@redhat.com> > --- > block.c | 78 > +++++++++++++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 61 insertions(+), 17 deletions(-) > > diff --git a/block.c b/block.c > index 0fe97de..cbd0708 100644 > --- a/block.c > +++ b/block.c > @@ -3403,6 +3403,8 @@ static int coroutine_fn > bdrv_co_do_pwritev(BlockDriverState *bs, > */ > tracked_request_begin(&req, bs, offset, bytes, true); > > + assert(qiov || flags & BDRV_REQ_ZERO_WRITE);
Perhaps as a follow-up you can add if ((flags & (BDRV_REQ_ZERO_WRITE|BDRV_REQ_MAY_UNMAP)) == BDRV_REQ_ZERO_WRITE|BDRV_REQ_MAY_UNMAP) { qiov = NULL; } so that the central area is always unmapped. You can have non-NULL qiov if the flags were added because of detect-zeroes=unmap. But in any case that would be a separate change. > if (offset & (align - 1)) { > QEMUIOVector head_qiov; > struct iovec head_iov; > @@ -3425,13 +3427,37 @@ static int coroutine_fn > bdrv_co_do_pwritev(BlockDriverState *bs, > } > BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); > > - qemu_iovec_init(&local_qiov, qiov->niov + 2); > - qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); > - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > - use_local_qiov = true; > + if (qiov) { > + qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 2 : 1); > + qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); > + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + use_local_qiov = true; > + bytes += offset & (align - 1); > + offset = offset & ~(align - 1); > + } else { > + memset(head_buf + (offset & (align - 1)), 0, > + align - (offset & (align - 1))); > + ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), > align, > + &head_qiov, 0); > + if (ret < 0) { > + goto fail; > + } > + bytes -= align - (offset & (align - 1)); > + offset = ROUND_UP(offset, align); > + } > + } > > - bytes += offset & (align - 1); > - offset = offset & ~(align - 1); > + if (!qiov) { > + uint64_t aligned_bytes = bytes & ~(align - 1); > + > + assert((offset & (align - 1)) == 0); > + ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes, > + NULL, flags); > + if (ret < 0) { > + goto fail; > + } > + bytes -= aligned_bytes; > + offset += aligned_bytes; > } > > if ((offset + bytes) & (align - 1)) { > @@ -3459,21 +3485,39 @@ static int coroutine_fn > bdrv_co_do_pwritev(BlockDriverState *bs, > } > BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); > > - if (!use_local_qiov) { > - qemu_iovec_init(&local_qiov, qiov->niov + 1); > - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > - use_local_qiov = true; > + if (qiov) { > + if (!use_local_qiov) { > + qemu_iovec_init(&local_qiov, qiov->niov + 1); > + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + use_local_qiov = true; > + } > + > + tail_bytes = (offset + bytes) & (align - 1); > + qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, > + align - tail_bytes); > + > + bytes = ROUND_UP(bytes, align); > + } else { > + assert((offset & (align - 1)) == 0); > + assert(bytes < align); > + > + memset(tail_buf, 0, bytes & (align - 1)); > + ret = bdrv_aligned_pwritev(bs, &req, offset, align, > + &tail_qiov, 0); > + if (ret < 0) { > + goto fail; > + } > + offset += align; > + bytes = 0; > } > > - tail_bytes = (offset + bytes) & (align - 1); > - qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - > tail_bytes); > - > - bytes = ROUND_UP(bytes, align); > } > > - ret = bdrv_aligned_pwritev(bs, &req, offset, bytes, > - use_local_qiov ? &local_qiov : qiov, > - flags); > + if (bytes) { > + ret = bdrv_aligned_pwritev(bs, &req, offset, bytes, > + use_local_qiov ? &local_qiov : qiov, > + flags); > + } > > fail: > tracked_request_end(&req); > Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>