[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure during truncate |
Date: |
Wed, 20 Sep 2017 08:11:44 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/19/2017 09:10 PM, Fam Zheng wrote:
>>
>> Do you suspect that almost certainly if bdrv_truncate() fails overall
>> that the image format driver will either unmount the image or become
>> read-only?
Uggh - it feels like I've bitten off more than I can chew with this
patch - I'm getting bogged down by trying to fix bad behavior in code
that is mostly unrelated to the patch at hand, so I don't have a good
opinion on WHAT is supposed to happen if bdrv_truncate() fails, only
that I'm trying to avoid compounding that failure even worse.
>> I suppose if *not* that's a bug for callers of bdrv_truncate to allow
>> that kind of monkey business, but if it CAN happen, hbitmap only guards
>> against such things with an assert (which, IIRC, is not guaranteed to be
>> on for all builds)
>
> It's guaranteed since a few hours ago:
>
> commit 262a69f4282e44426c7a132138581d400053e0a1
Indeed - but even without my patch, we would have hit the assertion
failures when trying to resize the dirty bitmap to -1 when
bdrv_nb_sectors() fails (which was likely if refresh_total_sectors()
failed).
>> So the question is: "bdrv_truncate failure is NOT considered recoverable
>> in ANY case, is it?"
>>
>> It may possibly be safer to, if the initial truncate request succeeds,
>> apply a best-effort to the bitmap before returning the error.
>
> Like fallback "offset" (or it aligned up to bs cluster size) if
> refresh_total_sectors() returns error? I think that is okay.
Here's my proposal for squashing in a best-effort dirty-bitmap resize no
matter what happens in refresh_total_sectors() (but really, if you
successfully truncate the disk but then get a failure while trying to
read back the actual new size, which may differ from the requested size,
you're probably doomed down the road anyways).
diff --git i/block.c w/block.c
index 3caf6bb093..ef5af81f66 100644
--- i/block.c
+++ w/block.c
@@ -3552,8 +3552,9 @@ int bdrv_truncate(BdrvChild *child, int64_t
offset, PreallocMode prealloc,
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not refresh total sector
count");
} else {
- bdrv_dirty_bitmap_truncate(bs, bs->total_sectors *
BDRV_SECTOR_SIZE);
+ offset = bs->total_sectors * BDRV_SECTOR_SIZE;
}
+ bdrv_dirty_bitmap_truncate(bs, offset);
bdrv_parent_cb_resize(bs);
atomic_inc(&bs->write_gen);
return ret;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v9 02/20] hbitmap: Rename serialization_granularity to serialization_align, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 03/20] qcow2: Ensure bitmap serialization is aligned, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 06/20] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 07/20] dirty-bitmap: Track bitmap size by bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 08/20] dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 09/20] qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based, Eric Blake, 2017/09/19
[Qemu-devel] [PATCH v9 10/20] dirty-bitmap: Set iterator start by offset, not sector, Eric Blake, 2017/09/19