Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure during truncate

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]