| Message ID | 20190607184802.100945-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap | expand |
07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote: > qcow2_can_store_new_dirty_bitmap works wrong, as it considers only > bitmaps already stored in the qcow2 image and ignores persistent > BdrvDirtyBitmap objects. > > So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 > bitmaps on open, so there should not be any bitmap in the image for > which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of > corruption, and no reason to check for corruptions here (open() and > close() are better places for it). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > > Hi! > > Patch is better than discussing I thing, so here is my counter-suggestion for > "[PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps" > by John. > > It's of course needs some additional refactoring, as first assert shows bad design, > I just wrote it in such manner to make minimum changes to fix the bug. > > Of course, > Reported-by: aihua liang <aliang@redhat.com> > may be applied here (if I understood correctly), and I hope that > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 > too. > > block/qcow2-bitmap.c | 38 +++++++++++++++++--------------------- > 1 file changed, 17 insertions(+), 21 deletions(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index b2487101ed..7d1b3eeb2b 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1619,8 +1619,11 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > Error **errp) > { > BDRVQcow2State *s = bs->opaque; > - bool found; > - Qcow2BitmapList *bm_list; > + BdrvDirtyBitmap *bitmap; > + uint64_t bitmap_directory_size = 0; > + uint32_t nb_bitmaps = 0; > + > + assert(!bdrv_find_dirty_bitmap(bs, name)); exactly bad, this should be checked in qmp_block_dirty_bitmap_add(), before checks around persistence. and aio_context_acquire may be dropped.. But anyway, idea is clear I think, consider it as RFC patch > > if (s->qcow_version < 3) { > /* Without autoclear_features, we would always have to assume > @@ -1636,36 +1639,29 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > goto fail; > } > > - if (s->nb_bitmaps == 0) { > - return true; > + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; > + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) > + { > + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { > + nb_bitmaps++; > + bitmap_directory_size += > + calc_dir_entry_size(strlen(bdrv_dirty_bitmap_name(bitmap)), 0); > + } > } > + nb_bitmaps++; > + bitmap_directory_size += calc_dir_entry_size(strlen(name), 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"); > goto fail; > } > > - if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) > > - QCOW2_MAX_BITMAP_DIRECTORY_SIZE) > - { > + if (bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { > error_setg(errp, "Not enough space in the bitmap directory"); > goto fail; > } > > - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > - s->bitmap_directory_size, errp); > - if (bm_list == NULL) { > - goto fail; > - } > - > - found = find_bitmap_by_name(bm_list, name); > - bitmap_list_free(bm_list); > - if (found) { > - error_setg(errp, "Bitmap with the same name is already stored"); > - goto fail; > - } > - > return true; > > fail: >
On 6/7/19 1:53 PM, Vladimir Sementsov-Ogievskiy wrote: > 07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote: >> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only >> bitmaps already stored in the qcow2 image and ignores persistent >> BdrvDirtyBitmap objects. >> >> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 >> bitmaps on open, so there should not be any bitmap in the image for >> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of >> corruption, and no reason to check for corruptions here (open() and >> close() are better places for it). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> >> Hi! >> >> Patch is better than discussing I thing, so here is my counter-suggestion for >> "[PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps" >> by John. >> >> It's of course needs some additional refactoring, as first assert shows bad design, >> I just wrote it in such manner to make minimum changes to fix the bug. >> >> + assert(!bdrv_find_dirty_bitmap(bs, name)); > > exactly bad, this should be checked in qmp_block_dirty_bitmap_add(), before checks around > persistence. and aio_context_acquire may be dropped.. > > But anyway, idea is clear I think, consider it as RFC patch Are you planning to post a v2, as this affects at least https://bugzilla.redhat.com/show_bug.cgi?id=1712636
On 10/10/19 11:39 AM, Eric Blake wrote: > On 6/7/19 1:53 PM, Vladimir Sementsov-Ogievskiy wrote: >> 07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote: >>> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only >>> bitmaps already stored in the qcow2 image and ignores persistent >>> BdrvDirtyBitmap objects. >>> >>> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 >>> bitmaps on open, so there should not be any bitmap in the image for >>> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of >>> corruption, and no reason to check for corruptions here (open() and >>> close() are better places for it). >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> >>> Hi! >>> >>> Patch is better than discussing I thing, so here is my >>> counter-suggestion for >>> "[PATCH 0/5] block/dirty-bitmap: check number and size constraints >>> against queued bitmaps" >>> by John. >>> >>> It's of course needs some additional refactoring, as first assert >>> shows bad design, >>> I just wrote it in such manner to make minimum changes to fix the bug. >>> > >>> + assert(!bdrv_find_dirty_bitmap(bs, name)); >> >> exactly bad, this should be checked in qmp_block_dirty_bitmap_add(), >> before checks around >> persistence. and aio_context_acquire may be dropped.. >> >> But anyway, idea is clear I think, consider it as RFC patch > > Are you planning to post a v2, as this affects at least > https://bugzilla.redhat.com/show_bug.cgi?id=1712636 > I suppose we ought to do it this way for now as it's less SLOC than my idea, but I have to admit I would still prefer to get rid of "can_store" altogether and provide concrete bitmap_store() and bitmap_remove() callbacks for purpose of symmetry and model simplicity. I guess I'll worry about that discussion some other time. --js
10.10.2019 21:46, John Snow wrote: > > > On 10/10/19 11:39 AM, Eric Blake wrote: >> On 6/7/19 1:53 PM, Vladimir Sementsov-Ogievskiy wrote: >>> 07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote: >>>> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only >>>> bitmaps already stored in the qcow2 image and ignores persistent >>>> BdrvDirtyBitmap objects. >>>> >>>> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 >>>> bitmaps on open, so there should not be any bitmap in the image for >>>> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of >>>> corruption, and no reason to check for corruptions here (open() and >>>> close() are better places for it). >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> >>>> Hi! >>>> >>>> Patch is better than discussing I thing, so here is my >>>> counter-suggestion for >>>> "[PATCH 0/5] block/dirty-bitmap: check number and size constraints >>>> against queued bitmaps" >>>> by John. >>>> >>>> It's of course needs some additional refactoring, as first assert >>>> shows bad design, >>>> I just wrote it in such manner to make minimum changes to fix the bug. >>>> >> >>>> + assert(!bdrv_find_dirty_bitmap(bs, name)); >>> >>> exactly bad, this should be checked in qmp_block_dirty_bitmap_add(), >>> before checks around >>> persistence. and aio_context_acquire may be dropped.. >>> >>> But anyway, idea is clear I think, consider it as RFC patch >> >> Are you planning to post a v2, as this affects at least >> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >> > > I suppose we ought to do it this way for now as it's less SLOC than my > idea, but I have to admit I would still prefer to get rid of "can_store" > altogether and provide concrete bitmap_store() and bitmap_remove() > callbacks for purpose of symmetry and model simplicity. > > I guess I'll worry about that discussion some other time. > > --js > Should I base it on master or on you bitmaps branch? Do we want it for stable?
On 10/11/19 6:02 AM, Vladimir Sementsov-Ogievskiy wrote: > 10.10.2019 21:46, John Snow wrote: >> >> >> On 10/10/19 11:39 AM, Eric Blake wrote: >>> On 6/7/19 1:53 PM, Vladimir Sementsov-Ogievskiy wrote: >>>> 07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote: >>>>> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only >>>>> bitmaps already stored in the qcow2 image and ignores persistent >>>>> BdrvDirtyBitmap objects. >>>>> >>>>> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 >>>>> bitmaps on open, so there should not be any bitmap in the image for >>>>> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of >>>>> corruption, and no reason to check for corruptions here (open() and >>>>> close() are better places for it). >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> >>>>> Hi! >>>>> >>>>> Patch is better than discussing I thing, so here is my >>>>> counter-suggestion for >>>>> "[PATCH 0/5] block/dirty-bitmap: check number and size constraints >>>>> against queued bitmaps" >>>>> by John. >>>>> >>>>> It's of course needs some additional refactoring, as first assert >>>>> shows bad design, >>>>> I just wrote it in such manner to make minimum changes to fix the bug. >>>>> >>> >>>>> + assert(!bdrv_find_dirty_bitmap(bs, name)); >>>> >>>> exactly bad, this should be checked in qmp_block_dirty_bitmap_add(), >>>> before checks around >>>> persistence. and aio_context_acquire may be dropped.. >>>> >>>> But anyway, idea is clear I think, consider it as RFC patch >>> >>> Are you planning to post a v2, as this affects at least >>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >>> >> >> I suppose we ought to do it this way for now as it's less SLOC than my >> idea, but I have to admit I would still prefer to get rid of "can_store" >> altogether and provide concrete bitmap_store() and bitmap_remove() >> callbacks for purpose of symmetry and model simplicity. >> >> I guess I'll worry about that discussion some other time. >> >> --js >> > > Should I base it on master or on you bitmaps branch? Do we want it for stable? > Not sure. I'm going to send the PR out today and you can decide what's best to do. (Please do CC qemu-stable, though.) --js
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index b2487101ed..7d1b3eeb2b 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1619,8 +1619,11 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, Error **errp) { BDRVQcow2State *s = bs->opaque; - bool found; - Qcow2BitmapList *bm_list; + BdrvDirtyBitmap *bitmap; + uint64_t bitmap_directory_size = 0; + uint32_t nb_bitmaps = 0; + + assert(!bdrv_find_dirty_bitmap(bs, name)); if (s->qcow_version < 3) { /* Without autoclear_features, we would always have to assume @@ -1636,36 +1639,29 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, goto fail; } - if (s->nb_bitmaps == 0) { - return true; + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) + { + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + nb_bitmaps++; + bitmap_directory_size += + calc_dir_entry_size(strlen(bdrv_dirty_bitmap_name(bitmap)), 0); + } } + nb_bitmaps++; + bitmap_directory_size += calc_dir_entry_size(strlen(name), 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"); goto fail; } - if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) > - QCOW2_MAX_BITMAP_DIRECTORY_SIZE) - { + if (bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { error_setg(errp, "Not enough space in the bitmap directory"); goto fail; } - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, - s->bitmap_directory_size, errp); - if (bm_list == NULL) { - goto fail; - } - - found = find_bitmap_by_name(bm_list, name); - bitmap_list_free(bm_list); - if (found) { - error_setg(errp, "Bitmap with the same name is already stored"); - goto fail; - } - return true; fail:
qcow2_can_store_new_dirty_bitmap works wrong, as it considers only bitmaps already stored in the qcow2 image and ignores persistent BdrvDirtyBitmap objects. So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 bitmaps on open, so there should not be any bitmap in the image for which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of corruption, and no reason to check for corruptions here (open() and close() are better places for it). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- Hi! Patch is better than discussing I thing, so here is my counter-suggestion for "[PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps" by John. It's of course needs some additional refactoring, as first assert shows bad design, I just wrote it in such manner to make minimum changes to fix the bug. Of course, Reported-by: aihua liang <aliang@redhat.com> may be applied here (if I understood correctly), and I hope that Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 too. block/qcow2-bitmap.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-)