[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: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure during truncate |
Date: |
Wed, 20 Sep 2017 15:50:57 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/20/2017 09:11 AM, Eric Blake wrote:
> 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.
>
Yes, I apologize -- I realize I'm holding this series hostage. For now I
am just trying to legitimately understand the behavior. I am willing to
accept "It's sorta busted right now, but -EOUTOFSCOPE"
>>> 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;
>
>
Don't respin on my accord, I'm trying to find out if there is a problem;
I'm not convinced of one yet. Just thinking out loud.
Two cases:
(1) Attempt to resize larger. Resize succeeds, but refresh fails.
Possibly a temporary protocol failure, but we'll assume the resize
actually worked. Bitmap does not get resized, however any caller of
truncate *must* assume that the resize did not succeed. Any calls to
write beyond previous EOF are a bug by the calling module.
(2) Attempt to resize smaller, an actual truncate. Call succeeds but
refresh doesn't. Bitmap is now larger than the drive. The bitmap itself
is perfectly capable of describing reads/writes even to the now-OOB
area, but it's unlikely the BB would submit any. Problems may arise if
the BB does not treat this as a hard failure and a user later attempts
to use this bitmap for a backup operation, as the trailing bits now
reference disk segments that may or may not physically exist. Likely to
hit EIO problems during block jobs.
If we do decide to resize the bitmap even on refresh failure, We
probably do still run the risk of the bitmap being slightly bigger or
slightly smaller than the actual size due to alignment.
It sounds like the resize operation itself needs to be able to return to
the caller the actual size of the operation instead of forcing the
caller to query separately in a follow-up call to really "fix" this.
Considering that either resizing or not resizing the bitmap after a
partial failure probably still leaves us with a possibly dangerous
bitmap, I don't think I'll hold you to the flames over this one.
--js
[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
[Qemu-devel] [PATCH v9 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset, Eric Blake, 2017/09/19