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 <ebl...@redhat.com>
Signed-off-by: Peter Lieven <p...@kamp.de>
---
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
