From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52086) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSvcC-0000Pf-4C for qemu-devel@nongnu.org; Tue, 03 Mar 2015 17:48:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSvc7-0006Rf-3r for qemu-devel@nongnu.org; Tue, 03 Mar 2015 17:48:36 -0500 Message-ID: <54F63A3C.7050409@redhat.com> Date: Tue, 03 Mar 2015 17:48:28 -0500 From: John Snow MIME-Version: 1.0 References: <1425338403-16138-1-git-send-email-jsnow@redhat.com> <1425338403-16138-15-git-send-email-jsnow@redhat.com> <54F5DB10.9010608@redhat.com> <54F62690.5010809@redhat.com> <54F62757.6000207@redhat.com> In-Reply-To: <54F62757.6000207@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate 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/03/2015 04:27 PM, Max Reitz wrote: > On 2015-03-03 at 16:24, John Snow wrote: >> >> >> On 03/03/2015 11:02 AM, Max Reitz wrote: >>> On 2015-03-02 at 18:20, John Snow wrote: >>>> Signed-off-by: John Snow >>>> --- >>>> block.c | 22 ++++++++++++++++++++ >>>> include/block/block.h | 1 + >>>> include/qemu/hbitmap.h | 10 +++++++++ >>>> util/hbitmap.c | 55 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 88 insertions(+) >>>> >>>> diff --git a/block.c b/block.c >>>> index e6b2696..5eaa874 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t >>>> offset) >>>> ret = drv->bdrv_truncate(bs, offset); >>>> if (ret == 0) { >>>> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); >>>> + bdrv_dirty_bitmap_truncate(bs); >>>> if (bs->blk) { >>>> blk_dev_resize_cb(bs->blk); >>>> } >>>> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap >>>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >>>> return parent; >>>> } >>>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t >>>> size) >>>> +{ >>>> + /* Should only be frozen during a block backup job, which should >>>> have >>>> + * blocked any resize actions. */ >>>> + assert(!bdrv_dirty_bitmap_frozen(bitmap)); >>>> + hbitmap_truncate(bitmap->bitmap, size); >>>> +} >>>> + >>>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) >>>> +{ >>>> + BdrvDirtyBitmap *bitmap; >>>> + uint64_t size = bdrv_nb_sectors(bs); >>>> + >>>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >>>> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >>>> + continue; >>>> + } >>> >>> Hm! >>> >>> The assert() above doesn't do anything because this condition will just >>> skip over frozen bitmaps. Maybe we should drop it? >>> >>> I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will always >>> be false anyway (hence the assertion), but it would stand with this >>> block dropped, too. >>> >>> Max >>> >> >> It's there for parity with the other bdrv helpers that perform a >> similar check. I am guarding against, in the future, any potential >> users from deciding to call this function directly for whichever reason. >> >> Requesting an action on a drive that contains bitmaps that are not >> ready for the action is OK. Requesting an action on a bitmap that is >> not ready for the action should never happen, hence the 'documentation >> assert.' >> >> My default action will be to leave it in. > > Technically that's fine (which is why I said my R-b stands), my only > problem with that is that if something really strange happens and we > actually end up with a frozen bitmap here, the action we're taking is > wrong. The bitmap will have the wrong size after the BDS was truncated, > and that's not right. > If something really strange happens I would rather prefer the person making the really strange patch to know sooner rather than later that they're violating some assumptions made for incremental backups. The only way the bitmap can be frozen is currently during a backup, which should prevent any user-facing resize actions from the outset. If that changes, it's important that we know about it. truncating a frozen bitmap is not a circumstance I want to entertain being possible. > I'd find just calling dirty_bitmap_truncate() regardless of the frozen > state more correct, the idea being "We want to resize all bitmaps, > because all bitmaps attached to this BDS need to be resized", and then, > in dirty_bitmap_truncate(), we notice that we cannot resize a frozen > bitmap. But we need to. So the assertion fails. > This is a good point, though. By the above reasoning, it's better to force this whole command to fail exquisitely. Since we already do a soft user check that should suffice up at qmp_drive_resize, we should already be avoiding the assert() under normal operating conditions. I'd rather not re-spin for this one item, though ... If it's fine by everyone else I'll tidy this up in the transaction patch that's going out shortly. > Max >