From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41577) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsOp3-0007Tc-Oj for qemu-devel@nongnu.org; Wed, 13 May 2015 01:03:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsOp0-0001pt-Ix for qemu-devel@nongnu.org; Wed, 13 May 2015 01:03:09 -0400 Date: Wed, 13 May 2015 13:03:01 +0800 From: Fam Zheng Message-ID: <20150513050237.GA22098@ad.nay.redhat.com> References: <1431410972-13087-1-git-send-email-famz@redhat.com> <1431410972-13087-3-git-send-email-famz@redhat.com> <20150512115212.GD3696@noname.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150512115212.GD3696@noname.str.redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, Stefan Hajnoczi , qemu-stable@nongnu.org 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 > > --- > > 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