[{"id":1771442,"web_url":"http://patchwork.ozlabs.org/comment/1771442/","msgid":"<26069637-9535-cb59-8920-9d9a7eef78b7@redhat.com>","list_archive_url":null,"date":"2017-09-19T23:00:19","subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","submitter":{"id":64343,"url":"http://patchwork.ozlabs.org/api/people/64343/","name":"John Snow","email":"jsnow@redhat.com"},"content":"On 09/19/2017 04:18 PM, Eric Blake wrote:\n> We've previously fixed several places where we failed to account\n> for possible errors from bdrv_nb_sectors(). Fix another one by\n> making bdrv_dirty_bitmap_truncate() take the new size from the\n> caller instead of querying itself; then adjust the sole caller\n> bdrv_truncate() to pass the size just determined by a successful\n> resize, or to skip the bitmap resize on failure, thus avoiding\n> sizing the bitmaps to -1.\n> \n> Signed-off-by: Eric Blake \n> \n> ---\n> v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]\n> v8: retitle and rework to avoid possibility of secondary failure [John]\n> v7: new patch [Kevin]\n> ---\n> include/block/dirty-bitmap.h | 2 +-\n> block.c | 15 ++++++++++-----\n> block/dirty-bitmap.c | 6 +++---\n> 3 files changed, 14 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h\n> index 8fd842eac9..7a27590047 100644\n> --- a/include/block/dirty-bitmap.h\n> +++ b/include/block/dirty-bitmap.h\n> @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);\n> void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);\n> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);\n> int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);\n> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);\n> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);\n> bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);\n> bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);\n> bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);\n> diff --git a/block.c b/block.c\n> index ee6a48976e..89261a7a53 100644\n> --- a/block.c\n> +++ b/block.c\n> @@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,\n> assert(!(bs->open_flags & BDRV_O_INACTIVE));\n> \n> ret = drv->bdrv_truncate(bs, offset, prealloc, errp);\n> - if (ret == 0) {\n> - ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n> - bdrv_dirty_bitmap_truncate(bs);\n> - bdrv_parent_cb_resize(bs);\n> - atomic_inc(&bs->write_gen);\n> + if (ret < 0) {\n> + return ret;\n> }\n> + ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n> + if (ret < 0) {\n> + error_setg_errno(errp, -ret, \"Could not refresh total sector count\");\n> + } else {\n\nSorry for dragging my feet on this point; if anyone else wants to R-B\nthis I will cede without much of a fuss, but perhaps you can help me\nunderstand.\n\n(Copying some questions I asked Eric on IRC, airing to list for wider\ndiscussion, and also because I had to drive home before the stores\nclosed for the evening)\n\nDo you suspect that almost certainly if bdrv_truncate() fails overall\nthat the image format driver will either unmount the image or become\nread-only?\n\nThere are calls from parallels, qcow, qcow2-refcount, qcow2, raw-format,\nvhdx-log, vhdx plus whichever calls from blk_truncate (jobs, all of the\nsame formats, blockdev, qemu-img)\n\nI'm just trying to picture exactly what's going to happen if we manage\nto resize the drive but then don't resize the bitmap -- say we expand\nthe drive larger but fail to refresh (and fail to resize the bitmap.)\n\nif the block format module does not immediately and dutifully stop all\nfurther writes -- is there anything that stops us from writing to new\nsectors that were beyond EOF previously?\n\nI suppose if *not* that's a bug for callers of bdrv_truncate to allow\nthat kind of monkey business, but if it CAN happen, hbitmap only guards\nagainst such things with an assert (which, IIRC, is not guaranteed to be\non for all builds)\n\n\nSo the question is: \"bdrv_truncate failure is NOT considered recoverable\nin ANY case, is it?\"\n\nIt may possibly be safer to, if the initial truncate request succeeds,\napply a best-effort to the bitmap before returning the error.\n\n> + bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE);\n> + }\n> + bdrv_parent_cb_resize(bs);\n> + atomic_inc(&bs->write_gen);\n> return ret;\n> }\n> \n> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c\n> index 42a55e4a4b..ee164fb518 100644\n> --- a/block/dirty-bitmap.c\n> +++ b/block/dirty-bitmap.c\n> @@ -1,7 +1,7 @@\n> /*\n> * Block Dirty Bitmap\n> *\n> - * Copyright (c) 2016 Red Hat. Inc\n> + * Copyright (c) 2016-2017 Red Hat. Inc\n> *\n> * Permission is hereby granted, free of charge, to any person obtaining a copy\n> * of this software and associated documentation files (the \"Software\"), to deal\n> @@ -302,10 +302,10 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,\n> * Truncates _all_ bitmaps attached to a BDS.\n> * Called with BQL taken.\n> */\n> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)\n> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)\n> {\n> BdrvDirtyBitmap *bitmap;\n> - uint64_t size = bdrv_nb_sectors(bs);\n> + int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE);\n> \n> bdrv_dirty_bitmaps_lock(bs);\n> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jsnow@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxdfB2YVZz9sNw\n\tfor ;\n\tWed, 20 Sep 2017 09:01:20 +1000 (AEST)","from localhost ([::1]:45787 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duRW0-0003Et-W4\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 19:01:17 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:57290)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duRVH-00037h-UE\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 19:00:33 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duRVG-0004xr-Qu\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 19:00:32 -0400","from mx1.redhat.com ([209.132.183.28]:55568)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duRVA-0004se-T5; Tue, 19 Sep 2017 19:00:25 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 471304E4C3;\n\tTue, 19 Sep 2017 23:00:23 +0000 (UTC)","from [10.18.17.130] (dhcp-17-130.bos.redhat.com [10.18.17.130])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 20D406060A;\n\tTue, 19 Sep 2017 23:00:20 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 471304E4C3","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-6-eblake@redhat.com>","From":"John Snow ","Message-ID":"<26069637-9535-cb59-8920-9d9a7eef78b7@redhat.com>","Date":"Tue, 19 Sep 2017 19:00:19 -0400","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170919201910.25656-6-eblake@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tTue, 19 Sep 2017 23:00:23 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, vsementsov@virtuozzo.com, famz@redhat.com,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771505,"web_url":"http://patchwork.ozlabs.org/comment/1771505/","msgid":"<20170920021014.GB18491@lemon>","list_archive_url":null,"date":"2017-09-20T02:10:14","subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Tue, 09/19 19:00, John Snow wrote:\n> \n> \n> On 09/19/2017 04:18 PM, Eric Blake wrote:\n> > We've previously fixed several places where we failed to account\n> > for possible errors from bdrv_nb_sectors(). Fix another one by\n> > making bdrv_dirty_bitmap_truncate() take the new size from the\n> > caller instead of querying itself; then adjust the sole caller\n> > bdrv_truncate() to pass the size just determined by a successful\n> > resize, or to skip the bitmap resize on failure, thus avoiding\n> > sizing the bitmaps to -1.\n> > \n> > Signed-off-by: Eric Blake \n> > \n> > ---\n> > v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]\n> > v8: retitle and rework to avoid possibility of secondary failure [John]\n> > v7: new patch [Kevin]\n> > ---\n> > include/block/dirty-bitmap.h | 2 +-\n> > block.c | 15 ++++++++++-----\n> > block/dirty-bitmap.c | 6 +++---\n> > 3 files changed, 14 insertions(+), 9 deletions(-)\n> > \n> > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h\n> > index 8fd842eac9..7a27590047 100644\n> > --- a/include/block/dirty-bitmap.h\n> > +++ b/include/block/dirty-bitmap.h\n> > @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);\n> > void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);\n> > int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);\n> > int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);\n> > -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);\n> > +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);\n> > bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);\n> > bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);\n> > bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);\n> > diff --git a/block.c b/block.c\n> > index ee6a48976e..89261a7a53 100644\n> > --- a/block.c\n> > +++ b/block.c\n> > @@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,\n> > assert(!(bs->open_flags & BDRV_O_INACTIVE));\n> > \n> > ret = drv->bdrv_truncate(bs, offset, prealloc, errp);\n> > - if (ret == 0) {\n> > - ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n> > - bdrv_dirty_bitmap_truncate(bs);\n> > - bdrv_parent_cb_resize(bs);\n> > - atomic_inc(&bs->write_gen);\n> > + if (ret < 0) {\n> > + return ret;\n> > }\n> > + ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n> > + if (ret < 0) {\n> > + error_setg_errno(errp, -ret, \"Could not refresh total sector count\");\n> > + } else {\n> \n> Sorry for dragging my feet on this point; if anyone else wants to R-B\n> this I will cede without much of a fuss, but perhaps you can help me\n> understand.\n> \n> (Copying some questions I asked Eric on IRC, airing to list for wider\n> discussion, and also because I had to drive home before the stores\n> closed for the evening)\n> \n> Do you suspect that almost certainly if bdrv_truncate() fails overall\n> that the image format driver will either unmount the image or become\n> read-only?\n> \n> There are calls from parallels, qcow, qcow2-refcount, qcow2, raw-format,\n> vhdx-log, vhdx plus whichever calls from blk_truncate (jobs, all of the\n> same formats, blockdev, qemu-img)\n> \n> I'm just trying to picture exactly what's going to happen if we manage\n> to resize the drive but then don't resize the bitmap -- say we expand\n> the drive larger but fail to refresh (and fail to resize the bitmap.)\n> \n> if the block format module does not immediately and dutifully stop all\n> further writes -- is there anything that stops us from writing to new\n> sectors that were beyond EOF previously?\n> \n> I suppose if *not* that's a bug for callers of bdrv_truncate to allow\n> that kind of monkey business, but if it CAN happen, hbitmap only guards\n> against such things with an assert (which, IIRC, is not guaranteed to be\n> on for all builds)\n\nIt's guaranteed since a few hours ago:\n\ncommit 262a69f4282e44426c7a132138581d400053e0a1\nAuthor: Eric Blake \nDate: Mon Sep 11 16:13:20 2017 -0500\n\n osdep.h: Prohibit disabling assert() in supported builds\n\n> \n> \n> So the question is: \"bdrv_truncate failure is NOT considered recoverable\n> in ANY case, is it?\"\n> \n> It may possibly be safer to, if the initial truncate request succeeds,\n> apply a best-effort to the bitmap before returning the error.\n\nLike fallback \"offset\" (or it aligned up to bs cluster size) if\nrefresh_total_sectors() returns error? I think that is okay.\n\nFam","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=famz@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxjtk2C0Nz9sBW\n\tfor ;\n\tWed, 20 Sep 2017 12:12:30 +1000 (AEST)","from localhost ([::1]:46260 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duUV2-0002g8-Dx\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 22:12:28 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:58001)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duUT5-0001mB-AA\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 22:10:28 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duUT4-0003xG-1s\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 22:10:27 -0400","from mx1.redhat.com ([209.132.183.28]:42896)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duUSz-0003uR-HB; Tue, 19 Sep 2017 22:10:21 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 4CF387E459;\n\tWed, 20 Sep 2017 02:10:20 +0000 (UTC)","from localhost (ovpn-12-90.pek2.redhat.com [10.72.12.90])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 59A9B5D6A8;\n\tWed, 20 Sep 2017 02:10:15 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 4CF387E459","Date":"Wed, 20 Sep 2017 10:10:14 +0800","From":"Fam Zheng ","To":"John Snow ","Message-ID":"<20170920021014.GB18491@lemon>","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-6-eblake@redhat.com>\n\t<26069637-9535-cb59-8920-9d9a7eef78b7@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<26069637-9535-cb59-8920-9d9a7eef78b7@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tWed, 20 Sep 2017 02:10:20 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, vsementsov@virtuozzo.com, qemu-block@nongnu.org,\n\tqemu-devel@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771844,"web_url":"http://patchwork.ozlabs.org/comment/1771844/","msgid":"","list_archive_url":null,"date":"2017-09-20T13:11:44","subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/19/2017 09:10 PM, Fam Zheng wrote:\n\n>>\n>> Do you suspect that almost certainly if bdrv_truncate() fails overall\n>> that the image format driver will either unmount the image or become\n>> read-only?\n\nUggh - it feels like I've bitten off more than I can chew with this\npatch - I'm getting bogged down by trying to fix bad behavior in code\nthat is mostly unrelated to the patch at hand, so I don't have a good\nopinion on WHAT is supposed to happen if bdrv_truncate() fails, only\nthat I'm trying to avoid compounding that failure even worse.\n\n>> I suppose if *not* that's a bug for callers of bdrv_truncate to allow\n>> that kind of monkey business, but if it CAN happen, hbitmap only guards\n>> against such things with an assert (which, IIRC, is not guaranteed to be\n>> on for all builds)\n> \n> It's guaranteed since a few hours ago:\n> \n> commit 262a69f4282e44426c7a132138581d400053e0a1\n\nIndeed - but even without my patch, we would have hit the assertion\nfailures when trying to resize the dirty bitmap to -1 when\nbdrv_nb_sectors() fails (which was likely if refresh_total_sectors()\nfailed).\n\n>> So the question is: \"bdrv_truncate failure is NOT considered recoverable\n>> in ANY case, is it?\"\n>>\n>> It may possibly be safer to, if the initial truncate request succeeds,\n>> apply a best-effort to the bitmap before returning the error.\n> \n> Like fallback \"offset\" (or it aligned up to bs cluster size) if\n> refresh_total_sectors() returns error? I think that is okay.\n\nHere's my proposal for squashing in a best-effort dirty-bitmap resize no\nmatter what happens in refresh_total_sectors() (but really, if you\nsuccessfully truncate the disk but then get a failure while trying to\nread back the actual new size, which may differ from the requested size,\nyou're probably doomed down the road anyways).\n\ndiff --git i/block.c w/block.c\nindex 3caf6bb093..ef5af81f66 100644\n--- i/block.c\n+++ w/block.c\n@@ -3552,8 +3552,9 @@ int bdrv_truncate(BdrvChild *child, int64_t\noffset, PreallocMode prealloc,\n if (ret < 0) {\n error_setg_errno(errp, -ret, \"Could not refresh total sector\ncount\");\n } else {\n- bdrv_dirty_bitmap_truncate(bs, bs->total_sectors *\nBDRV_SECTOR_SIZE);\n+ offset = bs->total_sectors * BDRV_SECTOR_SIZE;\n }\n+ bdrv_dirty_bitmap_truncate(bs, offset);\n bdrv_parent_cb_resize(bs);\n atomic_inc(&bs->write_gen);\n return ret;","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xy0qt3bS5z9rxj\n\tfor ;\n\tWed, 20 Sep 2017 23:26:01 +1000 (AEST)","from localhost ([::1]:48066 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duf0p-0001Hl-3t\n\tfor incoming@patchwork.ozlabs.org; Wed, 20 Sep 2017 09:25:59 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:42858)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dueoJ-0000Do-3k\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 09:13:06 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dueo9-0007l8-Vf\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 09:13:03 -0400","from mx1.redhat.com ([209.132.183.28]:42436)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duenC-0007Cj-V1; Wed, 20 Sep 2017 09:11:55 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 07DB5C00108D;\n\tWed, 20 Sep 2017 13:11:54 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 6528E5D97A;\n\tWed, 20 Sep 2017 13:11:45 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 07DB5C00108D","To":"Fam Zheng , John Snow ","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-6-eblake@redhat.com>\n\t<26069637-9535-cb59-8920-9d9a7eef78b7@redhat.com>\n\t<20170920021014.GB18491@lemon>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"","Date":"Wed, 20 Sep 2017 08:11:44 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170920021014.GB18491@lemon>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"5Lgxq2DsDSfF2p2tw4QFFgXQFaDf3e3Wa\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tWed, 20 Sep 2017 13:11:54 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, vsementsov@virtuozzo.com, qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1772174,"web_url":"http://patchwork.ozlabs.org/comment/1772174/","msgid":"","list_archive_url":null,"date":"2017-09-20T19:50:57","subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","submitter":{"id":64343,"url":"http://patchwork.ozlabs.org/api/people/64343/","name":"John Snow","email":"jsnow@redhat.com"},"content":"On 09/20/2017 09:11 AM, Eric Blake wrote:\n> On 09/19/2017 09:10 PM, Fam Zheng wrote:\n> \n>>>\n>>> Do you suspect that almost certainly if bdrv_truncate() fails overall\n>>> that the image format driver will either unmount the image or become\n>>> read-only?\n> \n> Uggh - it feels like I've bitten off more than I can chew with this\n> patch - I'm getting bogged down by trying to fix bad behavior in code\n> that is mostly unrelated to the patch at hand, so I don't have a good\n> opinion on WHAT is supposed to happen if bdrv_truncate() fails, only\n> that I'm trying to avoid compounding that failure even worse.\n> \n\nYes, I apologize -- I realize I'm holding this series hostage. For now I\nam just trying to legitimately understand the behavior. I am willing to\naccept \"It's sorta busted right now, but -EOUTOFSCOPE\"\n\n>>> I suppose if *not* that's a bug for callers of bdrv_truncate to allow\n>>> that kind of monkey business, but if it CAN happen, hbitmap only guards\n>>> against such things with an assert (which, IIRC, is not guaranteed to be\n>>> on for all builds)\n>>\n>> It's guaranteed since a few hours ago:\n>>\n>> commit 262a69f4282e44426c7a132138581d400053e0a1\n> \n> Indeed - but even without my patch, we would have hit the assertion\n> failures when trying to resize the dirty bitmap to -1 when\n> bdrv_nb_sectors() fails (which was likely if refresh_total_sectors()\n> failed).\n> \n>>> So the question is: \"bdrv_truncate failure is NOT considered recoverable\n>>> in ANY case, is it?\"\n>>>\n>>> It may possibly be safer to, if the initial truncate request succeeds,\n>>> apply a best-effort to the bitmap before returning the error.\n>>\n>> Like fallback \"offset\" (or it aligned up to bs cluster size) if\n>> refresh_total_sectors() returns error? I think that is okay.\n> \n> Here's my proposal for squashing in a best-effort dirty-bitmap resize no\n> matter what happens in refresh_total_sectors() (but really, if you\n> successfully truncate the disk but then get a failure while trying to\n> read back the actual new size, which may differ from the requested size,\n> you're probably doomed down the road anyways).\n> \n> diff --git i/block.c w/block.c\n> index 3caf6bb093..ef5af81f66 100644\n> --- i/block.c\n> +++ w/block.c\n> @@ -3552,8 +3552,9 @@ int bdrv_truncate(BdrvChild *child, int64_t\n> offset, PreallocMode prealloc,\n> if (ret < 0) {\n> error_setg_errno(errp, -ret, \"Could not refresh total sector\n> count\");\n> } else {\n> - bdrv_dirty_bitmap_truncate(bs, bs->total_sectors *\n> BDRV_SECTOR_SIZE);\n> + offset = bs->total_sectors * BDRV_SECTOR_SIZE;\n> }\n> + bdrv_dirty_bitmap_truncate(bs, offset);\n> bdrv_parent_cb_resize(bs);\n> atomic_inc(&bs->write_gen);\n> return ret;\n> \n> \n\nDon't respin on my accord, I'm trying to find out if there is a problem;\nI'm not convinced of one yet. Just thinking out loud.\n\nTwo cases:\n\n(1) Attempt to resize larger. Resize succeeds, but refresh fails.\nPossibly a temporary protocol failure, but we'll assume the resize\nactually worked. Bitmap does not get resized, however any caller of\ntruncate *must* assume that the resize did not succeed. Any calls to\nwrite beyond previous EOF are a bug by the calling module.\n\n(2) Attempt to resize smaller, an actual truncate. Call succeeds but\nrefresh doesn't. Bitmap is now larger than the drive. The bitmap itself\nis perfectly capable of describing reads/writes even to the now-OOB\narea, but it's unlikely the BB would submit any. Problems may arise if\nthe BB does not treat this as a hard failure and a user later attempts\nto use this bitmap for a backup operation, as the trailing bits now\nreference disk segments that may or may not physically exist. Likely to\nhit EIO problems during block jobs.\n\n\nIf we do decide to resize the bitmap even on refresh failure, We\nprobably do still run the risk of the bitmap being slightly bigger or\nslightly smaller than the actual size due to alignment.\n\nIt sounds like the resize operation itself needs to be able to return to\nthe caller the actual size of the operation instead of forcing the\ncaller to query separately in a follow-up call to really \"fix\" this.\n\nConsidering that either resizing or not resizing the bitmap after a\npartial failure probably still leaves us with a possibly dangerous\nbitmap, I don't think I'll hold you to the flames over this one.\n\n--js","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jsnow@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xy9PD4B57z9sNr\n\tfor ;\n\tThu, 21 Sep 2017 05:52:00 +1000 (AEST)","from localhost ([::1]:50394 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dul2M-0005Ra-Ku\n\tfor incoming@patchwork.ozlabs.org; Wed, 20 Sep 2017 15:51:58 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:38494)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dul1d-0005Is-7B\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 15:51:14 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dul1c-000145-1y\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 15:51:13 -0400","from mx1.redhat.com ([209.132.183.28]:45476)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dul1V-0000xT-2l; Wed, 20 Sep 2017 15:51:05 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 0FA906A15;\n\tWed, 20 Sep 2017 19:51:04 +0000 (UTC)","from [10.18.17.130] (dhcp-17-130.bos.redhat.com [10.18.17.130])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 565CD60178;\n\tWed, 20 Sep 2017 19:50:58 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 0FA906A15","To":"Eric Blake , Fam Zheng ","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-6-eblake@redhat.com>\n\t<26069637-9535-cb59-8920-9d9a7eef78b7@redhat.com>\n\t<20170920021014.GB18491@lemon>\n\t","From":"John Snow ","Message-ID":"","Date":"Wed, 20 Sep 2017 15:50:57 -0400","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tWed, 20 Sep 2017 19:51:04 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, vsementsov@virtuozzo.com, qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1774018,"web_url":"http://patchwork.ozlabs.org/comment/1774018/","msgid":"<697b6a91-5122-bc0d-569c-3ac6d94ec96a@virtuozzo.com>","list_archive_url":null,"date":"2017-09-23T12:04:10","subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"19.09.2017 23:18, Eric Blake wrote:\n> We've previously fixed several places where we failed to account\n> for possible errors from bdrv_nb_sectors(). Fix another one by\n> making bdrv_dirty_bitmap_truncate() take the new size from the\n> caller instead of querying itself; then adjust the sole caller\n> bdrv_truncate() to pass the size just determined by a successful\n> resize, or to skip the bitmap resize on failure, thus avoiding\n> sizing the bitmaps to -1.\n>\n> Signed-off-by: Eric Blake \n>\n> ---\n> v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]\n> v8: retitle and rework to avoid possibility of secondary failure [John]\n> v7: new patch [Kevin]\n> ---\n> include/block/dirty-bitmap.h | 2 +-\n> block.c | 15 ++++++++++-----\n> block/dirty-bitmap.c | 6 +++---\n> 3 files changed, 14 insertions(+), 9 deletions(-)\n>\n> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h\n> index 8fd842eac9..7a27590047 100644\n> --- a/include/block/dirty-bitmap.h\n> +++ b/include/block/dirty-bitmap.h\n> @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);\n> void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);\n> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);\n> int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);\n> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);\n> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);\n\nwhy not uint64_t as in following patches?\n\n> bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);\n> bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);\n> bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);\n> diff --git a/block.c b/block.c\n> index ee6a48976e..89261a7a53 100644\n> --- a/block.c\n> +++ b/block.c\n> @@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,\n> assert(!(bs->open_flags & BDRV_O_INACTIVE));\n>\n> ret = drv->bdrv_truncate(bs, offset, prealloc, errp);\n> - if (ret == 0) {\n> - ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n> - bdrv_dirty_bitmap_truncate(bs);\n> - bdrv_parent_cb_resize(bs);\n> - atomic_inc(&bs->write_gen);\n> + if (ret < 0) {\n> + return ret;\n> }\n> + ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n> + if (ret < 0) {\n> + error_setg_errno(errp, -ret, \"Could not refresh total sector count\");\n\nlooks like a separate bug - we didn't set errp with <0 return value\n\n> + } else {\n> + bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE);\n> + }\n> + bdrv_parent_cb_resize(bs);\n> + atomic_inc(&bs->write_gen);\n> return ret;\n> }\n>\n> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c\n> index 42a55e4a4b..ee164fb518 100644\n> --- a/block/dirty-bitmap.c\n> +++ b/block/dirty-bitmap.c\n> @@ -1,7 +1,7 @@\n> /*\n> * Block Dirty Bitmap\n> *\n> - * Copyright (c) 2016 Red Hat. Inc\n> + * Copyright (c) 2016-2017 Red Hat. Inc\n> *\n> * Permission is hereby granted, free of charge, to any person obtaining a copy\n> * of this software and associated documentation files (the \"Software\"), to deal\n> @@ -302,10 +302,10 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,\n> * Truncates _all_ bitmaps attached to a BDS.\n> * Called with BQL taken.\n> */\n> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)\n> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)\n> {\n> BdrvDirtyBitmap *bitmap;\n> - uint64_t size = bdrv_nb_sectors(bs);\n> + int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE);\n>\n> bdrv_dirty_bitmaps_lock(bs);\n> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {\n\n\nLooks like this all needs more work to make it really correct and safe \n(reading last John's comment)..\nAnd this patch don't really relate to the series, so if it will be the \nonly obstacle for merging, can we\nmerge all other patches first? I'll then rebase dirty bitmap migration \nseries on master..","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xzptr6l6Cz9t5b\n\tfor ;\n\tSat, 23 Sep 2017 22:04:52 +1000 (AEST)","from localhost ([::1]:34796 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dvjAw-0000kT-A6\n\tfor incoming@patchwork.ozlabs.org; Sat, 23 Sep 2017 08:04:50 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35812)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dvjAW-0000jW-Nd\n\tfor qemu-devel@nongnu.org; Sat, 23 Sep 2017 08:04:25 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dvjAR-0004rR-MV\n\tfor qemu-devel@nongnu.org; Sat, 23 Sep 2017 08:04:24 -0400","from mailhub.sw.ru ([195.214.232.25]:46889 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dvjAR-0004qz-9E\n\tfor qemu-devel@nongnu.org; Sat, 23 Sep 2017 08:04:19 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v8NC4ABS000972;\n\tSat, 23 Sep 2017 15:04:10 +0300 (MSK)"],"To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-6-eblake@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<697b6a91-5122-bc0d-569c-3ac6d94ec96a@virtuozzo.com>","Date":"Sat, 23 Sep 2017 15:04:10 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170919201910.25656-6-eblake@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, famz@redhat.com, jsnow@redhat.com,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1774744,"web_url":"http://patchwork.ozlabs.org/comment/1774744/","msgid":"","list_archive_url":null,"date":"2017-09-25T13:45:50","subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/23/2017 07:04 AM, Vladimir Sementsov-Ogievskiy wrote:\n> 19.09.2017 23:18, Eric Blake wrote:\n>> We've previously fixed several places where we failed to account\n>> for possible errors from bdrv_nb_sectors().  Fix another one by\n>> making bdrv_dirty_bitmap_truncate() take the new size from the\n>> caller instead of querying itself; then adjust the sole caller\n>> bdrv_truncate() to pass the size just determined by a successful\n>> resize, or to skip the bitmap resize on failure, thus avoiding\n>> sizing the bitmaps to -1.\n>>\n>> Signed-off-by: Eric Blake \n>>\n>> ---\n>> v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]\n>> v8: retitle and rework to avoid possibility of secondary failure [John]\n>> v7: new patch [Kevin]\n>> ---\n>>   include/block/dirty-bitmap.h |  2 +-\n>>   block.c                      | 15 ++++++++++-----\n>>   block/dirty-bitmap.c         |  6 +++---\n>>   3 files changed, 14 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h\n>> index 8fd842eac9..7a27590047 100644\n>> --- a/include/block/dirty-bitmap.h\n>> +++ b/include/block/dirty-bitmap.h\n>> @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter\n>> *iter);\n>>   void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);\n>>   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);\n>>   int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);\n>> -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);\n>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);\n> \n> why not uint64_t as in following patches?\n\nBecause off_t is signed, so you can never have more than 2^63 (and NOT\n2^64) bytes for your size anyways. The following patches use int64_t,\nrather than uint64_t, both because of off_t, and because it leaves room\nfor returning negative values on error.\n\n> \n>>   bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);\n>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);\n>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);\n>> diff --git a/block.c b/block.c\n>> index ee6a48976e..89261a7a53 100644\n>> --- a/block.c\n>> +++ b/block.c\n>> @@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t\n>> offset, PreallocMode prealloc,\n>>       assert(!(bs->open_flags & BDRV_O_INACTIVE));\n>>\n>>       ret = drv->bdrv_truncate(bs, offset, prealloc, errp);\n>> -    if (ret == 0) {\n>> -        ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n>> -        bdrv_dirty_bitmap_truncate(bs);\n>> -        bdrv_parent_cb_resize(bs);\n>> -        atomic_inc(&bs->write_gen);\n>> +    if (ret < 0) {\n>> +        return ret;\n>>       }\n>> +    ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);\n>> +    if (ret < 0) {\n>> +        error_setg_errno(errp, -ret, \"Could not refresh total sector\n>> count\");\n> \n> looks like a separate bug - we didn't set errp with <0 return value\n\nYes, it was a pre-existing bug. If I have to respin, I can update the\ncommit message to mention it.\n\n> \n> Looks like this all needs more work to make it really correct and safe\n> (reading last John's comment)..\n> And this patch don't really relate to the series, so if it will be the\n> only obstacle for merging, can we\n> merge all other patches first? I'll then rebase dirty bitmap migration\n> series on master..\n\nBut it does relate, because I have to do something to avoid calling a\nfailing bdrv_nb_sectors/bdrv_getlength.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y153g17MWz9t5c\n\tfor ;\n\tMon, 25 Sep 2017 23:46:54 +1000 (AEST)","from localhost ([::1]:42513 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwTim-0000gR-P7\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 09:46:52 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36704)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwTi3-0000bV-F4\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 09:46:13 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwTi2-0001tM-3j\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 09:46:07 -0400","from mx1.redhat.com ([209.132.183.28]:63092)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dwThv-0001ou-Px; Mon, 25 Sep 2017 09:46:00 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id A7EE61F56B;\n\tMon, 25 Sep 2017 13:45:58 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 5C5236C535;\n\tMon, 25 Sep 2017 13:45:51 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A7EE61F56B","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-devel@nongnu.org","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-6-eblake@redhat.com>\n\t<697b6a91-5122-bc0d-569c-3ac6d94ec96a@virtuozzo.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"","Date":"Mon, 25 Sep 2017 08:45:50 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<697b6a91-5122-bc0d-569c-3ac6d94ec96a@virtuozzo.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"w9NuVO49csUlOD9jEaTT1vuKuUoPsXvWA\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tMon, 25 Sep 2017 13:45:58 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query\n\tfailure during truncate","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, famz@redhat.com, jsnow@redhat.com,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]