| Message ID | 20190301191545.8728-7-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: add inconsistent bit | expand |
On 3/1/19 1:15 PM, John Snow wrote: > We didn't do any state checking on source bitmaps at all, > so this adds inconsistent and busy checks. readonly is > allowed, so you can still copy a readonly bitmap to a new > destination to use it for operations like drive-backup. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 3 +++ > 1 file changed, 3 insertions(+) > I understand forbidding inconsistent sources (because if the source is potentially missing bits, then the merge destination will also be missing bits and thus be inconsistent), but why forbid busy? If I've associated a bitmap with an NBD server (making it busy), it is still readable, and so I should still be able to merge its bits into another copy. > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 769668ccdc..8403c9981d 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > goto out; > } > > + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { Thus, I think this should be BDRV_BITMAP_INCONSISTENT.
On 3/1/19 2:44 PM, Eric Blake wrote: > On 3/1/19 1:15 PM, John Snow wrote: >> We didn't do any state checking on source bitmaps at all, >> so this adds inconsistent and busy checks. readonly is >> allowed, so you can still copy a readonly bitmap to a new >> destination to use it for operations like drive-backup. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/dirty-bitmap.c | 3 +++ >> 1 file changed, 3 insertions(+) >> > > I understand forbidding inconsistent sources (because if the source is > potentially missing bits, then the merge destination will also be > missing bits and thus be inconsistent), but why forbid busy? If I've > associated a bitmap with an NBD server (making it busy), it is still > readable, and so I should still be able to merge its bits into another copy. > True, do you rely on this, though? I was working from a space of "busy" meant "actively in-use by an operation, and COULD change" so I was forbidding it out of good hygiene. Clearly the ones in-use by NBD are actually static and unchanging, so it's safer -- but that might not be true for push backups, where you might not actually be getting what you think you are, because of the bifurcated nature of those bitmaps. If this causes a problem for you in the short-term I will simply roll this back, but it stands out to me. (I can't stop myself from trying to protect the user from themselves. It's clearly a recurring theme in my design and reviews.) >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 769668ccdc..8403c9981d 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, >> goto out; >> } >> >> + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { > > Thus, I think this should be BDRV_BITMAP_INCONSISTENT. >
On 3/1/19 1:48 PM, John Snow wrote: >> I understand forbidding inconsistent sources (because if the source is >> potentially missing bits, then the merge destination will also be >> missing bits and thus be inconsistent), but why forbid busy? If I've >> associated a bitmap with an NBD server (making it busy), it is still >> readable, and so I should still be able to merge its bits into another copy. >> > > True, do you rely on this, though? Not in my current libvirt code (as I create a temporary bitmap to hand to NBD, since it may be the merge of one or more disabled bitmaps in a differential backup case), so being tighter for now and relaxing later if we DO come up with a use is acceptable. > > I was working from a space of "busy" meant "actively in-use by an > operation, and COULD change" so I was forbidding it out of good hygiene. > > Clearly the ones in-use by NBD are actually static and unchanging, so > it's safer -- but that might not be true for push backups, where you > might not actually be getting what you think you are, because of the > bifurcated nature of those bitmaps. Oh, good point, especially after you worked so hard to merge locked/frozen into a single status - you WILL miss the bits from the successor (unless we teach the merge algorithm to pull in the busy bitmap's bits AND all the bits of its successors - but that feels like a lot of work if we don't have a client needing it now). Okay, with the extra justification mentioned in the commit message, > > If this causes a problem for you in the short-term I will simply roll > this back, but it stands out to me. > > (I can't stop myself from trying to protect the user from themselves. > It's clearly a recurring theme in my design and reviews.) > >>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>> index 769668ccdc..8403c9981d 100644 >>> --- a/block/dirty-bitmap.c >>> +++ b/block/dirty-bitmap.c >>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, >>> goto out; >>> } >>> >>> + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { >> >> Thus, I think this should be BDRV_BITMAP_INCONSISTENT. then I retract my complaint, and the code is acceptable for now. Reviewed-by: Eric Blake <eblake@redhat.com>
On 3/1/19 2:57 PM, Eric Blake wrote: > On 3/1/19 1:48 PM, John Snow wrote: > >>> I understand forbidding inconsistent sources (because if the source is >>> potentially missing bits, then the merge destination will also be >>> missing bits and thus be inconsistent), but why forbid busy? If I've >>> associated a bitmap with an NBD server (making it busy), it is still >>> readable, and so I should still be able to merge its bits into another copy. >>> >> >> True, do you rely on this, though? > > Not in my current libvirt code (as I create a temporary bitmap to hand > to NBD, since it may be the merge of one or more disabled bitmaps in a > differential backup case), so being tighter for now and relaxing later > if we DO come up with a use is acceptable. > >> >> I was working from a space of "busy" meant "actively in-use by an >> operation, and COULD change" so I was forbidding it out of good hygiene. >> >> Clearly the ones in-use by NBD are actually static and unchanging, so >> it's safer -- but that might not be true for push backups, where you >> might not actually be getting what you think you are, because of the >> bifurcated nature of those bitmaps. > > Oh, good point, especially after you worked so hard to merge > locked/frozen into a single status - you WILL miss the bits from the > successor (unless we teach the merge algorithm to pull in the busy > bitmap's bits AND all the bits of its successors - but that feels like a > lot of work if we don't have a client needing it now). Okay, with the > extra justification mentioned in the commit message, > (Though I am being a little fast and loose here: when we split a bitmap, the top-level one that retains the name actually stays unchanging and the child bitmap is the one that starts accruing writes from a blank canvas, but that STILL may not be what you expect when you choose it as a merge source, however.) >> >> If this causes a problem for you in the short-term I will simply roll >> this back, but it stands out to me. >> >> (I can't stop myself from trying to protect the user from themselves. >> It's clearly a recurring theme in my design and reviews.) >> >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 769668ccdc..8403c9981d 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, >>>> goto out; >>>> } >>>> >>>> + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { >>> >>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT. > > then I retract my complaint, and the code is acceptable for now. > > Reviewed-by: Eric Blake <eblake@redhat.com> > We could always split it back out later, but in basic terms for permissions and user perspective, "in use" seems robust enough of a resolution. (It might be safe to read, it might not be, who knows -- it's in use.) If it really comes to a point, we can always re-add a new status bit to let the end-user know if they're working with a bifurcated (I have to use weird vocabulary words sometimes) bitmap but at the moment it seems very safely an implementation detail. You can also check that for "enabled" bitmap as reported back to user via QAPI I check to see if the parent OR child is enabled and report that cumulatively as "enabled", because together they are "effectively" enabled. --js
01.03.2019 23:04, John Snow wrote: > > > On 3/1/19 2:57 PM, Eric Blake wrote: >> On 3/1/19 1:48 PM, John Snow wrote: >> >>>> I understand forbidding inconsistent sources (because if the source is >>>> potentially missing bits, then the merge destination will also be >>>> missing bits and thus be inconsistent), but why forbid busy? If I've >>>> associated a bitmap with an NBD server (making it busy), it is still >>>> readable, and so I should still be able to merge its bits into another copy. >>>> >>> >>> True, do you rely on this, though? >> >> Not in my current libvirt code (as I create a temporary bitmap to hand >> to NBD, since it may be the merge of one or more disabled bitmaps in a >> differential backup case), so being tighter for now and relaxing later >> if we DO come up with a use is acceptable. >> >>> >>> I was working from a space of "busy" meant "actively in-use by an >>> operation, and COULD change" so I was forbidding it out of good hygiene. >>> >>> Clearly the ones in-use by NBD are actually static and unchanging, so >>> it's safer -- but that might not be true for push backups, where you >>> might not actually be getting what you think you are, because of the >>> bifurcated nature of those bitmaps. >> >> Oh, good point, especially after you worked so hard to merge >> locked/frozen into a single status - you WILL miss the bits from the >> successor (unless we teach the merge algorithm to pull in the busy >> bitmap's bits AND all the bits of its successors - but that feels like a >> lot of work if we don't have a client needing it now). Okay, with the >> extra justification mentioned in the commit message, >> > > (Though I am being a little fast and loose here: when we split a bitmap, > the top-level one that retains the name actually stays unchanging and > the child bitmap is the one that starts accruing writes from a blank > canvas, but that STILL may not be what you expect when you choose it as > a merge source, however.) > >>> >>> If this causes a problem for you in the short-term I will simply roll >>> this back, but it stands out to me. >>> >>> (I can't stop myself from trying to protect the user from themselves. >>> It's clearly a recurring theme in my design and reviews.) >>> >>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>>> index 769668ccdc..8403c9981d 100644 >>>>> --- a/block/dirty-bitmap.c >>>>> +++ b/block/dirty-bitmap.c >>>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, >>>>> goto out; >>>>> } >>>>> >>>>> + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { >>>> >>>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT. >> >> then I retract my complaint, and the code is acceptable for now. >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> > > We could always split it back out later, but in basic terms for > permissions and user perspective, "in use" seems robust enough of a > resolution. (It might be safe to read, it might not be, who knows -- > it's in use.) I think, if we need some kind of sharing busy bitmaps, it should be two busy states: one, which allows reads for other users and one that doesn't. > > If it really comes to a point, we can always re-add a new status bit to > let the end-user know if they're working with a bifurcated (I have to > use weird vocabulary words sometimes) bitmap but at the moment it seems > very safely an implementation detail. > > You can also check that for "enabled" bitmap as reported back to user > via QAPI I check to see if the parent OR child is enabled and report > that cumulatively as "enabled", because together they are "effectively" > enabled. > > --js >
01.03.2019 22:15, John Snow wrote: > We didn't do any state checking on source bitmaps at all, > so this adds inconsistent and busy checks. readonly is > allowed, so you can still copy a readonly bitmap to a new > destination to use it for operations like drive-backup. > > Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/dirty-bitmap.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 769668ccdc..8403c9981d 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > goto out; > } > > + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { > + goto out; > + } > > if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) { > error_setg(errp, "Bitmaps are incompatible and can't be merged"); >
On 3/6/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote: > 01.03.2019 23:04, John Snow wrote: >> >> >> On 3/1/19 2:57 PM, Eric Blake wrote: >>> On 3/1/19 1:48 PM, John Snow wrote: >>> >>>>> I understand forbidding inconsistent sources (because if the source is >>>>> potentially missing bits, then the merge destination will also be >>>>> missing bits and thus be inconsistent), but why forbid busy? If I've >>>>> associated a bitmap with an NBD server (making it busy), it is still >>>>> readable, and so I should still be able to merge its bits into another copy. >>>>> >>>> >>>> True, do you rely on this, though? >>> >>> Not in my current libvirt code (as I create a temporary bitmap to hand >>> to NBD, since it may be the merge of one or more disabled bitmaps in a >>> differential backup case), so being tighter for now and relaxing later >>> if we DO come up with a use is acceptable. >>> >>>> >>>> I was working from a space of "busy" meant "actively in-use by an >>>> operation, and COULD change" so I was forbidding it out of good hygiene. >>>> >>>> Clearly the ones in-use by NBD are actually static and unchanging, so >>>> it's safer -- but that might not be true for push backups, where you >>>> might not actually be getting what you think you are, because of the >>>> bifurcated nature of those bitmaps. >>> >>> Oh, good point, especially after you worked so hard to merge >>> locked/frozen into a single status - you WILL miss the bits from the >>> successor (unless we teach the merge algorithm to pull in the busy >>> bitmap's bits AND all the bits of its successors - but that feels like a >>> lot of work if we don't have a client needing it now). Okay, with the >>> extra justification mentioned in the commit message, >>> >> >> (Though I am being a little fast and loose here: when we split a bitmap, >> the top-level one that retains the name actually stays unchanging and >> the child bitmap is the one that starts accruing writes from a blank >> canvas, but that STILL may not be what you expect when you choose it as >> a merge source, however.) >> >>>> >>>> If this causes a problem for you in the short-term I will simply roll >>>> this back, but it stands out to me. >>>> >>>> (I can't stop myself from trying to protect the user from themselves. >>>> It's clearly a recurring theme in my design and reviews.) >>>> >>>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>>>> index 769668ccdc..8403c9981d 100644 >>>>>> --- a/block/dirty-bitmap.c >>>>>> +++ b/block/dirty-bitmap.c >>>>>> @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, >>>>>> goto out; >>>>>> } >>>>>> >>>>>> + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { >>>>> >>>>> Thus, I think this should be BDRV_BITMAP_INCONSISTENT. >>> >>> then I retract my complaint, and the code is acceptable for now. >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >> >> We could always split it back out later, but in basic terms for >> permissions and user perspective, "in use" seems robust enough of a >> resolution. (It might be safe to read, it might not be, who knows -- >> it's in use.) > > I think, if we need some kind of sharing busy bitmaps, it should be two > busy states: one, which allows reads for other users and one that doesn't. > I think that's... hopefully premature. We don't need this NOW do we? I think this is OK as-is.
On 3/6/19 9:24 AM, John Snow wrote: >>> >>> We could always split it back out later, but in basic terms for >>> permissions and user perspective, "in use" seems robust enough of a >>> resolution. (It might be safe to read, it might not be, who knows -- >>> it's in use.) >> >> I think, if we need some kind of sharing busy bitmaps, it should be two >> busy states: one, which allows reads for other users and one that doesn't. >> Indeed, similar to flock having shared (multiple read-only users) and exclusive (single writer) locks. So it's nice to know we have a technical plan for growth, if we have a reason to use that plan. > > I think that's... hopefully premature. We don't need this NOW do we? No, we don't need it now. > > I think this is OK as-is. It sounds like we are in violent agreement ;)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 769668ccdc..8403c9981d 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -825,6 +825,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, goto out; } + if (bdrv_dirty_bitmap_check(src, BDRV_BITMAP_ALLOW_RO, errp)) { + goto out; + } if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) { error_setg(errp, "Bitmaps are incompatible and can't be merged");
We didn't do any state checking on source bitmaps at all, so this adds inconsistent and busy checks. readonly is allowed, so you can still copy a readonly bitmap to a new destination to use it for operations like drive-backup. Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 3 +++ 1 file changed, 3 insertions(+)