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