From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58519) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXwux-0005jL-Op for qemu-devel@nongnu.org; Tue, 17 Mar 2015 15:12:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXwuv-0003eQ-Ps for qemu-devel@nongnu.org; Tue, 17 Mar 2015 15:12:43 -0400 Message-ID: <55087C9F.50503@redhat.com> Date: Tue, 17 Mar 2015 15:12:31 -0400 From: John Snow MIME-Version: 1.0 References: <1425528911-10300-1-git-send-email-jsnow@redhat.com> <1425528911-10300-6-git-send-email-jsnow@redhat.com> <55087624.5040601@redhat.com> In-Reply-To: <55087624.5040601@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/11] block: add delayed bitmap successor cleanup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com 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 >> --- >> 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 >> #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