| Message ID | 1381233491-17019-15-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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 <eblake@redhat.com> > Signed-off-by: Peter Lieven <pl@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. Stefan
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 <eblake@redhat.com> >> Signed-off-by: Peter Lieven <pl@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: +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 - 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. Paolo
On 18.10.2013 14:38, Stefan Hajnoczi wrote: > 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 <eblake@redhat.com> >> Signed-off-by: Peter Lieven <pl@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. > > Stefan bdrv_has_discard_zeroes() indicates if unallocated blocks read back as zeroes. as a discard may silently fail this is no guarantee that a discard will result in a properly zeroed area. bdrv_has_discard_write_zeroes() indicates that the driver is able to honour the BDRV_REQ_MAY_UNMAP flag on bdrv_write_zeroes(). see documentation in block_int.h /* * Returns true if discarded blocks read back as zeroes. */ bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs); /* * Returns true if the driver can optimize writing zeroes by discarding * sectors. It is additionally required that the block device is * opened with BDRV_O_UNMAP and the that write zeroes request carries * the BDRV_REQ_MAY_UNMAP flag for this to work. */ bool (*bdrv_has_discard_write_zeroes)(BlockDriverState *bs); Peter
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 <eblake@redhat.com> > >> Signed-off-by: Peter Lieven <pl@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. Stefan
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 <eblake@redhat.com> >>> Signed-off-by: Peter Lieven <pl@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: > > > +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
Il 18/10/2013 15:26, Peter Lieven ha scritto: >> >> >> - 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. Yes. I'm fine with the current names actually, just thinking loudly. >> - 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. Yes, but I think he referred to block.c functions not BlockDriver functions. Paolo
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 <eblake@redhat.com> >>>> Signed-off-by: Peter Lieven <pl@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
Il 18/10/2013 15:52, Peter Lieven ha scritto: > > 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. Moving bdrv_has_zero_init to BDI is also a good idea, but it can be done later. Paolo
> Am 18.10.2013 um 15:50 schrieb Paolo Bonzini <pbonzini@redhat.com>: > > Il 18/10/2013 15:26, Peter Lieven ha scritto: >>> >>> >>> - 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. > > Yes. I'm fine with the current names actually, just thinking loudly. > >>> - 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. > > Yes, but I think he referred to block.c functions not BlockDriver functions. Ok, if Stefan and Kevin agree i will change it once more. I Would also like some Feedback on the new names for the functions and changed description. I can send a respin next week then. Peter > > Paolo
On Fri, Oct 18, 2013 at 09:10:43PM +0200, Peter Lieven wrote: > > > > Am 18.10.2013 um 15:50 schrieb Paolo Bonzini <pbonzini@redhat.com>: > > > > Il 18/10/2013 15:26, Peter Lieven ha scritto: > >>> > >>> > >>> - 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. > > > > Yes. I'm fine with the current names actually, just thinking loudly. > > > >>> - 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. > > > > Yes, but I think he referred to block.c functions not BlockDriver functions. > > Ok, if Stefan and Kevin agree i will change it once more. I Would also like some Feedback on the new names for the functions and changed description. I can send a respin next week then. (Catching up with old mails) Fine here. Stefan
Hi Stefan, please have a Look at v7 of this series. Hopefully the final one. Thx, > Am 30.10.2013 um 09:28 schrieb Stefan Hajnoczi <stefanha@redhat.com>: > >> On Fri, Oct 18, 2013 at 09:10:43PM +0200, Peter Lieven wrote: >> >> >>> Am 18.10.2013 um 15:50 schrieb Paolo Bonzini <pbonzini@redhat.com>: >>> >>> Il 18/10/2013 15:26, Peter Lieven ha scritto: >>>>> >>>>> >>>>> - 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. >>> >>> Yes. I'm fine with the current names actually, just thinking loudly. >>> >>>>> - 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. >>> >>> Yes, but I think he referred to block.c functions not BlockDriver functions. >> >> Ok, if Stefan and Kevin agree i will change it once more. I Would also like some Feedback on the new names for the functions and changed description. I can send a respin next week then. > > (Catching up with old mails) > > Fine here. > > Stefan
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)) { ret |= BDRV_BLOCK_ZERO; } else if (bs->backing_hd) { BlockDriverState *bs2 = bs->backing_hd;