| Message ID | 20190606184159.979-5-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/dirty-bitmap: check number and size constraints against queued bitmaps | expand |
On 6/6/19 1:41 PM, John Snow wrote: > When we check to see if we can store a bitmap, we don't check how many > we've queued up. This can cause a problem saving bitmaps on close > instead of when we request them to be added. With the stricter add > interface, prohibit these bitmaps specifically. > > To match, make the remove interface more strict as well; now rejecting > any requests to remove bitmaps that were never queued for storage. > > We don't need to "find" the bitmap when the interface has been given the > bitmap explicitly, but this is done to make sure that the bitmap given > actually does belong to the bs we were passed as a paranoia check to > enforce consistency. > > --- Oops - that marker... > > "What about directory size?" Please see the following patch. > > Signed-off-by: John Snow <jsnow@redhat.com> ...renders the S-o-b invisible. > +++ b/block/qcow2-bitmap.c > @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, > return NULL; > } > > +static int qcow2_remove_queued_dirty_bitmap( > + BlockDriverState *bs, const char *name, Error **errp) > +{ > + BDRVQcow2State *s = bs->opaque; > + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name); > + if (!bitmap) { > + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'", > + bdrv_get_node_name(bs), name); > + return -ENOENT; > + } > + assert(s->nb_queued_bitmaps > 0); > + assert(bdrv_dirty_bitmap_get_persistence(bitmap)); > + s->nb_queued_bitmaps -= 1; I tend to use -- over -= 1. > @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, > goto fail; > } > > + s->nb_queued_bitmaps += 1; And again, for ++ Reviewed-by: Eric Blake <eblake@redhat.com>
06.06.2019 21:41, John Snow wrote: > When we check to see if we can store a bitmap, we don't check how many > we've queued up. This can cause a problem saving bitmaps on close > instead of when we request them to be added. With the stricter add > interface, prohibit these bitmaps specifically. > > To match, make the remove interface more strict as well; now rejecting > any requests to remove bitmaps that were never queued for storage. > > We don't need to "find" the bitmap when the interface has been given the > bitmap explicitly, but this is done to make sure that the bitmap given > actually does belong to the bs we were passed as a paranoia check to > enforce consistency. If you want to check it, I'd really prefer to do it explictly, by adding "bool bdrv_has_dirty_bitmap(bs, bitmap) handler, or bitmap.bs field", instead of hiding it under inconvenient interface of helper, so we actually do name = bdrv_dirty_bitmap_name(bitmap); bitmap = bdrv_find_dirty_bitmap(bs, name); it really looks strange. Hmmm, when I read series cover letter and this commit message, I thought that you'll just calculate current number of persistent bitmaps on bs.. Do we really need to introduce additional counters on qcow2 state? So, you want to check nb_queued + s->nb_bitmaps instead of just s->nb_bitmaps. I think we can just count persistent dirty bitmaps and take that number (as, there should not be in-image bitmaps, for which we don't have in-ram version, but we can check it too and return an error on mismatch (or count in-image-not-in-ram bitmaps and this count to number of in-ram persistent bitmaps it seems an extra thing).. > > --- > > "What about directory size?" Please see the following patch. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow2.h | 1 + > block/dirty-bitmap.c | 8 +++----- > block/qcow2-bitmap.c | 31 ++++++++++++++++++++++++++----- > 3 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index ce07f003f7..ebf60ac236 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -317,6 +317,7 @@ typedef struct BDRVQcow2State { > QCowSnapshot *snapshots; > > uint32_t nb_bitmaps; > + uint32_t nb_queued_bitmaps; > uint64_t bitmap_directory_size; > uint64_t bitmap_directory_offset; > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 4667f9e65a..084c42af57 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -450,11 +450,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) > } > > /** > - * Remove persistent dirty bitmap from the storage if it exists. > - * Absence of bitmap is not an error, because we have the following scenario: > - * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no > - * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should > - * not fail. > + * Remove a persistent dirty bitmap from storage, > + * or dequeue it from being stored if it is enqueued. > + * > * This function doesn't release the corresponding BdrvDirtyBitmap. > */ > int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 930a6c91ff..7193c66787 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, > return NULL; > } > > +static int qcow2_remove_queued_dirty_bitmap( > + BlockDriverState *bs, const char *name, Error **errp) > +{ > + BDRVQcow2State *s = bs->opaque; > + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name); > + if (!bitmap) { > + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'", > + bdrv_get_node_name(bs), name); > + return -ENOENT; > + } > + assert(s->nb_queued_bitmaps > 0); > + assert(bdrv_dirty_bitmap_get_persistence(bitmap)); > + s->nb_queued_bitmaps -= 1; > + > + return 0; > +} > + > int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap, > Error **errp) > @@ -1413,9 +1430,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > const char *name = bdrv_dirty_bitmap_name(bitmap); > > if (s->nb_bitmaps == 0) { > - /* Absence of the bitmap is not an error: see explanation above > - * bdrv_remove_persistent_dirty_bitmap() definition. */ > - return 0; > + return qcow2_remove_queued_dirty_bitmap(bs, name, errp); > } > > if ((ret = bitmap_list_load(bs, &bm_list, errp))) { > @@ -1424,6 +1439,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > > bm = find_bitmap_by_name(bm_list, name); > if (bm == NULL) { > + ret = qcow2_remove_queued_dirty_bitmap(bs, name, errp); > goto fail; > } > > @@ -1544,6 +1560,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > error_setg_errno(errp, -ret, "Failed to update bitmap extension"); > goto fail; > } > + s->nb_queued_bitmaps = 0; > > /* Bitmap directory was successfully updated, so, old data can be dropped. > * TODO it is better to reuse these clusters */ > @@ -1618,6 +1635,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, > Qcow2BitmapList *bm_list; > const char *name = bdrv_dirty_bitmap_name(bitmap); > uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); > + uint32_t nb_bitmaps; > int ret = 0; > > if (s->qcow_version < 3) { > @@ -1636,11 +1654,12 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, > goto fail; > } > > - if (s->nb_bitmaps == 0) { > + nb_bitmaps = s->nb_bitmaps + s->nb_queued_bitmaps; > + if (nb_bitmaps == 0) { > return 0; > } > > - if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { > + if (nb_bitmaps >= QCOW2_MAX_BITMAPS) { > error_setg(errp, > "Maximum number of persistent bitmaps is already reached"); > ret = -EOVERFLOW; > @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, > goto fail; > } > > + s->nb_queued_bitmaps += 1; > + > return 0; > fail: > error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", >
On 6/6/19 10:27 PM, Eric Blake wrote: > On 6/6/19 1:41 PM, John Snow wrote: >> When we check to see if we can store a bitmap, we don't check how many >> we've queued up. This can cause a problem saving bitmaps on close >> instead of when we request them to be added. With the stricter add >> interface, prohibit these bitmaps specifically. >> >> To match, make the remove interface more strict as well; now rejecting >> any requests to remove bitmaps that were never queued for storage. >> >> We don't need to "find" the bitmap when the interface has been given the >> bitmap explicitly, but this is done to make sure that the bitmap given >> actually does belong to the bs we were passed as a paranoia check to >> enforce consistency. >> >> --- > > Oops - that marker... > >> >> "What about directory size?" Please see the following patch. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > > ...renders the S-o-b invisible. > >> +++ b/block/qcow2-bitmap.c >> @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, >> return NULL; >> } >> >> +static int qcow2_remove_queued_dirty_bitmap( >> + BlockDriverState *bs, const char *name, Error **errp) >> +{ >> + BDRVQcow2State *s = bs->opaque; >> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name); >> + if (!bitmap) { >> + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'", >> + bdrv_get_node_name(bs), name); >> + return -ENOENT; >> + } >> + assert(s->nb_queued_bitmaps > 0); >> + assert(bdrv_dirty_bitmap_get_persistence(bitmap)); >> + s->nb_queued_bitmaps -= 1; > > I tend to use -- over -= 1. > >> @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >> goto fail; >> } >> >> + s->nb_queued_bitmaps += 1; > > And again, for ++ > > Reviewed-by: Eric Blake <eblake@redhat.com> > Wow, sorry, lots of Python lately!
On 6/7/19 10:58 AM, Vladimir Sementsov-Ogievskiy wrote: > 06.06.2019 21:41, John Snow wrote: >> When we check to see if we can store a bitmap, we don't check how many >> we've queued up. This can cause a problem saving bitmaps on close >> instead of when we request them to be added. With the stricter add >> interface, prohibit these bitmaps specifically. >> >> To match, make the remove interface more strict as well; now rejecting >> any requests to remove bitmaps that were never queued for storage. >> >> We don't need to "find" the bitmap when the interface has been given the >> bitmap explicitly, but this is done to make sure that the bitmap given >> actually does belong to the bs we were passed as a paranoia check to >> enforce consistency. > > If you want to check it, I'd really prefer to do it explictly, > by adding "bool bdrv_has_dirty_bitmap(bs, bitmap) handler, or bitmap.bs field", > instead of hiding it under inconvenient interface of helper, so we actually do > > name = bdrv_dirty_bitmap_name(bitmap); > bitmap = bdrv_find_dirty_bitmap(bs, name); > > it really looks strange. > You're right, it does look weird. It's not obvious when you read it what the real purpose is. I'll try to rectify it with an explicit helper like you suggest. > Hmmm, when I read series cover letter and this commit message, I thought > that you'll just calculate current number of persistent bitmaps on bs.. > Do we really need to introduce additional counters on qcow2 state? > I suppose I could do that, too -- that seems a bit heavier than just incrementing a counter, but it has less risk of getting out of sync. > So, you want to check nb_queued + s->nb_bitmaps instead of just s->nb_bitmaps. > > I think we can just count persistent dirty bitmaps and take that number > (as, there should not be in-image bitmaps, for which we don't have in-ram > version, but we can check it too and return an error on mismatch (or count > in-image-not-in-ram bitmaps and this count to number of in-ram persistent > bitmaps it seems an extra thing).. > If I cache the bm_list as discussed in patch 2, we can actually avoid re-enumerating all of the bitmaps and just go off of that data without having to add any new counters. --js
diff --git a/block/qcow2.h b/block/qcow2.h index ce07f003f7..ebf60ac236 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -317,6 +317,7 @@ typedef struct BDRVQcow2State { QCowSnapshot *snapshots; uint32_t nb_bitmaps; + uint32_t nb_queued_bitmaps; uint64_t bitmap_directory_size; uint64_t bitmap_directory_offset; diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 4667f9e65a..084c42af57 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -450,11 +450,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) } /** - * Remove persistent dirty bitmap from the storage if it exists. - * Absence of bitmap is not an error, because we have the following scenario: - * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no - * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should - * not fail. + * Remove a persistent dirty bitmap from storage, + * or dequeue it from being stored if it is enqueued. + * * This function doesn't release the corresponding BdrvDirtyBitmap. */ int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 930a6c91ff..7193c66787 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, return NULL; } +static int qcow2_remove_queued_dirty_bitmap( + BlockDriverState *bs, const char *name, Error **errp) +{ + BDRVQcow2State *s = bs->opaque; + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name); + if (!bitmap) { + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'", + bdrv_get_node_name(bs), name); + return -ENOENT; + } + assert(s->nb_queued_bitmaps > 0); + assert(bdrv_dirty_bitmap_get_persistence(bitmap)); + s->nb_queued_bitmaps -= 1; + + return 0; +} + int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp) @@ -1413,9 +1430,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name = bdrv_dirty_bitmap_name(bitmap); if (s->nb_bitmaps == 0) { - /* Absence of the bitmap is not an error: see explanation above - * bdrv_remove_persistent_dirty_bitmap() definition. */ - return 0; + return qcow2_remove_queued_dirty_bitmap(bs, name, errp); } if ((ret = bitmap_list_load(bs, &bm_list, errp))) { @@ -1424,6 +1439,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, bm = find_bitmap_by_name(bm_list, name); if (bm == NULL) { + ret = qcow2_remove_queued_dirty_bitmap(bs, name, errp); goto fail; } @@ -1544,6 +1560,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) error_setg_errno(errp, -ret, "Failed to update bitmap extension"); goto fail; } + s->nb_queued_bitmaps = 0; /* Bitmap directory was successfully updated, so, old data can be dropped. * TODO it is better to reuse these clusters */ @@ -1618,6 +1635,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, Qcow2BitmapList *bm_list; const char *name = bdrv_dirty_bitmap_name(bitmap); uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); + uint32_t nb_bitmaps; int ret = 0; if (s->qcow_version < 3) { @@ -1636,11 +1654,12 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, goto fail; } - if (s->nb_bitmaps == 0) { + nb_bitmaps = s->nb_bitmaps + s->nb_queued_bitmaps; + if (nb_bitmaps == 0) { return 0; } - if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { + if (nb_bitmaps >= QCOW2_MAX_BITMAPS) { error_setg(errp, "Maximum number of persistent bitmaps is already reached"); ret = -EOVERFLOW; @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, goto fail; } + s->nb_queued_bitmaps += 1; + return 0; fail: error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
When we check to see if we can store a bitmap, we don't check how many we've queued up. This can cause a problem saving bitmaps on close instead of when we request them to be added. With the stricter add interface, prohibit these bitmaps specifically. To match, make the remove interface more strict as well; now rejecting any requests to remove bitmaps that were never queued for storage. We don't need to "find" the bitmap when the interface has been given the bitmap explicitly, but this is done to make sure that the bitmap given actually does belong to the bs we were passed as a paranoia check to enforce consistency. --- "What about directory size?" Please see the following patch. Signed-off-by: John Snow <jsnow@redhat.com> --- block/qcow2.h | 1 + block/dirty-bitmap.c | 8 +++----- block/qcow2-bitmap.c | 31 ++++++++++++++++++++++++++----- 3 files changed, 30 insertions(+), 10 deletions(-)