[{"id":1768242,"web_url":"http://patchwork.ozlabs.org/comment/1768242/","msgid":"<84d98f85-958f-ad74-6951-68fd4241fcdf@redhat.com>","list_archive_url":null,"date":"2017-09-13T23:27:30","subject":"Re: [Qemu-devel] [PATCH v7 05/20] dirty-bitmap: Check for size\n\tquery failure during truncate","submitter":{"id":64343,"url":"http://patchwork.ozlabs.org/api/people/64343/","name":"John Snow","email":"jsnow@redhat.com"},"content":"On 09/12/2017 04:31 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() report the error rather then\n> silently resizing bitmaps to -1. Then adjust the sole caller\n\nNice failure mode. It was not immediately obvious to me that this could\nfail, but here we all are.\n\n> bdrv_truncate() to both reduce the likelihood of failure (blindly\n> calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()\n> fails was not nice) as well as propagate any actual failures.\n> \n> Signed-off-by: Eric Blake \n> \n> ---\n> v7: new patch [Kevin]\n> ---\n> include/block/dirty-bitmap.h | 2 +-\n> block.c | 19 ++++++++++++++-----\n> block/dirty-bitmap.c | 12 ++++++++----\n> 3 files changed, 23 insertions(+), 10 deletions(-)\n> \n> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h\n> index 8fd842eac9..15101b59d5 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> +int bdrv_dirty_bitmap_truncate(BlockDriverState *bs);\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..790dcce360 100644\n> --- a/block.c\n> +++ b/block.c\n> @@ -3450,12 +3450,21 @@ 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> + return ret;\n> + }\n> + ret = bdrv_dirty_bitmap_truncate(bs);\n> + if (ret < 0) {\n> + error_setg_errno(errp, -ret, \"Could not refresh total sector count\");\n\nYou probably meant to write a different error message here.\n\nAlso, what happens if the actual truncate call works, but the bitmap\nresizing fails? What state does that leave us in?","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-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.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 3xsyX14Jd6z9t2h\n\tfor ;\n\tThu, 14 Sep 2017 09:28:16 +1000 (AEST)","from localhost ([::1]:45019 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dsH4o-000619-Bk\n\tfor incoming@patchwork.ozlabs.org; Wed, 13 Sep 2017 19:28:14 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:33486)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dsH4M-0005xe-Vk\n\tfor qemu-devel@nongnu.org; Wed, 13 Sep 2017 19:27:48 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dsH4L-0004bL-Vh\n\tfor qemu-devel@nongnu.org; Wed, 13 Sep 2017 19:27:46 -0400","from mx1.redhat.com ([209.132.183.28]:46148)\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 1dsH4B-0004Op-CE; Wed, 13 Sep 2017 19:27:35 -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 008EC13A4C;\n\tWed, 13 Sep 2017 23:27:34 +0000 (UTC)","from [10.18.17.231] (dhcp-17-231.bos.redhat.com [10.18.17.231])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 7B7456017A;\n\tWed, 13 Sep 2017 23:27:30 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 008EC13A4C","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170912203119.24166-1-eblake@redhat.com>\n\t<20170912203119.24166-6-eblake@redhat.com>","From":"John Snow ","Message-ID":"<84d98f85-958f-ad74-6951-68fd4241fcdf@redhat.com>","Date":"Wed, 13 Sep 2017 19:27:30 -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":"<20170912203119.24166-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.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tWed, 13 Sep 2017 23:27:34 +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 v7 05/20] dirty-bitmap: Check for size\n\tquery failure 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, Fam Zheng , \n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1768542,"web_url":"http://patchwork.ozlabs.org/comment/1768542/","msgid":"","list_archive_url":null,"date":"2017-09-14T11:56:13","subject":"Re: [Qemu-devel] [PATCH v7 05/20] dirty-bitmap: Check for size\n\tquery failure during truncate","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/13/2017 06:27 PM, John Snow wrote:\n> \n> \n> On 09/12/2017 04:31 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() report the error rather then\n>> silently resizing bitmaps to -1. Then adjust the sole caller\n> \n> Nice failure mode. It was not immediately obvious to me that this could\n> fail, but here we all are.\n> \n>> bdrv_truncate() to both reduce the likelihood of failure (blindly\n>> calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()\n>> fails was not nice) as well as propagate any actual failures.\n>>\n>> Signed-off-by: Eric Blake \n>>\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>> + return ret;\n>> + }\n>> + ret = bdrv_dirty_bitmap_truncate(bs);\n>> + if (ret < 0) {\n>> + error_setg_errno(errp, -ret, \"Could not refresh total sector count\");\n> \n> You probably meant to write a different error message here.\n> \n> Also, what happens if the actual truncate call works, but the bitmap\n> resizing fails? What state does that leave us in?\n\nAnother option: bdrv_dirty_bitmap_truncate() can only fail if\nbdrv_nb_sectors() can fail. We WANT to use the actual size of the\ndevice (which might be slightly different than the requested size passed\nto bdrv_truncate in the first place), but we could change\nbdrv_dirty_bitmap_truncate() to take an actual size as a parameter\ninstead of having to query bdrv_nb_sectors() for the size; and we can\nchange the contract of refresh_total_sectors() to query the actual size\nbefore returning (remember, offset >> BDRV_SECTOR_BITS is only the hint\nsize, and may differ from the actual size). That way, if\nrefresh_total_sectors() succeeds, then bdrv_dirty_bitmap_truncate()\ncannot fail.\n\nI'm not sure, however, how invasive it will be to make\nrefresh_total_sectors() guarantee that it can return the actual size\nthat was set even when that size differs from the hint. Still, it\nsounds like the right approach to take, so I'll play with it.","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=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 3xtH8J5wCSz9ryv\n\tfor ;\n\tThu, 14 Sep 2017 21:57:20 +1000 (AEST)","from localhost ([::1]:47107 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dsSli-00017h-V1\n\tfor incoming@patchwork.ozlabs.org; Thu, 14 Sep 2017 07:57:19 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35541)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dsSkw-0000na-6s\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 07:56:31 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dsSkv-0003Yq-4y\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 07:56:30 -0400","from mx1.redhat.com ([209.132.183.28]:45048)\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 1dsSkp-0003R4-G8; Thu, 14 Sep 2017 07:56:23 -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 6CE295D686;\n\tThu, 14 Sep 2017 11:56:22 +0000 (UTC)","from [10.10.120.201] (ovpn-120-201.rdu2.redhat.com [10.10.120.201])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id DFC5D6F435;\n\tThu, 14 Sep 2017 11:56:14 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 6CE295D686","To":"John Snow , qemu-devel@nongnu.org","References":"<20170912203119.24166-1-eblake@redhat.com>\n\t<20170912203119.24166-6-eblake@redhat.com>\n\t<84d98f85-958f-ad74-6951-68fd4241fcdf@redhat.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"","Date":"Thu, 14 Sep 2017 06:56:13 -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":"<84d98f85-958f-ad74-6951-68fd4241fcdf@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"g651pGaWqKfeEjFlFd3fhdHJbp1h2FNsB\"","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\tThu, 14 Sep 2017 11:56:22 +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 v7 05/20] dirty-bitmap: Check for size\n\tquery failure 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, Fam Zheng , \n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]