| Message ID | 1473957290-13382-2-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 09/15 19:34, Denis V. Lunev wrote: > They should work very similar, covering same areas if backing store is > shorter than the image. This change is necessary for the followup patch > switching to bdrv_get_block_status_above() in mirror to avoid assert > in check_block. > > This change should be made very carefully. Let us assume that we have > top image and 2 backing stores L0->L1->L2. Stupid question: which one is top and which are backing? > L0: -------------- > L1: ------- > L2: -------======= > The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED > and we should return it as filled in L0 image with properly calculated > *pnum value. What '-', '=' and ' ' represent aren't immediately clear to me, could you put a legend in the message too? Something like: '-': allocated '=': unallocated ' ': beyong EOF > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > --- > block/io.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 420944d..067d465 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, > BlockDriverState **file) > { > BlockDriverState *p; > - int64_t ret = 0; > + int64_t ret = 0, res = nb_sectors; > > assert(bs != base); > for (p = bs; p != base; p = backing_bs(p)) { > - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); > - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { > - break; > + int sc; > + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); > + if (ret < 0) { > + return ret; > + } else if (ret & BDRV_BLOCK_ALLOCATED) { > + *pnum = sc; > + return ret; > + } > + > + if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) { > + res = sc; > } > + > /* [sector_num, pnum] unallocated on this layer, which could be only > * the first part of [sector_num, nb_sectors]. */ > - nb_sectors = MIN(nb_sectors, *pnum); > + nb_sectors = MIN(nb_sectors, sc); > + > + if (nb_sectors == 0) { > + break; > + } > } > + > + *pnum = res; > return ret; > } > > -- > 2.7.4 > >
On 09/19/2016 04:21 AM, Fam Zheng wrote: > On Thu, 09/15 19:34, Denis V. Lunev wrote: >> They should work very similar, covering same areas if backing store is >> shorter than the image. This change is necessary for the followup patch >> switching to bdrv_get_block_status_above() in mirror to avoid assert >> in check_block. >> >> This change should be made very carefully. Let us assume that we have >> top image and 2 backing stores L0->L1->L2. > Stupid question: which one is top and which are backing? L0 is top, L2 is at bottom. >> L0: -------------- >> L1: ------- >> L2: -------======= >> The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED >> and we should return it as filled in L0 image with properly calculated >> *pnum value. > What '-', '=' and ' ' represent aren't immediately clear to me, could you put a > legend in the message too? Something like: > > '-': allocated > '=': unallocated > ' ': beyong EOF ok. here '-' in unallocated '=' is allocated. virtual size of L1 image is shorter that L2 and L0, thus ' ' is beyond EOF. Thank you, will rewrite today. Den >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> CC: Fam Zheng <famz@redhat.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Max Reitz <mreitz@redhat.com> >> CC: Jeff Cody <jcody@redhat.com> >> --- >> block/io.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/block/io.c b/block/io.c >> index 420944d..067d465 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, >> BlockDriverState **file) >> { >> BlockDriverState *p; >> - int64_t ret = 0; >> + int64_t ret = 0, res = nb_sectors; >> >> assert(bs != base); >> for (p = bs; p != base; p = backing_bs(p)) { >> - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); >> - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { >> - break; >> + int sc; >> + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); >> + if (ret < 0) { >> + return ret; >> + } else if (ret & BDRV_BLOCK_ALLOCATED) { >> + *pnum = sc; >> + return ret; >> + } >> + >> + if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) { >> + res = sc; >> } >> + >> /* [sector_num, pnum] unallocated on this layer, which could be only >> * the first part of [sector_num, nb_sectors]. */ >> - nb_sectors = MIN(nb_sectors, *pnum); >> + nb_sectors = MIN(nb_sectors, sc); >> + >> + if (nb_sectors == 0) { >> + break; >> + } >> } >> + >> + *pnum = res; >> return ret; >> } >> >> -- >> 2.7.4 >> >>
On 09/18/2016 11:37 PM, Denis V. Lunev wrote: > On 09/19/2016 04:21 AM, Fam Zheng wrote: >> On Thu, 09/15 19:34, Denis V. Lunev wrote: >>> They should work very similar, covering same areas if backing store is >>> shorter than the image. This change is necessary for the followup patch >>> switching to bdrv_get_block_status_above() in mirror to avoid assert >>> in check_block. >>> >>> This change should be made very carefully. Let us assume that we have >>> top image and 2 backing stores L0->L1->L2. >> Stupid question: which one is top and which are backing? > L0 is top, L2 is at bottom. I typically write this as: L2 <- L1 <- L0 (read "L2 backs L1, which in turn backs L0") with the active on the right. So I understand the confusion in Fam's question where you were using the opposite direction.
On 2016-09-15 at 18:34, Denis V. Lunev wrote: > They should work very similar, covering same areas if backing store is > shorter than the image. This change is necessary for the followup patch > switching to bdrv_get_block_status_above() in mirror to avoid assert > in check_block. > > This change should be made very carefully. Let us assume that we have > top image and 2 backing stores L0->L1->L2. > L0: -------------- > L1: ------- > L2: -------======= > The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED > and we should return it as filled in L0 image with properly calculated > *pnum value. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > --- > block/io.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 420944d..067d465 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, > BlockDriverState **file) > { > BlockDriverState *p; > - int64_t ret = 0; > + int64_t ret = 0, res = nb_sectors; It's not wrong to make res an int64_t, but an int is sufficient. > > assert(bs != base); > for (p = bs; p != base; p = backing_bs(p)) { > - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); > - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { > - break; > + int sc; > + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); > + if (ret < 0) { > + return ret; > + } else if (ret & BDRV_BLOCK_ALLOCATED) { > + *pnum = sc; > + return ret; > + } > + > + if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) { > + res = sc; This definitely requires some comments because it took me a long time to figure out why we need "res" to be a separate variable from "nb_sectors" and why this condition is like it is. So what I think this does is: Basically, we want to return our final nb_sectors in *pnum. But we can have the constellation you noted in the commit message: A short intermediate layer, and the bottom layer has some data allocated beyond the end of that intermediate layer. Now, when we pass through that intermediate layer, we need to shorten nb_sectors so that we don't query anything beyond the end of that intermediate layer because it doesn't matter anyway. But we also want to remember that all of this area appears as unallocated to the top layer, so therefore we have to keep a second variable ("res") which retains this information. Therefore, nb_sectors is always exactly the range we want to query, and "res" is the range we know to appear unallocated. This condition here tries to adjust "res" so that it conforms to that specification. However, I'm not quite sure it actually does that. Let's take the case from your commit message: L0: -------------- L1: ------- L2: -------======= Let's say we invoke this function in the range [0, 14]. After passing through L0, res is 14 and nb_sectors is 14. After L1, res is still 14, but nb_sectors is 7. So far, so good. But when passing through L2, "sc" will be 7 (and it will actually always be 7, regardless of what comes past sector 7, because nb_sectors is 7). Since L2 is larger than just 7 sectors, we will now reduce res to 7 as well (because sector_num + sc (= 0 + 7 = 7) < p->total_sectors (= 14)). So therefore, we will set *pnum to 7. That doesn't seem too bad to me, but we could have achieved the same result by just setting *pnum to nb_sectors and not having to track the separate "res" variable. Thus, I'm not quite sure what the point of this is. "res" will only be longer than "nb_sectors" as long as the layers get shorter or stay the same length when going downwards. As soon as one layer is longer than the one above it, "res" will probably be truncated to "sc" (which is going to be the same value as "nb_sectors", unless bdrv_co_get_block_status() returns a *pnum > nb_sectors). I'm not sure whether I'm missing something here, though. > } > + > /* [sector_num, pnum] unallocated on this layer, which could be only The "pnum" here should be changed to "sc". > * the first part of [sector_num, nb_sectors]. */ > - nb_sectors = MIN(nb_sectors, *pnum); > + nb_sectors = MIN(nb_sectors, sc); > + > + if (nb_sectors == 0) { > + break; While I can see that in this case ret would be 0, I think it wouldn't hurt to add an explicit "ret = 0;" here, too. Max > + } > } > + > + *pnum = res; > return ret; > } > >
On Tue, Sep 20, 2016 at 01:18:12AM +0200, Max Reitz wrote: > On 2016-09-15 at 18:34, Denis V. Lunev wrote: > >They should work very similar, covering same areas if backing store is > >shorter than the image. This change is necessary for the followup patch > >switching to bdrv_get_block_status_above() in mirror to avoid assert > >in check_block. > > > >This change should be made very carefully. Let us assume that we have > >top image and 2 backing stores L0->L1->L2. > > L0: -------------- > > L1: ------- > > L2: -------======= > >The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED > >and we should return it as filled in L0 image with properly calculated > >*pnum value. > > > >Signed-off-by: Denis V. Lunev <den@openvz.org> > >CC: Stefan Hajnoczi <stefanha@redhat.com> > >CC: Fam Zheng <famz@redhat.com> > >CC: Kevin Wolf <kwolf@redhat.com> > >CC: Max Reitz <mreitz@redhat.com> > >CC: Jeff Cody <jcody@redhat.com> > >--- > > block/io.c | 25 ++++++++++++++++++++----- > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > >diff --git a/block/io.c b/block/io.c > >index 420944d..067d465 100644 > >--- a/block/io.c > >+++ b/block/io.c > >@@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, > > BlockDriverState **file) > > { > > BlockDriverState *p; > >- int64_t ret = 0; > >+ int64_t ret = 0, res = nb_sectors; > > It's not wrong to make res an int64_t, but an int is sufficient. > > > > > assert(bs != base); > > for (p = bs; p != base; p = backing_bs(p)) { > >- ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); > >- if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { > >- break; > >+ int sc; > >+ ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); > >+ if (ret < 0) { > >+ return ret; > >+ } else if (ret & BDRV_BLOCK_ALLOCATED) { > >+ *pnum = sc; > >+ return ret; > >+ } > >+ > >+ if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) { > >+ res = sc; For what its worth, I think the code in bdrv_is_allocated_above() is a bit more readable, which does largely the same thing as this function now does with this patch. Some of it is due to with the nomenclature of 'res' and 'sc' here, neither of which are very intuitive. Part of it is out of this patch's control: the explicit variable convention of "top", "intermediate", and "base" in the while loop of bdrv_is_allocated_above() also aids readability. I think since the functionality is largely pulled from bdrv_is_allocated_above(), it would make sense to copy its syntax more directly. > > This definitely requires some comments because it took me a long time to > figure out why we need "res" to be a separate variable from "nb_sectors" and > why this condition is like it is. > > So what I think this does is: > > Basically, we want to return our final nb_sectors in *pnum. But we can have > the constellation you noted in the commit message: A short intermediate > layer, and the bottom layer has some data allocated beyond the end of that > intermediate layer. > > Now, when we pass through that intermediate layer, we need to shorten > nb_sectors so that we don't query anything beyond the end of that > intermediate layer because it doesn't matter anyway. > That's the issue with this patch; we don't need to shorten nb_sectors, precisely because it doesn't matter if it extends past the end of the current layer. [1] > But we also want to remember that all of this area appears as unallocated to > the top layer, so therefore we have to keep a second variable ("res") which > retains this information. > > Therefore, nb_sectors is always exactly the range we want to query, and > "res" is the range we know to appear unallocated. This condition here tries > to adjust "res" so that it conforms to that specification. > This changes makes this function similar to "bdrv_is_allocated_above()". It may be useful to refer back to commit 63ba17d, to see the rationale when that functioned got the similar functionality: block: Fix is_allocated_above with resized files In an image chain, if the base image is smaller than the current image, we need to make sure to use the current images count of unallocated blocks once we get to the end of the base image. Without this change the code will return 0 blocks when it gets to the end of the base image and mirror_run will fail its assertion. This sounds compatible with your above analysis. > However, I'm not quite sure it actually does that. Let's take the case from > your commit message: > > L0: -------------- > L1: ------- > L2: -------======= > > Let's say we invoke this function in the range [0, 14]. After passing > through L0, res is 14 and nb_sectors is 14. After L1, res is still 14, but > nb_sectors is 7. So far, so good. > > But when passing through L2, "sc" will be 7 (and it will actually always be > 7, regardless of what comes past sector 7, because nb_sectors is 7). Since > L2 is larger than just 7 sectors, we will now reduce res to 7 as well > (because sector_num + sc (= 0 + 7 = 7) < p->total_sectors (= 14)). > > So therefore, we will set *pnum to 7. That doesn't seem too bad to me, but > we could have achieved the same result by just setting *pnum to nb_sectors > and not having to track the separate "res" variable. > > > Thus, I'm not quite sure what the point of this is. "res" will only be > longer than "nb_sectors" as long as the layers get shorter or stay the same > length when going downwards. As soon as one layer is longer than the one > above it, "res" will probably be truncated to "sc" (which is going to be the > same value as "nb_sectors", unless bdrv_co_get_block_status() returns a > *pnum > nb_sectors). > > I'm not sure whether I'm missing something here, though. > There is a bug, below [1]: > > } > >+ > > /* [sector_num, pnum] unallocated on this layer, which could be only > > The "pnum" here should be changed to "sc". > > > * the first part of [sector_num, nb_sectors]. */ > >- nb_sectors = MIN(nb_sectors, *pnum); > >+ nb_sectors = MIN(nb_sectors, sc); [1] The above code should be deleted completely. There isn't a reason to set nb_sectors to the lowest value here - we should always be passing in the original nb_sectors value (the underlying driver will truncate the returned pnum value as appropriate). > >+ > >+ if (nb_sectors == 0) { > >+ break; This can go as well. -Jeff > > While I can see that in this case ret would be 0, I think it wouldn't hurt > to add an explicit "ret = 0;" here, too. > > Max > > >+ } > > } > >+ > >+ *pnum = res; > > return ret; > > } > > > >
Am 19.09.2016 um 22:39 hat Eric Blake geschrieben: > On 09/18/2016 11:37 PM, Denis V. Lunev wrote: > > On 09/19/2016 04:21 AM, Fam Zheng wrote: > >> On Thu, 09/15 19:34, Denis V. Lunev wrote: > >>> They should work very similar, covering same areas if backing store is > >>> shorter than the image. This change is necessary for the followup patch > >>> switching to bdrv_get_block_status_above() in mirror to avoid assert > >>> in check_block. > >>> > >>> This change should be made very carefully. Let us assume that we have > >>> top image and 2 backing stores L0->L1->L2. > >> Stupid question: which one is top and which are backing? > > L0 is top, L2 is at bottom. > > I typically write this as: > > L2 <- L1 <- L0 > > (read "L2 backs L1, which in turn backs L0") with the active on the > right. So I understand the confusion in Fam's question where you were > using the opposite direction. And I tend to use this one: base <- sn1 <- sn2 <- top "sn*" isn't any better than "L*", but having at least one of "base" and "top" (or "active") in there disambiguates the roles of the nodes. Kevin
On Mon, Sep 26, 2016 at 05:04:21PM +0200, Kevin Wolf wrote: > Am 19.09.2016 um 22:39 hat Eric Blake geschrieben: [...] > > I typically write this as: > > > > L2 <- L1 <- L0 > > > > (read "L2 backs L1, which in turn backs L0") with the active on the > > right. So I understand the confusion in Fam's question where you were > > using the opposite direction. > > And I tend to use this one: > > base <- sn1 <- sn2 <- top > > "sn*" isn't any better than "L*", but having at least one of "base" and > "top" (or "active") in there disambiguates the roles of the nodes. Not to quibble over terminology too much, but now that I'm writing a doc that I want to submit upstream, I began with your (Kevin's) notation. Then, I thought: Hmm, "sn1" could also be referred to as 'base', and "sn2" as 'top' when using `block-commit`' (and `block-stream`, once it starts supporting intermediate streaming?). And, moreover, as Eric (correctly) warns elsewhere about file-names vs. points-in-time: the guest state when 'sn1' was created is contained in 'base', so one could argue that 'sn1' ("snapshot 1") is a misnomer, and is technically 'overlay1'. So, I used the below notation until recently, including 'active' (with the rationale Kevin mentioned): base <- overlay1 <- overlay2 <- active Then, someone asked: "In the above chain, are you pointing to 'overlay2' as active, or is 'active' a separate image unto itself"? "Sigh, so it is still prone to misunderstanding", I thought. Given that, for now, though slightly more verbose and space-occupying, I settled on the below (occasionally doing s/base/orig/, to avoid the "overlay1 could be referred to as 'base' in some cases" problem): Live QEMU | v base <- overlay1 <- overlay2 <- overlay3 FWIW, the above also avoids the problem of a file called 'active' being described as: "previously-active, but not anymore, because its contents are merged into its backing file" in the event of a 'block-commit'. </end quibble>
diff --git a/block/io.c b/block/io.c index 420944d..067d465 100644 --- a/block/io.c +++ b/block/io.c @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, BlockDriverState **file) { BlockDriverState *p; - int64_t ret = 0; + int64_t ret = 0, res = nb_sectors; assert(bs != base); for (p = bs; p != base; p = backing_bs(p)) { - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { - break; + int sc; + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); + if (ret < 0) { + return ret; + } else if (ret & BDRV_BLOCK_ALLOCATED) { + *pnum = sc; + return ret; + } + + if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) { + res = sc; } + /* [sector_num, pnum] unallocated on this layer, which could be only * the first part of [sector_num, nb_sectors]. */ - nb_sectors = MIN(nb_sectors, *pnum); + nb_sectors = MIN(nb_sectors, sc); + + if (nb_sectors == 0) { + break; + } } + + *pnum = res; return ret; }
They should work very similar, covering same areas if backing store is shorter than the image. This change is necessary for the followup patch switching to bdrv_get_block_status_above() in mirror to avoid assert in check_block. This change should be made very carefully. Let us assume that we have top image and 2 backing stores L0->L1->L2. L0: -------------- L1: ------- L2: -------======= The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED and we should return it as filled in L0 image with properly calculated *pnum value. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Fam Zheng <famz@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Max Reitz <mreitz@redhat.com> CC: Jeff Cody <jcody@redhat.com> --- block/io.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)