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
