From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axFwi-0002zT-6Y for qemu-devel@nongnu.org; Mon, 02 May 2016 11:39:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axFwK-0002EE-5U for qemu-devel@nongnu.org; Mon, 02 May 2016 11:39:34 -0400 Date: Mon, 2 May 2016 17:35:29 +0200 From: Kevin Wolf Message-ID: <20160502153529.GE4882@noname.redhat.com> References: <1462052936-16933-1-git-send-email-eblake@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462052936-16933-1-git-send-email-eblake@redhat.com> Subject: Re: [Qemu-devel] [PATCH] block: Don't lose FUA flag during ZERO_WRITE fallback List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, qemu-stable@nongnu.org, Stefan Hajnoczi , Fam Zheng , Max Reitz , "open list:Block I/O path" Am 30.04.2016 um 23:48 hat Eric Blake geschrieben: > NBD has situations where it can support FUA but not ZERO_WRITE; > when that happens, the generic block layer fallback was losing > the FUA flag. The problem of losing flags unrelated to > ZERO_WRITE has been latent in bdrv_co_do_write_zeroes() since > aa7bfbff, but back then, it did not matter because there was no > FUA flag. But ever since 93f5e6d8 added bdrv_co_writev_flags(), > the loss of flags can impact correctness. > > Compare to commit 9eeb6dd, which got it right in > bdrv_co_do_zero_pwritev(). > > Symptoms: I tested with qemu-io with default writethrough cache > (which is supposed to use FUA semantics on every write), and > targetted an NBD client connected to a server that intentionally > did not advertise NBD_FLAG_SEND_FUA. When doing 'write 0 512', > the NBD client sent two operations (NBD_CMD_WRITE then > NBD_CMD_FLUSH) to get the fallback FUA semantics; but when doing > 'write -z 0 512', the NBD client sent only NBD_CMD_WRITE; the > missing flush meant that an ill-timed disconnect could leave > the zeroes unflushed. > > CC: qemu-stable@nongnu.org > Signed-off-by: Eric Blake > --- > > As written, this patch applies to 2.7 on top of Kevin's block-next > branch. Since it's (probably) too late for 2.6, we'll need to > backport it to there, but the backport will have to use > bdrv_co_writev_flags since 2.6 lacks bdrv_driver_pwritev(). > > block/io.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/block/io.c b/block/io.c > index 0db1146..bd46e47 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1213,7 +1213,8 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, > qemu_iovec_init_external(&qiov, &iov, 1); > > ret = bdrv_driver_pwritev(bs, sector_num * BDRV_SECTOR_SIZE, > - num * BDRV_SECTOR_SIZE, &qiov, 0); > + num * BDRV_SECTOR_SIZE, &qiov, > + flags & ~BDRV_REQ_ZERO_WRITE); This is a good change, but it's only in the fallback code. If we succeed here: if (drv->bdrv_co_write_zeroes) { ret = drv->bdrv_co_write_zeroes(bs, sector_num, num, flags); } then we still don't get the necessary flush unless the driver's .bdrv_co_write_zeroes() implementation explicitly handles FUA. As far as I know, we don't have any driver that implements FUA there. Kevin