From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60990) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXA2f-0000wP-8N for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:24:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VXA2W-0007x0-Ky for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:24:37 -0400 Received: from mail-ea0-x232.google.com ([2a00:1450:4013:c01::232]:58943) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXA2W-0007wt-CF for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:24:28 -0400 Received: by mail-ea0-f178.google.com with SMTP id a15so2009832eae.9 for ; Fri, 18 Oct 2013 06:24:27 -0700 (PDT) Date: Fri, 18 Oct 2013 15:24:25 +0200 From: Stefan Hajnoczi Message-ID: <20131018132425.GA20105@stefanha-thinkpad.redhat.com> References: <1381233491-17019-1-git-send-email-pl@kamp.de> <1381233491-17019-15-git-send-email-pl@kamp.de> <20131018123812.GB19041@stefanha-thinkpad.redhat.com> <52612E47.9050004@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52612E47.9050004@redhat.com> Subject: Re: [Qemu-devel] [PATCHv4 14/17] block/get_block_status: fix BDRV_BLOCK_ZERO for unallocated blocks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, anthony@codemonkey.ws, Peter Lieven , qemu-devel@nongnu.org, ronniesahlberg@gmail.com, stefanha@redhat.com On Fri, Oct 18, 2013 at 02:49:11PM +0200, Paolo Bonzini wrote: > Il 18/10/2013 14:38, Stefan Hajnoczi ha scritto: > > On Tue, Oct 08, 2013 at 01:58:08PM +0200, Peter Lieven wrote: > >> this patch does 2 things: > >> a) only do additional call outs if BDRV_BLOCK_ZERO is not already set. > >> b) use the newly introduced bdrv_has_discard_zeroes() to return the > >> zero state of an unallocated block. the used callout to > >> bdrv_has_zero_init() is only valid right after bdrv_create. > >> > >> Reviewed-by: Eric Blake > >> Signed-off-by: Peter Lieven > >> --- > >> block.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/block.c b/block.c > >> index fc931e3..1be4418 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -3247,8 +3247,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > >> return ret; > >> } > >> > >> - if (!(ret & BDRV_BLOCK_DATA)) { > >> - if (bdrv_has_zero_init(bs)) { > >> + if (!(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO)) { > >> + if (bdrv_has_discard_zeroes(bs)) { > > > > I'm a little unclear about the semantics of bdrv_has_discard_zeroes(). > > Originally I thought it just meant any blocks discarded will read back > > as zeroes. But here it implies that any unallocated block reads > > back as zeroes too? > > > > In other words, this patch assumes unallocated blocks behave the same as > > discarded blocks wrt to zeroes. > > Note that earlier patches introduce both bdrv_has_discard_zeroes and > bdrv_has_discard_write_zeroes. There is no documentation, but the iscsi > implementation let us understand the meaning: There are doc comments but they differ from what you've posted: + /* + * Returns true if discarded blocks read back as zeroes. + */ + bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs); > +static bool iscsi_has_discard_zeroes(BlockDriverState *bs) > +{ > + IscsiLun *iscsilun = bs->opaque; > + return !!iscsilun->lbprz; > +} > > That is, unallocated block reads back as zeroes Okay, your semantics make sense. With them the later patches are correct. Peter: Please update the doc comments although and consider Paolo's comments about block driver info. Stefan