| Message ID | 1446663467-22485-4-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/04/2015 01:57 PM, Max Reitz wrote: > bdrv_delete() is not very happy about deleting BlockDriverStates with > dirty bitmaps still attached to them. In the past, we got around that > very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and > bdrv_close() simply ignoring that condition. We should fix that by > releasing all dirty bitmaps in bdrv_close() and drop the assertion in > bdrv_delete(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 3493501..23448ed 100644 > --- a/block.c > +++ b/block.c > @@ -87,6 +87,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > const BdrvChildRole *child_role, Error **errp); > > static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); > +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs); > + > /* If non-zero, use only whitelisted block drivers */ > static int use_bdrv_whitelist; > > @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs) > bdrv_drain(bs); /* in case flush left pending I/O */ > notifier_list_notify(&bs->close_notifiers, bs); > > + bdrv_release_all_dirty_bitmaps(bs); > + > if (bs->blk) { > blk_dev_change_media_cb(bs->blk, false); > } > @@ -2123,7 +2127,6 @@ static void bdrv_delete(BlockDriverState *bs) > assert(!bs->job); > assert(bdrv_op_blocker_is_empty(bs)); > assert(!bs->refcnt); > - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); > > bdrv_close(bs); > > @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > } > } > > +/** > + * Release all dirty bitmaps attached to a BDS, independently of whether they > + * are frozen or not (for use in bdrv_close()). > + */ This comment caught my attention ... > +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs) > +{ > + BdrvDirtyBitmap *bm, *next; > + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { > + QLIST_REMOVE(bm, list); > + hbitmap_free(bm->bitmap); > + g_free(bm->name); > + g_free(bm); > + } > +} > + > void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > { > assert(!bdrv_dirty_bitmap_frozen(bitmap)); > Currently, the only way a bitmap could/should be frozen is if it is in use by a job. If a job is running, you probably shouldn't delete stuff it is using out from under it. I am assuming by now that it's actually likely that you've canceled any jobs attached to this node before you go through the motions of deleting it, and it should be safe to just call bdrv_release_dirty_bitmap ... We don't want the two foreach loops though, so maybe just factor out a helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap can share. You can leave the frozen assertion in just bdrv_release_dirty_bitmap before it invokes the helper, so that bdrv_delete always succeeds in case something gets out-of-sync in the internals. (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you should probably call the helper on its child object too... or just make the helper recursive.)
Am 04.11.2015 um 19:57 hat Max Reitz geschrieben: > bdrv_delete() is not very happy about deleting BlockDriverStates with > dirty bitmaps still attached to them. In the past, we got around that > very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and > bdrv_close() simply ignoring that condition. We should fix that by > releasing all dirty bitmaps in bdrv_close() This doesn't sound right. If there was a dirty bitmap, there must be a user associated with it. Now that we simply free the bitmap, that user has a dangling pointer. An exception would be if we knew that the only "user" of this bitmap is the monitor because the monitor doesn't actually maintain its own list of bitmaps. However, it's doubtful whether bdrv_close() should remove something that the QMP client added explicitly. > and drop the assertion in bdrv_delete(). Why? It should still hold true. Kevin
On 06.11.2015 19:59, John Snow wrote: > > > On 11/04/2015 01:57 PM, Max Reitz wrote: >> bdrv_delete() is not very happy about deleting BlockDriverStates with >> dirty bitmaps still attached to them. In the past, we got around that >> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and >> bdrv_close() simply ignoring that condition. We should fix that by >> releasing all dirty bitmaps in bdrv_close() and drop the assertion in >> bdrv_delete(). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/block.c b/block.c >> index 3493501..23448ed 100644 >> --- a/block.c >> +++ b/block.c >> @@ -87,6 +87,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, >> const BdrvChildRole *child_role, Error **errp); >> >> static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs); >> + >> /* If non-zero, use only whitelisted block drivers */ >> static int use_bdrv_whitelist; >> >> @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs) >> bdrv_drain(bs); /* in case flush left pending I/O */ >> notifier_list_notify(&bs->close_notifiers, bs); >> >> + bdrv_release_all_dirty_bitmaps(bs); >> + >> if (bs->blk) { >> blk_dev_change_media_cb(bs->blk, false); >> } >> @@ -2123,7 +2127,6 @@ static void bdrv_delete(BlockDriverState *bs) >> assert(!bs->job); >> assert(bdrv_op_blocker_is_empty(bs)); >> assert(!bs->refcnt); >> - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >> >> bdrv_close(bs); >> >> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) >> } >> } >> >> +/** >> + * Release all dirty bitmaps attached to a BDS, independently of whether they >> + * are frozen or not (for use in bdrv_close()). >> + */ > > This comment caught my attention ... > >> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs) >> +{ >> + BdrvDirtyBitmap *bm, *next; >> + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >> + QLIST_REMOVE(bm, list); >> + hbitmap_free(bm->bitmap); >> + g_free(bm->name); >> + g_free(bm); >> + } >> +} >> + >> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> { >> assert(!bdrv_dirty_bitmap_frozen(bitmap)); >> > > Currently, the only way a bitmap could/should be frozen is if it is in > use by a job. If a job is running, you probably shouldn't delete stuff > it is using out from under it. Right. Good thing I'm fixing everything and bdrv_close() will no longer be called unless there are no references to the BDS anymore (and each block job holds an own reference to its BDS). :-) (At least that's how it's supposed to be; also, there is an assert(!bs->job) in bdrv_close()) Also, I wanted to mirror current behavior. Right now, we are just leaking all dirty bitmaps on bdrv_close() (bdrv_delete() asserts that there are none, but if you invoke bdrv_close() directly...). > I am assuming by now that it's actually likely that you've canceled any > jobs attached to this node before you go through the motions of deleting > it, and it should be safe to just call bdrv_release_dirty_bitmap ... Well, yes, but then I'd have to iterate through all the dirty bitmaps to call bdrv_release_dirty_bitmap() for each of them, and then bdrv_release_dirty_bitmap() iterates through all of them again to check whether it really belongs to the BDS. That seemed a bit like a waste. > We don't want the two foreach loops though, so maybe just factor out a > helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap > can share. You can leave the frozen assertion > in just bdrv_release_dirty_bitmap before it invokes the helper, so that > bdrv_delete always succeeds in case something gets out-of-sync in the > internals. Hm, yes, that should do just fine, thanks. > (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you > should probably call the helper on its child object too... or just make > the helper recursive.) I think it'll be actually better to just keep the assertion in and thus not allow releasing frozen dirty bitmaps in bdrv_close(). In addition I'll add an assertion that !bs->refcount to bdrv_close(). Max
On 09.11.2015 17:04, Kevin Wolf wrote: > Am 04.11.2015 um 19:57 hat Max Reitz geschrieben: >> bdrv_delete() is not very happy about deleting BlockDriverStates with >> dirty bitmaps still attached to them. In the past, we got around that >> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and >> bdrv_close() simply ignoring that condition. We should fix that by >> releasing all dirty bitmaps in bdrv_close() > > This doesn't sound right. If there was a dirty bitmap, there must be a > user associated with it. Now that we simply free the bitmap, that user > has a dangling pointer. Well, having an assertion there means that we already assumed that case to be impossible. Even though it isn't, as you yourself describe: > An exception would be if we knew that the only "user" of this bitmap is > the monitor because the monitor doesn't actually maintain its own list > of bitmaps. However, it's doubtful whether bdrv_close() should remove > something that the QMP client added explicitly. So you are proposing that bdrv_close() should fail if there are still dirty bitmaps attached? I don't like that either. The bitmaps are attached to the BDS, that much is exposed over QMP, too. If the BDS is released it's only natural to assume that all its bitmaps are released, too. If you don't want that, you need to make sure that the monitor has a reference to the BDS itself so the user can defer the call to blockdev-del until he's/she's ready. Maybe we need some QMP command to fetch a reference for the monitor for that to be more usable, I don't know. It will work with blockdev-add alone, too, though. >> and drop the assertion in bdrv_delete(). > > Why? It should still hold true. But it does not for user-added bitmaps. Actually, on master, you can't break that assertion by adding a bitmap through QMP, but with the BB and media series, you can. And that's because as of that series, eject will no longer force-call bdrv_close() (which bypasses the assertion althogether) but bdrv_unref() instead (leading to bdrv_delete(), and that gets you the assertion). I'm not really keen on fixing that in that series, though, since I consider leaking not much better than a failed assertion, especially considering I'm trying to actually fix it here anyway. Max
On 09.11.2015 17:21, Max Reitz wrote: > On 06.11.2015 19:59, John Snow wrote: >> >> >> On 11/04/2015 01:57 PM, Max Reitz wrote: >>> bdrv_delete() is not very happy about deleting BlockDriverStates with >>> dirty bitmaps still attached to them. In the past, we got around that >>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and >>> bdrv_close() simply ignoring that condition. We should fix that by >>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in >>> bdrv_delete(). >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block.c | 20 +++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/block.c b/block.c >>> index 3493501..23448ed 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -87,6 +87,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, >>> const BdrvChildRole *child_role, Error **errp); >>> >>> static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs); >>> + >>> /* If non-zero, use only whitelisted block drivers */ >>> static int use_bdrv_whitelist; >>> >>> @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs) >>> bdrv_drain(bs); /* in case flush left pending I/O */ >>> notifier_list_notify(&bs->close_notifiers, bs); >>> >>> + bdrv_release_all_dirty_bitmaps(bs); >>> + >>> if (bs->blk) { >>> blk_dev_change_media_cb(bs->blk, false); >>> } >>> @@ -2123,7 +2127,6 @@ static void bdrv_delete(BlockDriverState *bs) >>> assert(!bs->job); >>> assert(bdrv_op_blocker_is_empty(bs)); >>> assert(!bs->refcnt); >>> - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >>> >>> bdrv_close(bs); >>> >>> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) >>> } >>> } >>> >>> +/** >>> + * Release all dirty bitmaps attached to a BDS, independently of whether they >>> + * are frozen or not (for use in bdrv_close()). >>> + */ >> >> This comment caught my attention ... >> >>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs) >>> +{ >>> + BdrvDirtyBitmap *bm, *next; >>> + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >>> + QLIST_REMOVE(bm, list); >>> + hbitmap_free(bm->bitmap); >>> + g_free(bm->name); >>> + g_free(bm); >>> + } >>> +} >>> + >>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >>> { >>> assert(!bdrv_dirty_bitmap_frozen(bitmap)); >>> >> >> Currently, the only way a bitmap could/should be frozen is if it is in >> use by a job. If a job is running, you probably shouldn't delete stuff >> it is using out from under it. > > Right. Good thing I'm fixing everything and bdrv_close() will no longer > be called unless there are no references to the BDS anymore (and each > block job holds an own reference to its BDS). :-) > > (At least that's how it's supposed to be; also, there is an > assert(!bs->job) in bdrv_close()) > > Also, I wanted to mirror current behavior. Right now, we are just > leaking all dirty bitmaps on bdrv_close() (bdrv_delete() asserts that > there are none, but if you invoke bdrv_close() directly...). > >> I am assuming by now that it's actually likely that you've canceled any >> jobs attached to this node before you go through the motions of deleting >> it, and it should be safe to just call bdrv_release_dirty_bitmap ... > > Well, yes, but then I'd have to iterate through all the dirty bitmaps to > call bdrv_release_dirty_bitmap() for each of them, and then > bdrv_release_dirty_bitmap() iterates through all of them again to check > whether it really belongs to the BDS. That seemed a bit like a waste. > >> We don't want the two foreach loops though, so maybe just factor out a >> helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap >> can share. You can leave the frozen assertion >> in just bdrv_release_dirty_bitmap before it invokes the helper, so that >> bdrv_delete always succeeds in case something gets out-of-sync in the >> internals. > > Hm, yes, that should do just fine, thanks. > >> (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you >> should probably call the helper on its child object too... or just make >> the helper recursive.) > > I think it'll be actually better to just keep the assertion in and thus > not allow releasing frozen dirty bitmaps in bdrv_close(). In addition > I'll add an assertion that !bs->refcount to bdrv_close(). OK, I can't actually do that yet. There is a bdrv_close() in one of the fail paths of bdrv_open_inherit(), and the refcount of that BDS will probably be 1. There is generally no way that BDS has any bitmaps attached to it, so we're fine in that regard. I'll revisit this after this series and its follow-up. Max
diff --git a/block.c b/block.c index 3493501..23448ed 100644 --- a/block.c +++ b/block.c @@ -87,6 +87,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, const BdrvChildRole *child_role, Error **errp); static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs); + /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs) bdrv_drain(bs); /* in case flush left pending I/O */ notifier_list_notify(&bs->close_notifiers, bs); + bdrv_release_all_dirty_bitmaps(bs); + if (bs->blk) { blk_dev_change_media_cb(bs->blk, false); } @@ -2123,7 +2127,6 @@ static void bdrv_delete(BlockDriverState *bs) assert(!bs->job); assert(bdrv_op_blocker_is_empty(bs)); assert(!bs->refcnt); - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); bdrv_close(bs); @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) } } +/** + * Release all dirty bitmaps attached to a BDS, independently of whether they + * are frozen or not (for use in bdrv_close()). + */ +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs) +{ + BdrvDirtyBitmap *bm, *next; + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { + QLIST_REMOVE(bm, list); + hbitmap_free(bm->bitmap); + g_free(bm->name); + g_free(bm); + } +} + void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { assert(!bdrv_dirty_bitmap_frozen(bitmap));
bdrv_delete() is not very happy about deleting BlockDriverStates with dirty bitmaps still attached to them. In the past, we got around that very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and bdrv_close() simply ignoring that condition. We should fix that by releasing all dirty bitmaps in bdrv_close() and drop the assertion in bdrv_delete(). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)