| Message ID | 1408215258-12545-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/16/2014 12:54 PM, Max Reitz wrote: > When falling through to the underlying file in > bdrv_co_get_block_status(), do not let the number of sectors for which > information could be obtained be overwritten. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote : > When falling through to the underlying file in > bdrv_co_get_block_status(), do not let the number of sectors for which > information could be obtained be overwritten. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 3e252a2..c922664 100644 > --- a/block.c > +++ b/block.c > @@ -3991,9 +3991,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > if (bs->file && > (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && > (ret & BDRV_BLOCK_OFFSET_VALID)) { > + int backing_pnum; > + > ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, > - *pnum, pnum); > - if (ret2 >= 0) { > + *pnum, &backing_pnum); > + if (ret2 >= 0 && backing_pnum >= *pnum) { About backing_pnum >= *pnum. The documentation of bdrv_co_get_block_status says: * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes * beyond the end of the disk image it will be clamped. */ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum. This means that backing_pnum > *pnum will never happen. I think either this test is wrong or the doc is wrong. Best regards Benoît > /* Ignore errors. This is just providing extra information, it > * is useful but not necessary. > */ > -- > 2.0.4 > >
Am 10.10.2014 um 13:50 schrieb Benoît Canet: > The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote : >> When falling through to the underlying file in >> bdrv_co_get_block_status(), do not let the number of sectors for which >> information could be obtained be overwritten. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/block.c b/block.c >> index 3e252a2..c922664 100644 >> --- a/block.c >> +++ b/block.c >> @@ -3991,9 +3991,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, >> if (bs->file && >> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && >> (ret & BDRV_BLOCK_OFFSET_VALID)) { >> + int backing_pnum; >> + >> ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, >> - *pnum, pnum); >> - if (ret2 >= 0) { >> + *pnum, &backing_pnum); >> + if (ret2 >= 0 && backing_pnum >= *pnum) { > About backing_pnum >= *pnum. > > The documentation of bdrv_co_get_block_status says: > > * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes > * beyond the end of the disk image it will be clamped. > */ > static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > int64_t sector_num, > int nb_sectors, int *pnum) > > So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum. > > This means that backing_pnum > *pnum will never happen. > > I think either this test is wrong or the doc is wrong. Thank you for confusing me, I had to think quite a while about this. *g* The condition is not for error checking. If it was, it would be the wrong order (the condition should be true on success, that's why it's "ret2 >= 0" and not "ret2 < 0", so it should then be "backing_pnum <= *pnum"). So what this is testing is whether all sectors in the underlying file in the queried range are read as zero. But if "backing_pnum < *pnum" that is not the case, some clusters are not zero. So we may not set the zero flag if backing_pnum < *pnum; or as it reads in the code, we may only set it if backing_pnum >= *pnum. This is not about whether *pnum > backing_pnum, but more about whether backing_pnum == *pnum (but >= would be fine, too, if bdrv_co_get_block_status() supported it, so that's why I wrote it that way). However, I'm starting to think about whether it would be better, for the backing_pnum < *pnum case, not to not set the zero flag, but rather simply set *pnum = backing_pnum. And this in turn would be pretty equivalent to just omitting this patch, because: If we get to this point where we query the underlying file and it returns a certain number of sectors is zero; then we therefore want to set *pnum = backing_pnum (both if backing_pnum < *pnum and if backing_pnum == *pnum; backing_pnum > *pnum cannot happen, as you pointed out). On the other hand, if the sectors are not reported to be zero, but backing_pnum < *pnum, we want to shorten *pnum accordingly as well because this may indicate that after another backing_pnum sectors, we arrive at a hole in the file. There is only one point I can imagine where it makes sense not to let backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status() reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond the EOF. I think this might actually happen with qcow2, if one cluster simply lies beyond the EOF (which is perfectly valid). So I conclude that this patch has its use after all but needs to be modified so that backing_pnum always overwrites *pnum; except for when backing_pnum is zero (which should only happen at or after the EOF) in which case the zero flag should be set and *pnum should be left as it was. And now in all honesty: Thanks for confusing me, I guess I can think better when I'm confused. :-) Max > Best regards > > Benoît > > >> /* Ignore errors. This is just providing extra information, it >> * is useful but not necessary. >> */ >> -- >> 2.0.4 >> >>
The Saturday 11 Oct 2014 à 11:44:20 (+0200), Max Reitz wrote : > Am 10.10.2014 um 13:50 schrieb Benoît Canet: > >The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote : > >>When falling through to the underlying file in > >>bdrv_co_get_block_status(), do not let the number of sectors for which > >>information could be obtained be overwritten. > >> > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> block.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >>diff --git a/block.c b/block.c > >>index 3e252a2..c922664 100644 > >>--- a/block.c > >>+++ b/block.c > >>@@ -3991,9 +3991,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > >> if (bs->file && > >> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && > >> (ret & BDRV_BLOCK_OFFSET_VALID)) { > >>+ int backing_pnum; > >>+ > >> ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, > >>- *pnum, pnum); > >>- if (ret2 >= 0) { > >>+ *pnum, &backing_pnum); > >>+ if (ret2 >= 0 && backing_pnum >= *pnum) { > >About backing_pnum >= *pnum. > > > >The documentation of bdrv_co_get_block_status says: > > > > * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes > > * beyond the end of the disk image it will be clamped. > > */ > >static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > > int64_t sector_num, > > int nb_sectors, int *pnum) > > > >So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum. > > > >This means that backing_pnum > *pnum will never happen. > > > >I think either this test is wrong or the doc is wrong. > > Thank you for confusing me, I had to think quite a while about this. *g* > > The condition is not for error checking. If it was, it would be the wrong > order (the condition should be true on success, that's why it's "ret2 >= 0" > and not "ret2 < 0", so it should then be "backing_pnum <= *pnum"). So what > this is testing is whether all sectors in the underlying file in the queried > range are read as zero. But if "backing_pnum < *pnum" that is not the case, > some clusters are not zero. So we may not set the zero flag if backing_pnum > < *pnum; or as it reads in the code, we may only set it if backing_pnum >= > *pnum. This is not about whether *pnum > backing_pnum, but more about > whether backing_pnum == *pnum (but >= would be fine, too, if > bdrv_co_get_block_status() supported it, so that's why I wrote it that way). > > However, I'm starting to think about whether it would be better, for the > backing_pnum < *pnum case, not to not set the zero flag, but rather simply > set *pnum = backing_pnum. And this in turn would be pretty equivalent to > just omitting this patch, because: > > If we get to this point where we query the underlying file and it returns a > certain number of sectors is zero; then we therefore want to set *pnum = > backing_pnum (both if backing_pnum < *pnum and if backing_pnum == *pnum; > backing_pnum > *pnum cannot happen, as you pointed out). On the other hand, > if the sectors are not reported to be zero, but backing_pnum < *pnum, we > want to shorten *pnum accordingly as well because this may indicate that > after another backing_pnum sectors, we arrive at a hole in the file. > > There is only one point I can imagine where it makes sense not to let > backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status() > reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond the > EOF. I think this might actually happen with qcow2, if one cluster simply > lies beyond the EOF (which is perfectly valid). So I conclude that this > patch has its use after all but needs to be modified so that backing_pnum > always overwrites *pnum; except for when backing_pnum is zero (which should > only happen at or after the EOF) in which case the zero flag should be set > and *pnum should be left as it was. > > And now in all honesty: Thanks for confusing me, I guess I can think better > when I'm confused. :-) > You better have killer english skills to sumarize this in a nice commit message :) I'll read the next version. Best regards Benoît > Max > > >Best regards > > > >Benoît > > > > > >> /* Ignore errors. This is just providing extra information, it > >> * is useful but not necessary. > >> */ > >>-- > >>2.0.4 > >> > >> > >
diff --git a/block.c b/block.c index 3e252a2..c922664 100644 --- a/block.c +++ b/block.c @@ -3991,9 +3991,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, if (bs->file && (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID)) { + int backing_pnum; + ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, - *pnum, pnum); - if (ret2 >= 0) { + *pnum, &backing_pnum); + if (ret2 >= 0 && backing_pnum >= *pnum) { /* Ignore errors. This is just providing extra information, it * is useful but not necessary. */
When falling through to the underlying file in bdrv_co_get_block_status(), do not let the number of sectors for which information could be obtained be overwritten. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)