| Message ID | 1425528911-10300-6-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2015-03-04 at 23:15, John Snow wrote: > Allow bitmap successors to carry reference counts. > > We can in a later patch use this ability to clean up the dirty bitmap > according to both the individual job's success and the success of all > jobs in the transaction group. > > The code for cleaning up a bitmap is also moved from backup_run to > backup_complete. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++---- > block/backup.c | 23 ++++++--------- > include/block/block.h | 11 ++++--- > 3 files changed, 87 insertions(+), 26 deletions(-) > > diff --git a/block.c b/block.c > index 5eaa874..a0036af 100644 > --- a/block.c > +++ b/block.c > @@ -51,6 +51,12 @@ > #include <windows.h> > #endif > > +typedef enum BitmapSuccessorAction { > + SUCCESSOR_ACTION_UNDEFINED = 0, > + SUCCESSOR_ACTION_ABDICATE, > + SUCCESSOR_ACTION_RECLAIM > +} BitmapSuccessorAction; Maybe making this a QAPI enum can come in handy later. I don't know in which QMP commands one would use it, but if I could predict the future, I'd make trillions (or even billions) at the stock market. (It's just that it already looks so much alike to a QAPI enum that I can't help but to think of making it one) > + > /** > * A BdrvDirtyBitmap can be in three possible states: > * (1) successor is false and disabled is false: full r/w mode > @@ -65,6 +71,8 @@ struct BdrvDirtyBitmap { > char *name; /* Optional non-empty unique ID */ > int64_t size; /* Size of the bitmap (Number of sectors) */ > bool disabled; /* Bitmap is read-only */ > + int successor_refcount; /* Number of active handles to the successor */ > + BitmapSuccessorAction act; /* Action to take on successor upon release */ > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > > @@ -5508,6 +5516,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, > > /* Install the successor and freeze the parent */ > bitmap->successor = child; > + bitmap->successor_refcount = 1; > return 0; > } > > @@ -5515,9 +5524,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, > * For a bitmap with a successor, yield our name to the successor, > * Delete the old bitmap, and return a handle to the new bitmap. > */ > -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > - BdrvDirtyBitmap *bitmap, > - Error **errp) > +static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > + Error **errp) > { > char *name; > BdrvDirtyBitmap *successor = bitmap->successor; > @@ -5542,9 +5551,9 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > * We may wish to re-join the parent and child/successor. > * The merged parent will be un-frozen, but not explicitly re-enabled. > */ > -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > - BdrvDirtyBitmap *parent, > - Error **errp) > +static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > + BdrvDirtyBitmap *parent, > + Error **errp) > { > BdrvDirtyBitmap *successor = parent->successor; > > @@ -5563,6 +5572,64 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > return parent; > } > > +static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs, > + BdrvDirtyBitmap *parent, > + Error **errp) > +{ > + if (parent->successor_refcount) { I don't know whether you're intending to call this function from anywhere outside of bdrv_dirty_bitmap_decref(), but if you don't, please just make this an assert(!parent->successor_refcount). > + error_setg(errp, "Cannot free the successor for bitmap '%s', " > + "because the refcount is non-zero.", parent->name); Hm, again with the full stops? *g* > + return NULL; > + } > + > + switch (parent->act) { > + case SUCCESSOR_ACTION_UNDEFINED: > + error_setg(errp, "Cannot free the successor for bitmap '%s', " > + "because the successor action has not yet been set.", Indeed, again with the full stops. ;-) > + parent->name); > + return NULL; So you're (for now) only calling this function from bdrv_dirty_bitmap_decref(), and that function always makes sure that parent->act is set to SUCCESSOR_ACTION_{RECLAIM,ABDICATE}. Why not add an assert(parent->act != SUCCESSOR_ACTION_UNDEFINED) before the switch? (If that causes problems in regards to compiler warnings or something (not having all enum values covered in the switch), just add an assert(0); under "case SUCCESSOR_ACTION_UNDEFINED:".) > + case SUCCESSOR_ACTION_RECLAIM: > + return bdrv_reclaim_dirty_bitmap(bs, parent, errp); > + case SUCCESSOR_ACTION_ABDICATE: > + return bdrv_dirty_bitmap_abdicate(bs, parent, errp); > + default: > + error_setg(errp, > + "Unrecognized successor action (%d), " > + "cannot free successor for bitmap '%s'", > + parent->act, parent->name); > + return NULL; This can be made an assert(0), a g_assert_not_reached(), or simply an abort() (with the latter probably being the preferred way). So I think that all the error_setg() calls are actually cases that should never happen. Better make them abort() and drop the Error parameter (because *_decref() and *_free() functions normally (for good reason) don't have Error parameters...). > + } > +} > + > +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, I don't know whether I'm that content with the name chosen, because you're actually decrementing the refcount of the successor; but since the successor is basically a clone of the original bitmap (and I mean in the Star Trek sense, that it's a teleported clone and the original is intended to be destroyed so the successor can replace it), decrementing the refcount of the successor basically is equal to decrementing the refcount of the bitmap itself (as long as there is a successor, which you are asserting; maybe you want to add a comment about that to include/block/block.h, that one can only use this on frozen bitmaps?). > + BdrvDirtyBitmap *parent, > + int ret, > + Error **errp) > +{ > + assert(bdrv_dirty_bitmap_frozen(parent)); > + assert(parent->successor); > + > + if (ret) { > + parent->act = SUCCESSOR_ACTION_RECLAIM; > + } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) { > + parent->act = SUCCESSOR_ACTION_ABDICATE; > + } > + > + parent->successor_refcount--; > + if (parent->successor_refcount == 0) { > + return bdrv_free_bitmap_successor(bs, parent, errp); If you drop the Error parameter from bdrv_free_bitmap_successor(), you can drop it from this function, too. > + } > + return parent; > +} > + > +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent) > +{ > + assert(bdrv_dirty_bitmap_frozen(parent)); > + assert(parent->successor); > + > + parent->successor_refcount++; > +} > + > static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size) > { > /* Should only be frozen during a block backup job, which should have > diff --git a/block/backup.c b/block/backup.c > index 41bd9af..4332df4 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque) > > bdrv_unref(s->target); > > + if (s->sync_bitmap) { > + BdrvDirtyBitmap *bm; > + bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, data->ret, NULL); > + assert(bm); You can use &error_abort as the Error object and drop the assert(); or, if you are dropping the Error parameter, there is no need to check the return value at all, because it will always be non-NULL (there won't be any code path in the function returning NULL at all). Maybe you can even drop the return value, too. I just looked through the series: Actually, you're never using the Error parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you really should drop it (and maybe the return value along with it). > + } > + > block_job_completed(job, data->ret); > g_free(data); > } > @@ -419,19 +425,6 @@ leave: > qemu_co_rwlock_wrlock(&job->flush_rwlock); > qemu_co_rwlock_unlock(&job->flush_rwlock); > > - if (job->sync_bitmap) { > - BdrvDirtyBitmap *bm; > - if (ret < 0) { > - /* Merge the successor back into the parent, delete nothing. */ > - bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); > - assert(bm); > - bdrv_enable_dirty_bitmap(job->sync_bitmap); Hm, what is that function call doing here? It feels like it shouldn't have been part of your transactionless series (because other than this, there is no caller of bdrv_{en,dis}able_dirty_bitmap() at all). (You're silently removing it here; removing it is fine, but I guess it shouldn't have been there in the first place) > - } else { > - /* Everything is fine, delete this bitmap and install the backup. */ > - bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); > - assert(bm); > - } > - } > hbitmap_free(job->bitmap); > > bdrv_iostatus_disable(target); > @@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > > error: > if (sync_bitmap) { > - bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); > + BdrvDirtyBitmap *ret; > + ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL); > + assert(ret); > } > } > diff --git a/include/block/block.h b/include/block/block.h > index 3a85690..d7859a7 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); > int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap, > Error **errp); > -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > - BdrvDirtyBitmap *bitmap, > - Error **errp); > -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > - BdrvDirtyBitmap *bitmap, > - Error **errp); > +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, > + BdrvDirtyBitmap *parent, > + int ret, > + Error **errp); > +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent); > BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, > const char *name); > void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap); I'm happy with this patch if you drop the Error parameter from bdrv_dirty_bitmap_decref() (possibly dropping the return value, too) and turn all the error cases into assertions. Max
On 03/17/2015 02:44 PM, Max Reitz wrote: > On 2015-03-04 at 23:15, John Snow wrote: >> Allow bitmap successors to carry reference counts. >> >> We can in a later patch use this ability to clean up the dirty bitmap >> according to both the individual job's success and the success of all >> jobs in the transaction group. >> >> The code for cleaning up a bitmap is also moved from backup_run to >> backup_complete. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block.c | 79 >> +++++++++++++++++++++++++++++++++++++++++++++++---- >> block/backup.c | 23 ++++++--------- >> include/block/block.h | 11 ++++--- >> 3 files changed, 87 insertions(+), 26 deletions(-) >> >> diff --git a/block.c b/block.c >> index 5eaa874..a0036af 100644 >> --- a/block.c >> +++ b/block.c >> @@ -51,6 +51,12 @@ >> #include <windows.h> >> #endif >> +typedef enum BitmapSuccessorAction { >> + SUCCESSOR_ACTION_UNDEFINED = 0, >> + SUCCESSOR_ACTION_ABDICATE, >> + SUCCESSOR_ACTION_RECLAIM >> +} BitmapSuccessorAction; > > Maybe making this a QAPI enum can come in handy later. I don't know in > which QMP commands one would use it, but if I could predict the future, > I'd make trillions (or even billions) at the stock market. > > (It's just that it already looks so much alike to a QAPI enum that I > can't help but to think of making it one) > I don't see a reason to expose it, so I won't. We can always add it in later. >> + >> /** >> * A BdrvDirtyBitmap can be in three possible states: >> * (1) successor is false and disabled is false: full r/w mode >> @@ -65,6 +71,8 @@ struct BdrvDirtyBitmap { >> char *name; /* Optional non-empty unique ID */ >> int64_t size; /* Size of the bitmap (Number of >> sectors) */ >> bool disabled; /* Bitmap is read-only */ >> + int successor_refcount; /* Number of active handles to the >> successor */ >> + BitmapSuccessorAction act; /* Action to take on successor upon >> release */ >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> @@ -5508,6 +5516,7 @@ int >> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> /* Install the successor and freeze the parent */ >> bitmap->successor = child; >> + bitmap->successor_refcount = 1; >> return 0; >> } >> @@ -5515,9 +5524,9 @@ int >> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> * For a bitmap with a successor, yield our name to the successor, >> * Delete the old bitmap, and return a handle to the new bitmap. >> */ >> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> - BdrvDirtyBitmap *bitmap, >> - Error **errp) >> +static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> + BdrvDirtyBitmap >> *bitmap, >> + Error **errp) >> { >> char *name; >> BdrvDirtyBitmap *successor = bitmap->successor; >> @@ -5542,9 +5551,9 @@ BdrvDirtyBitmap >> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> * We may wish to re-join the parent and child/successor. >> * The merged parent will be un-frozen, but not explicitly re-enabled. >> */ >> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> - BdrvDirtyBitmap *parent, >> - Error **errp) >> +static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap >> *parent, >> + Error **errp) >> { >> BdrvDirtyBitmap *successor = parent->successor; >> @@ -5563,6 +5572,64 @@ BdrvDirtyBitmap >> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> return parent; >> } >> +static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs, >> + BdrvDirtyBitmap >> *parent, >> + Error **errp) >> +{ >> + if (parent->successor_refcount) { > > I don't know whether you're intending to call this function from > anywhere outside of bdrv_dirty_bitmap_decref(), but if you don't, please > just make this an assert(!parent->successor_refcount). > OK. >> + error_setg(errp, "Cannot free the successor for bitmap '%s', " >> + "because the refcount is non-zero.", parent->name); > > Hm, again with the full stops? *g* > Can we make checkpatch check for this? I am serious. I can't stop myself from doing this.\n >> + return NULL; >> + } >> + >> + switch (parent->act) { >> + case SUCCESSOR_ACTION_UNDEFINED: >> + error_setg(errp, "Cannot free the successor for bitmap '%s', " >> + "because the successor action has not yet been set.", > > Indeed, again with the full stops. ;-) > >> + parent->name); >> + return NULL; > > So you're (for now) only calling this function from > bdrv_dirty_bitmap_decref(), and that function always makes sure that > parent->act is set to SUCCESSOR_ACTION_{RECLAIM,ABDICATE}. Why not add > an assert(parent->act != SUCCESSOR_ACTION_UNDEFINED) before the switch? > (If that causes problems in regards to compiler warnings or something > (not having all enum values covered in the switch), just add an > assert(0); under "case SUCCESSOR_ACTION_UNDEFINED:".) > OK, sure. >> + case SUCCESSOR_ACTION_RECLAIM: >> + return bdrv_reclaim_dirty_bitmap(bs, parent, errp); >> + case SUCCESSOR_ACTION_ABDICATE: >> + return bdrv_dirty_bitmap_abdicate(bs, parent, errp); >> + default: >> + error_setg(errp, >> + "Unrecognized successor action (%d), " >> + "cannot free successor for bitmap '%s'", >> + parent->act, parent->name); >> + return NULL; > > This can be made an assert(0), a g_assert_not_reached(), or simply an > abort() (with the latter probably being the preferred way). > > So I think that all the error_setg() calls are actually cases that > should never happen. Better make them abort() and drop the Error > parameter (because *_decref() and *_free() functions normally (for good > reason) don't have Error parameters...). > Uhh, yeah. I'll harden these functions to be meaner about failing. >> + } >> +} >> + >> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, > > I don't know whether I'm that content with the name chosen, because > you're actually decrementing the refcount of the successor; but since > the successor is basically a clone of the original bitmap (and I mean in > the Star Trek sense, that it's a teleported clone and the original is > intended to be destroyed so the successor can replace it), decrementing > the refcount of the successor basically is equal to decrementing the > refcount of the bitmap itself (as long as there is a successor, which > you are asserting; maybe you want to add a comment about that to > include/block/block.h, that one can only use this on frozen bitmaps?). > Actual struggle I had when naming it myself. Went with the simpler answer that was less accurate. I might just augment with a comment explaining what exactly is being decremented. >> + BdrvDirtyBitmap *parent, >> + int ret, >> + Error **errp) >> +{ >> + assert(bdrv_dirty_bitmap_frozen(parent)); >> + assert(parent->successor); >> + >> + if (ret) { >> + parent->act = SUCCESSOR_ACTION_RECLAIM; >> + } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) { >> + parent->act = SUCCESSOR_ACTION_ABDICATE; >> + } >> + >> + parent->successor_refcount--; >> + if (parent->successor_refcount == 0) { >> + return bdrv_free_bitmap_successor(bs, parent, errp); > > If you drop the Error parameter from bdrv_free_bitmap_successor(), you > can drop it from this function, too. > Noted. >> + } >> + return parent; >> +} >> + >> +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent) >> +{ >> + assert(bdrv_dirty_bitmap_frozen(parent)); >> + assert(parent->successor); >> + >> + parent->successor_refcount++; >> +} >> + >> static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t >> size) >> { >> /* Should only be frozen during a block backup job, which should >> have >> diff --git a/block/backup.c b/block/backup.c >> index 41bd9af..4332df4 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void >> *opaque) >> bdrv_unref(s->target); >> + if (s->sync_bitmap) { >> + BdrvDirtyBitmap *bm; >> + bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, >> data->ret, NULL); >> + assert(bm); > > You can use &error_abort as the Error object and drop the assert(); or, > if you are dropping the Error parameter, there is no need to check the > return value at all, because it will always be non-NULL (there won't be > any code path in the function returning NULL at all). Maybe you can even > drop the return value, too. > > I just looked through the series: Actually, you're never using the Error > parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you > really should drop it (and maybe the return value along with it). > Return code is there for future usages, I suppose. I wanted to carry it around as a "generic" thing. In this one usage, we don't wind up needing it, but I wanted to make an allowance for it in this API. I'll trim down the soft errors. >> + } >> + >> block_job_completed(job, data->ret); >> g_free(data); >> } >> @@ -419,19 +425,6 @@ leave: >> qemu_co_rwlock_wrlock(&job->flush_rwlock); >> qemu_co_rwlock_unlock(&job->flush_rwlock); >> - if (job->sync_bitmap) { >> - BdrvDirtyBitmap *bm; >> - if (ret < 0) { >> - /* Merge the successor back into the parent, delete >> nothing. */ >> - bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); >> - assert(bm); >> - bdrv_enable_dirty_bitmap(job->sync_bitmap); > > Hm, what is that function call doing here? It feels like it shouldn't > have been part of your transactionless series (because other than this, > there is no caller of bdrv_{en,dis}able_dirty_bitmap() at all). > Originally it was to re-enable a bitmap after it was frozen for the operation, but that concept has long since been ditched, so it's just a NOP right now, basically. I'll try to pull it out of the original series. > (You're silently removing it here; removing it is fine, but I guess it > shouldn't have been there in the first place) > >> - } else { >> - /* Everything is fine, delete this bitmap and install the >> backup. */ >> - bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); >> - assert(bm); >> - } >> - } >> hbitmap_free(job->bitmap); >> bdrv_iostatus_disable(target); >> @@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs, >> BlockDriverState *target, >> error: >> if (sync_bitmap) { >> - bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); >> + BdrvDirtyBitmap *ret; >> + ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL); >> + assert(ret); >> } >> } >> diff --git a/include/block/block.h b/include/block/block.h >> index 3a85690..d7859a7 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState >> *bs); >> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, >> Error **errp); >> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> - BdrvDirtyBitmap *bitmap, >> - Error **errp); >> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> - BdrvDirtyBitmap *bitmap, >> - Error **errp); >> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, >> + BdrvDirtyBitmap *parent, >> + int ret, >> + Error **errp); >> +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent); >> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, >> const char *name); >> void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap); > > I'm happy with this patch if you drop the Error parameter from > bdrv_dirty_bitmap_decref() (possibly dropping the return value, too) and > turn all the error cases into assertions. > > Max > Okie-dokey. --js
On 03/17/2015 02:44 PM, Max Reitz wrote: > On 2015-03-04 at 23:15, John Snow wrote: >> Allow bitmap successors to carry reference counts. >> >> We can in a later patch use this ability to clean up the dirty bitmap >> according to both the individual job's success and the success of all >> jobs in the transaction group. >> >> The code for cleaning up a bitmap is also moved from backup_run to >> backup_complete. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block.c | 79 >> +++++++++++++++++++++++++++++++++++++++++++++++---- >> block/backup.c | 23 ++++++--------- >> include/block/block.h | 11 ++++--- >> 3 files changed, 87 insertions(+), 26 deletions(-) >> >> diff --git a/block.c b/block.c >> index 5eaa874..a0036af 100644 >> --- a/block.c >> +++ b/block.c >> @@ -51,6 +51,12 @@ >> #include <windows.h> >> #endif >> +typedef enum BitmapSuccessorAction { >> + SUCCESSOR_ACTION_UNDEFINED = 0, >> + SUCCESSOR_ACTION_ABDICATE, >> + SUCCESSOR_ACTION_RECLAIM >> +} BitmapSuccessorAction; > > Maybe making this a QAPI enum can come in handy later. I don't know in > which QMP commands one would use it, but if I could predict the future, > I'd make trillions (or even billions) at the stock market. > > (It's just that it already looks so much alike to a QAPI enum that I > can't help but to think of making it one) > >> + >> /** >> * A BdrvDirtyBitmap can be in three possible states: >> * (1) successor is false and disabled is false: full r/w mode >> @@ -65,6 +71,8 @@ struct BdrvDirtyBitmap { >> char *name; /* Optional non-empty unique ID */ >> int64_t size; /* Size of the bitmap (Number of >> sectors) */ >> bool disabled; /* Bitmap is read-only */ >> + int successor_refcount; /* Number of active handles to the >> successor */ >> + BitmapSuccessorAction act; /* Action to take on successor upon >> release */ >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> @@ -5508,6 +5516,7 @@ int >> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> /* Install the successor and freeze the parent */ >> bitmap->successor = child; >> + bitmap->successor_refcount = 1; >> return 0; >> } >> @@ -5515,9 +5524,9 @@ int >> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> * For a bitmap with a successor, yield our name to the successor, >> * Delete the old bitmap, and return a handle to the new bitmap. >> */ >> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> - BdrvDirtyBitmap *bitmap, >> - Error **errp) >> +static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> + BdrvDirtyBitmap >> *bitmap, >> + Error **errp) >> { >> char *name; >> BdrvDirtyBitmap *successor = bitmap->successor; >> @@ -5542,9 +5551,9 @@ BdrvDirtyBitmap >> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> * We may wish to re-join the parent and child/successor. >> * The merged parent will be un-frozen, but not explicitly re-enabled. >> */ >> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> - BdrvDirtyBitmap *parent, >> - Error **errp) >> +static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap >> *parent, >> + Error **errp) >> { >> BdrvDirtyBitmap *successor = parent->successor; >> @@ -5563,6 +5572,64 @@ BdrvDirtyBitmap >> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> return parent; >> } >> +static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs, >> + BdrvDirtyBitmap >> *parent, >> + Error **errp) >> +{ >> + if (parent->successor_refcount) { > > I don't know whether you're intending to call this function from > anywhere outside of bdrv_dirty_bitmap_decref(), but if you don't, please > just make this an assert(!parent->successor_refcount). > No, deleted. >> + error_setg(errp, "Cannot free the successor for bitmap '%s', " >> + "because the refcount is non-zero.", parent->name); > > Hm, again with the full stops? *g* > Erased error message altogether. >> + return NULL; >> + } >> + >> + switch (parent->act) { >> + case SUCCESSOR_ACTION_UNDEFINED: >> + error_setg(errp, "Cannot free the successor for bitmap '%s', " >> + "because the successor action has not yet been set.", > > Indeed, again with the full stops. ;-) > And this one. >> + parent->name); >> + return NULL; > > So you're (for now) only calling this function from > bdrv_dirty_bitmap_decref(), and that function always makes sure that > parent->act is set to SUCCESSOR_ACTION_{RECLAIM,ABDICATE}. Why not add > an assert(parent->act != SUCCESSOR_ACTION_UNDEFINED) before the switch? > (If that causes problems in regards to compiler warnings or something > (not having all enum values covered in the switch), just add an > assert(0); under "case SUCCESSOR_ACTION_UNDEFINED:".) > Will use g_assert_not_reached() because I like the way it reads. >> + case SUCCESSOR_ACTION_RECLAIM: >> + return bdrv_reclaim_dirty_bitmap(bs, parent, errp); >> + case SUCCESSOR_ACTION_ABDICATE: >> + return bdrv_dirty_bitmap_abdicate(bs, parent, errp); >> + default: >> + error_setg(errp, >> + "Unrecognized successor action (%d), " >> + "cannot free successor for bitmap '%s'", >> + parent->act, parent->name); >> + return NULL; > > This can be made an assert(0), a g_assert_not_reached(), or simply an > abort() (with the latter probably being the preferred way). > > So I think that all the error_setg() calls are actually cases that > should never happen. Better make them abort() and drop the Error > parameter (because *_decref() and *_free() functions normally (for good > reason) don't have Error parameters...). > The problem all stems from when I made abdicate and reclaim callable interfaces in the original patch series, so they carried errp pointers and I let them flow outwards from there. So, the transactionless set is still on-list, so I could clean those interfaces too, but I suppose I wanted to see how this series was received before I squashed too many things back into the v1 ... I'd rather leave that series be for obvious reasons. >> + } >> +} >> + >> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, > > I don't know whether I'm that content with the name chosen, because > you're actually decrementing the refcount of the successor; but since > the successor is basically a clone of the original bitmap (and I mean in > the Star Trek sense, that it's a teleported clone and the original is > intended to be destroyed so the successor can replace it), decrementing > the refcount of the successor basically is equal to decrementing the > refcount of the bitmap itself (as long as there is a successor, which > you are asserting; maybe you want to add a comment about that to > include/block/block.h, that one can only use this on frozen bitmaps?). > I could get clever with the name and call it bdrv_frozen_bitmap_decref, which hopefully still shows membership to the bdrv_dirty_bitmap class of functions but clarifies its usage sufficiently. >> + BdrvDirtyBitmap *parent, >> + int ret, >> + Error **errp) >> +{ >> + assert(bdrv_dirty_bitmap_frozen(parent)); >> + assert(parent->successor); >> + >> + if (ret) { >> + parent->act = SUCCESSOR_ACTION_RECLAIM; >> + } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) { >> + parent->act = SUCCESSOR_ACTION_ABDICATE; >> + } >> + >> + parent->successor_refcount--; >> + if (parent->successor_refcount == 0) { >> + return bdrv_free_bitmap_successor(bs, parent, errp); > > If you drop the Error parameter from bdrv_free_bitmap_successor(), you > can drop it from this function, too. > Done. >> + } >> + return parent; >> +} >> + >> +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent) >> +{ >> + assert(bdrv_dirty_bitmap_frozen(parent)); >> + assert(parent->successor); >> + >> + parent->successor_refcount++; >> +} >> + >> static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t >> size) >> { >> /* Should only be frozen during a block backup job, which should >> have >> diff --git a/block/backup.c b/block/backup.c >> index 41bd9af..4332df4 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void >> *opaque) >> bdrv_unref(s->target); >> + if (s->sync_bitmap) { >> + BdrvDirtyBitmap *bm; >> + bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, >> data->ret, NULL); >> + assert(bm); > > You can use &error_abort as the Error object and drop the assert(); or, > if you are dropping the Error parameter, there is no need to check the > return value at all, because it will always be non-NULL (there won't be > any code path in the function returning NULL at all). Maybe you can even > drop the return value, too. > > I just looked through the series: Actually, you're never using the Error > parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you > really should drop it (and maybe the return value along with it). > I actually use this parameter to return the "new bitmap" (if any) after the decrement operation. I wanted to leave the window open for merge optimizations, so I tell the caller which bitmap remains after the operation. I will cull the errp, but will likely leave the return code. >> + } >> + >> block_job_completed(job, data->ret); >> g_free(data); >> } >> @@ -419,19 +425,6 @@ leave: >> qemu_co_rwlock_wrlock(&job->flush_rwlock); >> qemu_co_rwlock_unlock(&job->flush_rwlock); >> - if (job->sync_bitmap) { >> - BdrvDirtyBitmap *bm; >> - if (ret < 0) { >> - /* Merge the successor back into the parent, delete >> nothing. */ >> - bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); >> - assert(bm); >> - bdrv_enable_dirty_bitmap(job->sync_bitmap); > > Hm, what is that function call doing here? It feels like it shouldn't > have been part of your transactionless series (because other than this, > there is no caller of bdrv_{en,dis}able_dirty_bitmap() at all). > > (You're silently removing it here; removing it is fine, but I guess it > shouldn't have been there in the first place) > Fixed in v4 for the transactionless series. >> - } else { >> - /* Everything is fine, delete this bitmap and install the >> backup. */ >> - bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); >> - assert(bm); >> - } >> - } >> hbitmap_free(job->bitmap); >> bdrv_iostatus_disable(target); >> @@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs, >> BlockDriverState *target, >> error: >> if (sync_bitmap) { >> - bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); >> + BdrvDirtyBitmap *ret; >> + ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL); >> + assert(ret); >> } >> } >> diff --git a/include/block/block.h b/include/block/block.h >> index 3a85690..d7859a7 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState >> *bs); >> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, >> Error **errp); >> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> - BdrvDirtyBitmap *bitmap, >> - Error **errp); >> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> - BdrvDirtyBitmap *bitmap, >> - Error **errp); >> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, >> + BdrvDirtyBitmap *parent, >> + int ret, >> + Error **errp); >> +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent); >> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, >> const char *name); >> void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap); > > I'm happy with this patch if you drop the Error parameter from > bdrv_dirty_bitmap_decref() (possibly dropping the return value, too) and > turn all the error cases into assertions. > > Max >
On 2015-03-17 at 18:46, John Snow wrote: > > > On 03/17/2015 02:44 PM, Max Reitz wrote: >> On 2015-03-04 at 23:15, John Snow wrote: [snip] >>> + } >>> +} >>> + >>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, >> >> I don't know whether I'm that content with the name chosen, because >> you're actually decrementing the refcount of the successor; but since >> the successor is basically a clone of the original bitmap (and I mean in >> the Star Trek sense, that it's a teleported clone and the original is >> intended to be destroyed so the successor can replace it), decrementing >> the refcount of the successor basically is equal to decrementing the >> refcount of the bitmap itself (as long as there is a successor, which >> you are asserting; maybe you want to add a comment about that to >> include/block/block.h, that one can only use this on frozen bitmaps?). >> > > I could get clever with the name and call it > bdrv_frozen_bitmap_decref, which hopefully still shows membership to > the bdrv_dirty_bitmap class of functions but clarifies its usage > sufficiently. Sounds good to me. [snip] >>> diff --git a/block/backup.c b/block/backup.c >>> index 41bd9af..4332df4 100644 >>> --- a/block/backup.c >>> +++ b/block/backup.c >>> @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void >>> *opaque) >>> bdrv_unref(s->target); >>> + if (s->sync_bitmap) { >>> + BdrvDirtyBitmap *bm; >>> + bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, >>> data->ret, NULL); >>> + assert(bm); >> >> You can use &error_abort as the Error object and drop the assert(); or, >> if you are dropping the Error parameter, there is no need to check the >> return value at all, because it will always be non-NULL (there won't be >> any code path in the function returning NULL at all). Maybe you can even >> drop the return value, too. >> >> I just looked through the series: Actually, you're never using the Error >> parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you >> really should drop it (and maybe the return value along with it). >> > > I actually use this parameter to return the "new bitmap" (if any) > after the decrement operation. I wanted to leave the window open for > merge optimizations, so I tell the caller which bitmap remains after > the operation. > > I will cull the errp, but will likely leave the return code. OK. Max
diff --git a/block.c b/block.c index 5eaa874..a0036af 100644 --- a/block.c +++ b/block.c @@ -51,6 +51,12 @@ #include <windows.h> #endif +typedef enum BitmapSuccessorAction { + SUCCESSOR_ACTION_UNDEFINED = 0, + SUCCESSOR_ACTION_ABDICATE, + SUCCESSOR_ACTION_RECLAIM +} BitmapSuccessorAction; + /** * A BdrvDirtyBitmap can be in three possible states: * (1) successor is false and disabled is false: full r/w mode @@ -65,6 +71,8 @@ struct BdrvDirtyBitmap { char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap (Number of sectors) */ bool disabled; /* Bitmap is read-only */ + int successor_refcount; /* Number of active handles to the successor */ + BitmapSuccessorAction act; /* Action to take on successor upon release */ QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -5508,6 +5516,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, /* Install the successor and freeze the parent */ bitmap->successor = child; + bitmap->successor_refcount = 1; return 0; } @@ -5515,9 +5524,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, * For a bitmap with a successor, yield our name to the successor, * Delete the old bitmap, and return a handle to the new bitmap. */ -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, - Error **errp) +static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + Error **errp) { char *name; BdrvDirtyBitmap *successor = bitmap->successor; @@ -5542,9 +5551,9 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, * We may wish to re-join the parent and child/successor. * The merged parent will be un-frozen, but not explicitly re-enabled. */ -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, - BdrvDirtyBitmap *parent, - Error **errp) +static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *parent, + Error **errp) { BdrvDirtyBitmap *successor = parent->successor; @@ -5563,6 +5572,64 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, return parent; } +static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs, + BdrvDirtyBitmap *parent, + Error **errp) +{ + if (parent->successor_refcount) { + error_setg(errp, "Cannot free the successor for bitmap '%s', " + "because the refcount is non-zero.", parent->name); + return NULL; + } + + switch (parent->act) { + case SUCCESSOR_ACTION_UNDEFINED: + error_setg(errp, "Cannot free the successor for bitmap '%s', " + "because the successor action has not yet been set.", + parent->name); + return NULL; + case SUCCESSOR_ACTION_RECLAIM: + return bdrv_reclaim_dirty_bitmap(bs, parent, errp); + case SUCCESSOR_ACTION_ABDICATE: + return bdrv_dirty_bitmap_abdicate(bs, parent, errp); + default: + error_setg(errp, + "Unrecognized successor action (%d), " + "cannot free successor for bitmap '%s'", + parent->act, parent->name); + return NULL; + } +} + +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, + BdrvDirtyBitmap *parent, + int ret, + Error **errp) +{ + assert(bdrv_dirty_bitmap_frozen(parent)); + assert(parent->successor); + + if (ret) { + parent->act = SUCCESSOR_ACTION_RECLAIM; + } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) { + parent->act = SUCCESSOR_ACTION_ABDICATE; + } + + parent->successor_refcount--; + if (parent->successor_refcount == 0) { + return bdrv_free_bitmap_successor(bs, parent, errp); + } + return parent; +} + +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent) +{ + assert(bdrv_dirty_bitmap_frozen(parent)); + assert(parent->successor); + + parent->successor_refcount++; +} + static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size) { /* Should only be frozen during a block backup job, which should have diff --git a/block/backup.c b/block/backup.c index 41bd9af..4332df4 100644 --- a/block/backup.c +++ b/block/backup.c @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque) bdrv_unref(s->target); + if (s->sync_bitmap) { + BdrvDirtyBitmap *bm; + bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, data->ret, NULL); + assert(bm); + } + block_job_completed(job, data->ret); g_free(data); } @@ -419,19 +425,6 @@ leave: qemu_co_rwlock_wrlock(&job->flush_rwlock); qemu_co_rwlock_unlock(&job->flush_rwlock); - if (job->sync_bitmap) { - BdrvDirtyBitmap *bm; - if (ret < 0) { - /* Merge the successor back into the parent, delete nothing. */ - bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); - assert(bm); - bdrv_enable_dirty_bitmap(job->sync_bitmap); - } else { - /* Everything is fine, delete this bitmap and install the backup. */ - bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); - assert(bm); - } - } hbitmap_free(job->bitmap); bdrv_iostatus_disable(target); @@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, error: if (sync_bitmap) { - bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); + BdrvDirtyBitmap *ret; + ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL); + assert(ret); } } diff --git a/include/block/block.h b/include/block/block.h index 3a85690..d7859a7 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp); -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, - Error **errp); -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, - Error **errp); +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs, + BdrvDirtyBitmap *parent, + int ret, + Error **errp); +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent); BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name); void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
Allow bitmap successors to carry reference counts. We can in a later patch use this ability to clean up the dirty bitmap according to both the individual job's success and the success of all jobs in the transaction group. The code for cleaning up a bitmap is also moved from backup_run to backup_complete. Signed-off-by: John Snow <jsnow@redhat.com> --- block.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++---- block/backup.c | 23 ++++++--------- include/block/block.h | 11 ++++--- 3 files changed, 87 insertions(+), 26 deletions(-)