From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33312) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXA4p-0002m2-42 for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:26:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VXA4j-0000RX-HY for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:26:51 -0400 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:55113 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXA4j-0000RT-5S for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:26:45 -0400 Message-ID: <52613709.6040604@kamp.de> Date: Fri, 18 Oct 2013 15:26:33 +0200 From: Peter Lieven MIME-Version: 1.0 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> In-Reply-To: <52612E47.9050004@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 , Stefan Hajnoczi Cc: kwolf@redhat.com, anthony@codemonkey.ws, ronniesahlberg@gmail.com, qemu-devel@nongnu.org, stefanha@redhat.com On 18.10.2013 14:49, 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: > > > +static bool iscsi_has_discard_zeroes(BlockDriverState *bs) > +{ > + IscsiLun *iscsilun = bs->opaque; > + return !!iscsilun->lbprz; > +} > > That is, unallocated block reads back as zeroes > > +static bool iscsi_has_discard_write_zeroes(BlockDriverState *bs) > +{ > + IscsiLun *iscsilun = bs->opaque; > + return iscsilun->lbprz && iscsilun->lbp.lbpws; > +} > > That is, discarded blocks read back zeroes. This is because: > > - UNMAP is not guaranteeing that blocks are discarded, and thus not > really guaranteeing anything on its contents. > > - but WRITE SAME is guaranteeing that blocks you "write same" read with > the payload of the command. This means that in practice for !LBPRZ the > WRITE SAME command will not discard (unless the firmware has bugs). > > - so for !LBPRZ you must use UNMAP, but for LBPRZ you can use WRITE SAME > and guarantee that the block reads as zero > > > Perhaps better names could be > > - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes This would conform to the linux ioctl BLKDISCARDZEROES. However, we need the write_zeroes operation for a guarantee that zeroes are return. > > - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes > > But I'm not sure why we have different BlockDriver APIs. I'd rather put > the new flags in BlockDriverInfo, and make the new functions simple > wrappers around bdrv_get_info. I think I proposed that before, maybe I > wasn't clear or I was misunderstood. I think Kevin wanted to have special functions for this. Peter