| Message ID | 20190603120005.37394-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: block-dirty-bitmap-remove transaction action | expand |
On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: > Add functionality to make bitmap temporary anonymous. It will be used > to implement bitmap remove transaction action. We need hide bitmap > persistence too, as there are should not be unnamed persistent bitmaps. > Ah, so this effectively ... "hides" a bitmap from any further transaction actions. It also "hides" it from getting flushed to disk... sort of? The outer loop in store works with bdrv_dirty_bitmap_next, and we'll skip this bitmap because it's anonymous/not persistent. There's a second loop where we iterate bm_list, and we'll skip storing this bitmap because that entry won't have an in-memory bitmap associated with it in bm_list. ...But then we'll call update_ext_header_and_dir with the stale entries in bm_list? > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/dirty-bitmap.h | 2 ++ > block/dirty-bitmap.c | 26 ++++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 8044ace63e..542e437123 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, > BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap, > Error **errp); > +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap); > +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap); > > #endif > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 49646a30e6..592964635e 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap { > bool busy; /* Bitmap is busy, it can't be used via QMP */ > BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ > char *name; /* Optional non-empty unique ID */ > + char *hidden_name; /* Backup of @name for removal transaction > + action. Used for hide/unhide API. */ > + bool hidden_persistent; /* Backup of @persistent for removal transaction > + action. */ > int64_t size; /* Size of the bitmap, in bytes */ > bool disabled; /* Bitmap is disabled. It ignores all writes to > the device */ > @@ -849,3 +853,25 @@ out: > qemu_mutex_unlock(src->mutex); > } > } > + > +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap) > +{ > + qemu_mutex_lock(bitmap->mutex); > + assert(!bitmap->hidden_name); > + bitmap->hidden_name = bitmap->name; > + bitmap->hidden_persistent = bitmap->persistent; > + bitmap->name = NULL; > + bitmap->persistent = false; > + qemu_mutex_unlock(bitmap->mutex); > +} > + > +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap) > +{ > + qemu_mutex_lock(bitmap->mutex); > + assert(!bitmap->name); > + bitmap->name = bitmap->hidden_name; > + bitmap->persistent = bitmap->hidden_persistent; > + bitmap->hidden_name = NULL; > + bitmap->hidden_persistent = false; > + qemu_mutex_unlock(bitmap->mutex); > +} >
08.06.2019 1:39, John Snow wrote: > > > On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >> Add functionality to make bitmap temporary anonymous. It will be used >> to implement bitmap remove transaction action. We need hide bitmap >> persistence too, as there are should not be unnamed persistent bitmaps. >> > > Ah, so this effectively ... "hides" a bitmap from any further > transaction actions. It also "hides" it from getting flushed to disk... > sort of? > > The outer loop in store works with bdrv_dirty_bitmap_next, and we'll > skip this bitmap because it's anonymous/not persistent. > > There's a second loop where we iterate bm_list, and we'll skip storing > this bitmap because that entry won't have an in-memory bitmap associated > with it in bm_list. > > ...But then we'll call update_ext_header_and_dir with the stale entries > in bm_list? Hidden is a temprory state, so, we should not go to close() in this state. It's a state inside a transaction. Stale entries in list will be marked IN_USE anyway, so there is no damage anyway. > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/dirty-bitmap.h | 2 ++ >> block/dirty-bitmap.c | 26 ++++++++++++++++++++++++++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >> index 8044ace63e..542e437123 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, >> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, >> Error **errp); >> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap); >> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap); >> >> #endif >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 49646a30e6..592964635e 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap { >> bool busy; /* Bitmap is busy, it can't be used via QMP */ >> BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ >> char *name; /* Optional non-empty unique ID */ >> + char *hidden_name; /* Backup of @name for removal transaction >> + action. Used for hide/unhide API. */ >> + bool hidden_persistent; /* Backup of @persistent for removal transaction >> + action. */ >> int64_t size; /* Size of the bitmap, in bytes */ >> bool disabled; /* Bitmap is disabled. It ignores all writes to >> the device */ >> @@ -849,3 +853,25 @@ out: >> qemu_mutex_unlock(src->mutex); >> } >> } >> + >> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap) >> +{ >> + qemu_mutex_lock(bitmap->mutex); >> + assert(!bitmap->hidden_name); >> + bitmap->hidden_name = bitmap->name; >> + bitmap->hidden_persistent = bitmap->persistent; >> + bitmap->name = NULL; >> + bitmap->persistent = false; >> + qemu_mutex_unlock(bitmap->mutex); >> +} >> + >> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap) >> +{ >> + qemu_mutex_lock(bitmap->mutex); >> + assert(!bitmap->name); >> + bitmap->name = bitmap->hidden_name; >> + bitmap->persistent = bitmap->hidden_persistent; >> + bitmap->hidden_name = NULL; >> + bitmap->hidden_persistent = false; >> + qemu_mutex_unlock(bitmap->mutex); >> +} >>
10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote: > 08.06.2019 1:39, John Snow wrote: >> >> >> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Add functionality to make bitmap temporary anonymous. It will be used >>> to implement bitmap remove transaction action. We need hide bitmap >>> persistence too, as there are should not be unnamed persistent bitmaps. >>> >> >> Ah, so this effectively ... "hides" a bitmap from any further >> transaction actions. It also "hides" it from getting flushed to disk... >> sort of? >> >> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll >> skip this bitmap because it's anonymous/not persistent. >> >> There's a second loop where we iterate bm_list, and we'll skip storing >> this bitmap because that entry won't have an in-memory bitmap associated >> with it in bm_list. >> >> ...But then we'll call update_ext_header_and_dir with the stale entries >> in bm_list? > > Hidden is a temprory state, so, we should not go to close() in this state. > It's a state inside a transaction. > > Stale entries in list will be marked IN_USE anyway, so there is no damage anyway. I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, seems you are right, we are storing stale IN_USE bitmaps. No serious damage, but bad design: creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow. > >> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> include/block/dirty-bitmap.h | 2 ++ >>> block/dirty-bitmap.c | 26 ++++++++++++++++++++++++++ >>> 2 files changed, 28 insertions(+) >>> >>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>> index 8044ace63e..542e437123 100644 >>> --- a/include/block/dirty-bitmap.h >>> +++ b/include/block/dirty-bitmap.h >>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, >>> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, >>> Error **errp); >>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap); >>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap); >>> #endif >>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>> index 49646a30e6..592964635e 100644 >>> --- a/block/dirty-bitmap.c >>> +++ b/block/dirty-bitmap.c >>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap { >>> bool busy; /* Bitmap is busy, it can't be used via QMP */ >>> BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ >>> char *name; /* Optional non-empty unique ID */ >>> + char *hidden_name; /* Backup of @name for removal transaction >>> + action. Used for hide/unhide API. */ >>> + bool hidden_persistent; /* Backup of @persistent for removal transaction >>> + action. */ >>> int64_t size; /* Size of the bitmap, in bytes */ >>> bool disabled; /* Bitmap is disabled. It ignores all writes to >>> the device */ >>> @@ -849,3 +853,25 @@ out: >>> qemu_mutex_unlock(src->mutex); >>> } >>> } >>> + >>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap) >>> +{ >>> + qemu_mutex_lock(bitmap->mutex); >>> + assert(!bitmap->hidden_name); >>> + bitmap->hidden_name = bitmap->name; >>> + bitmap->hidden_persistent = bitmap->persistent; >>> + bitmap->name = NULL; >>> + bitmap->persistent = false; >>> + qemu_mutex_unlock(bitmap->mutex); >>> +} >>> + >>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap) >>> +{ >>> + qemu_mutex_lock(bitmap->mutex); >>> + assert(!bitmap->name); >>> + bitmap->name = bitmap->hidden_name; >>> + bitmap->persistent = bitmap->hidden_persistent; >>> + bitmap->hidden_name = NULL; >>> + bitmap->hidden_persistent = false; >>> + qemu_mutex_unlock(bitmap->mutex); >>> +} >>> > >
10.06.2019 12:42, Vladimir Sementsov-Ogievskiy wrote: > 10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote: >> 08.06.2019 1:39, John Snow wrote: >>> >>> >>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> Add functionality to make bitmap temporary anonymous. It will be used >>>> to implement bitmap remove transaction action. We need hide bitmap >>>> persistence too, as there are should not be unnamed persistent bitmaps. >>>> >>> >>> Ah, so this effectively ... "hides" a bitmap from any further >>> transaction actions. It also "hides" it from getting flushed to disk... >>> sort of? >>> >>> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll >>> skip this bitmap because it's anonymous/not persistent. >>> >>> There's a second loop where we iterate bm_list, and we'll skip storing >>> this bitmap because that entry won't have an in-memory bitmap associated >>> with it in bm_list. >>> >>> ...But then we'll call update_ext_header_and_dir with the stale entries >>> in bm_list? >> >> Hidden is a temprory state, so, we should not go to close() in this state. >> It's a state inside a transaction. >> >> Stale entries in list will be marked IN_USE anyway, so there is no damage anyway. > > I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, seems you > are right, we are storing stale IN_USE bitmaps. No serious damage, but bad design: > creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow. Ahaha, and I'm again wrong, as my series do right thing: remove bitmap from the image in .prepare, so there would not be stale entries, all is OK. > >> >>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> include/block/dirty-bitmap.h | 2 ++ >>>> block/dirty-bitmap.c | 26 ++++++++++++++++++++++++++ >>>> 2 files changed, 28 insertions(+) >>>> >>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>>> index 8044ace63e..542e437123 100644 >>>> --- a/include/block/dirty-bitmap.h >>>> +++ b/include/block/dirty-bitmap.h >>>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, >>>> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, >>>> Error **errp); >>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap); >>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap); >>>> #endif >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 49646a30e6..592964635e 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap { >>>> bool busy; /* Bitmap is busy, it can't be used via QMP */ >>>> BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ >>>> char *name; /* Optional non-empty unique ID */ >>>> + char *hidden_name; /* Backup of @name for removal transaction >>>> + action. Used for hide/unhide API. */ >>>> + bool hidden_persistent; /* Backup of @persistent for removal transaction >>>> + action. */ >>>> int64_t size; /* Size of the bitmap, in bytes */ >>>> bool disabled; /* Bitmap is disabled. It ignores all writes to >>>> the device */ >>>> @@ -849,3 +853,25 @@ out: >>>> qemu_mutex_unlock(src->mutex); >>>> } >>>> } >>>> + >>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap) >>>> +{ >>>> + qemu_mutex_lock(bitmap->mutex); >>>> + assert(!bitmap->hidden_name); >>>> + bitmap->hidden_name = bitmap->name; >>>> + bitmap->hidden_persistent = bitmap->persistent; >>>> + bitmap->name = NULL; >>>> + bitmap->persistent = false; >>>> + qemu_mutex_unlock(bitmap->mutex); >>>> +} >>>> + >>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap) >>>> +{ >>>> + qemu_mutex_lock(bitmap->mutex); >>>> + assert(!bitmap->name); >>>> + bitmap->name = bitmap->hidden_name; >>>> + bitmap->persistent = bitmap->hidden_persistent; >>>> + bitmap->hidden_name = NULL; >>>> + bitmap->hidden_persistent = false; >>>> + qemu_mutex_unlock(bitmap->mutex); >>>> +} >>>> >> >> > >
10.06.2019 12:42, Vladimir Sementsov-Ogievskiy wrote: > 10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote: >> 08.06.2019 1:39, John Snow wrote: >>> >>> >>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> Add functionality to make bitmap temporary anonymous. It will be used >>>> to implement bitmap remove transaction action. We need hide bitmap >>>> persistence too, as there are should not be unnamed persistent bitmaps. >>>> >>> >>> Ah, so this effectively ... "hides" a bitmap from any further >>> transaction actions. It also "hides" it from getting flushed to disk... >>> sort of? >>> >>> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll >>> skip this bitmap because it's anonymous/not persistent. >>> >>> There's a second loop where we iterate bm_list, and we'll skip storing >>> this bitmap because that entry won't have an in-memory bitmap associated >>> with it in bm_list. >>> >>> ...But then we'll call update_ext_header_and_dir with the stale entries >>> in bm_list? >> >> Hidden is a temprory state, so, we should not go to close() in this state. >> It's a state inside a transaction. >> >> Stale entries in list will be marked IN_USE anyway, so there is no damage anyway. > > I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, seems you > are right, we are storing stale IN_USE bitmaps. No serious damage, but bad design: > creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow. Ahaha, and I'm wrong again, as my series do right thing: remove bitmap from the image in .prepare, so there will be no stale entries.. > >> >>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> include/block/dirty-bitmap.h | 2 ++ >>>> block/dirty-bitmap.c | 26 ++++++++++++++++++++++++++ >>>> 2 files changed, 28 insertions(+) >>>> >>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>>> index 8044ace63e..542e437123 100644 >>>> --- a/include/block/dirty-bitmap.h >>>> +++ b/include/block/dirty-bitmap.h >>>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, >>>> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, >>>> Error **errp); >>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap); >>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap); >>>> #endif >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 49646a30e6..592964635e 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap { >>>> bool busy; /* Bitmap is busy, it can't be used via QMP */ >>>> BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ >>>> char *name; /* Optional non-empty unique ID */ >>>> + char *hidden_name; /* Backup of @name for removal transaction >>>> + action. Used for hide/unhide API. */ >>>> + bool hidden_persistent; /* Backup of @persistent for removal transaction >>>> + action. */ >>>> int64_t size; /* Size of the bitmap, in bytes */ >>>> bool disabled; /* Bitmap is disabled. It ignores all writes to >>>> the device */ >>>> @@ -849,3 +853,25 @@ out: >>>> qemu_mutex_unlock(src->mutex); >>>> } >>>> } >>>> + >>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap) >>>> +{ >>>> + qemu_mutex_lock(bitmap->mutex); >>>> + assert(!bitmap->hidden_name); >>>> + bitmap->hidden_name = bitmap->name; >>>> + bitmap->hidden_persistent = bitmap->persistent; >>>> + bitmap->name = NULL; >>>> + bitmap->persistent = false; >>>> + qemu_mutex_unlock(bitmap->mutex); >>>> +} >>>> + >>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap) >>>> +{ >>>> + qemu_mutex_lock(bitmap->mutex); >>>> + assert(!bitmap->name); >>>> + bitmap->name = bitmap->hidden_name; >>>> + bitmap->persistent = bitmap->hidden_persistent; >>>> + bitmap->hidden_name = NULL; >>>> + bitmap->hidden_persistent = false; >>>> + qemu_mutex_unlock(bitmap->mutex); >>>> +} >>>> >> >> > >
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 8044ace63e..542e437123 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp); +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap); +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap); #endif diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 49646a30e6..592964635e 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap { bool busy; /* Bitmap is busy, it can't be used via QMP */ BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ char *name; /* Optional non-empty unique ID */ + char *hidden_name; /* Backup of @name for removal transaction + action. Used for hide/unhide API. */ + bool hidden_persistent; /* Backup of @persistent for removal transaction + action. */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to the device */ @@ -849,3 +853,25 @@ out: qemu_mutex_unlock(src->mutex); } } + +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap) +{ + qemu_mutex_lock(bitmap->mutex); + assert(!bitmap->hidden_name); + bitmap->hidden_name = bitmap->name; + bitmap->hidden_persistent = bitmap->persistent; + bitmap->name = NULL; + bitmap->persistent = false; + qemu_mutex_unlock(bitmap->mutex); +} + +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap) +{ + qemu_mutex_lock(bitmap->mutex); + assert(!bitmap->name); + bitmap->name = bitmap->hidden_name; + bitmap->persistent = bitmap->hidden_persistent; + bitmap->hidden_name = NULL; + bitmap->hidden_persistent = false; + qemu_mutex_unlock(bitmap->mutex); +}
Add functionality to make bitmap temporary anonymous. It will be used to implement bitmap remove transaction action. We need hide bitmap persistence too, as there are should not be unnamed persistent bitmaps. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/dirty-bitmap.h | 2 ++ block/dirty-bitmap.c | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+)