From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41542) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXAUM-0008Ch-P2 for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:53:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VXAUG-0001d0-VN for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:53:14 -0400 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:36772 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VXAUG-0001cn-JS for qemu-devel@nongnu.org; Fri, 18 Oct 2013 09:53:08 -0400 Message-ID: <52613D38.7000507@kamp.de> Date: Fri, 18 Oct 2013 15:52:56 +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> <20131018132425.GA20105@stefanha-thinkpad.redhat.com> In-Reply-To: <20131018132425.GA20105@stefanha-thinkpad.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: Stefan Hajnoczi , Paolo Bonzini Cc: kwolf@redhat.com, anthony@codemonkey.ws, ronniesahlberg@gmail.com, qemu-devel@nongnu.org, stefanha@redhat.com On 18.10.2013 15:24, Stefan Hajnoczi wrote: > 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. Ok, we have 2 features, but we need better names for them. a) unallocated blocks read back as zeroes. I would suggest to rename bdrv_has_discard_zeroes() to bdrv_unallocated_blocks_return_zero() and update the comment to: /* * Returns true if unallocated blocks read back as zeroes. */ b) the driver has an efficient way of speeding up bdrv_write_zeroes by unmapping blocks. for iscsi this is done through WRITESAME16 with the UNMAP flag. for other drivers this could be a similar approach as long as it is guaranteed that its not writing actual zeroes to disk and that zeroes are returned in any case. I would suggest to rename bdrv_has_discard_write_zeroes to bdrv_can_write_zeroes_by_unmap(). /* * Returns true if the driver can optimize writing zeroes by unmapping * without actually writing real zeroes to disk. However, it must be guaranteed * that all blocks read back as zeroes afterwards. It is additionally required that * the block device is opened with BDRV_O_UNMAP and the that the * bdrv_write_zeroes request carries the BDRV_REQ_MAY_UNMAP flag for * this to work. */ Regarding putting this info into the BDI I am fine with that, but I would keep the wrapper functions. On the other hand, bdrv_has_zero_init is also not in the BDI... I had it in the BDI and got the request to move it to separate functions. To finish this series I need a definitive decision where to put it. Peter