From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45779) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YT9kx-0006ST-2A for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:54:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YT9kt-00051T-6O for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:54:35 -0500 Message-ID: <54F70E93.7070603@redhat.com> Date: Wed, 04 Mar 2015 08:54:27 -0500 From: Max Reitz 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> <54F63A3C.7050409@redhat.com> In-Reply-To: <54F63A3C.7050409@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: John Snow , 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 2015-03-03 at 17:48, John Snow wrote: > > > 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. Me, too. That's why I'd like an assertion. :-) > The only way the bitmap can be frozen is currently during a backup, > which should prevent any user-facing resize actions from the outset. Right. This is why I'm (grudgingly) fine with it. > 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. That's why I'd like the assertion to do something instead of being skipped silently. :-) >> 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. Right, an assert() is for cases that cannot happen (some call it "for contracts"). > 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. That's completely fine with me. Max