[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: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/20] dirty-bitmap: Avoid size query failure during truncate |
Date: |
Wed, 20 Sep 2017 10:10:14 +0800 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Tue, 09/19 19:00, John Snow wrote:
>
>
> On 09/19/2017 04:18 PM, Eric Blake wrote:
> > We've previously fixed several places where we failed to account
> > for possible errors from bdrv_nb_sectors(). Fix another one by
> > making bdrv_dirty_bitmap_truncate() take the new size from the
> > caller instead of querying itself; then adjust the sole caller
> > bdrv_truncate() to pass the size just determined by a successful
> > resize, or to skip the bitmap resize on failure, thus avoiding
> > sizing the bitmaps to -1.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> >
> > ---
> > v9: skip only bdrv_dirty_bitmap_truncate on error [Fam]
> > v8: retitle and rework to avoid possibility of secondary failure [John]
> > v7: new patch [Kevin]
> > ---
> > include/block/dirty-bitmap.h | 2 +-
> > block.c | 15 ++++++++++-----
> > block/dirty-bitmap.c | 6 +++---
> > 3 files changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> > index 8fd842eac9..7a27590047 100644
> > --- a/include/block/dirty-bitmap.h
> > +++ b/include/block/dirty-bitmap.h
> > @@ -83,7 +83,7 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
> > void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
> > int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> > int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
> > -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> > +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
> > bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
> > bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
> > bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
> > diff --git a/block.c b/block.c
> > index ee6a48976e..89261a7a53 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -3450,12 +3450,17 @@ int bdrv_truncate(BdrvChild *child, int64_t offset,
> > PreallocMode prealloc,
> > assert(!(bs->open_flags & BDRV_O_INACTIVE));
> >
> > ret = drv->bdrv_truncate(bs, offset, prealloc, errp);
> > - if (ret == 0) {
> > - ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> > - bdrv_dirty_bitmap_truncate(bs);
> > - bdrv_parent_cb_resize(bs);
> > - atomic_inc(&bs->write_gen);
> > + if (ret < 0) {
> > + return ret;
> > }
> > + ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> > + if (ret < 0) {
> > + error_setg_errno(errp, -ret, "Could not refresh total sector
> > count");
> > + } else {
>
> Sorry for dragging my feet on this point; if anyone else wants to R-B
> this I will cede without much of a fuss, but perhaps you can help me
> understand.
>
> (Copying some questions I asked Eric on IRC, airing to list for wider
> discussion, and also because I had to drive home before the stores
> closed for the evening)
>
> 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?
>
> There are calls from parallels, qcow, qcow2-refcount, qcow2, raw-format,
> vhdx-log, vhdx plus whichever calls from blk_truncate (jobs, all of the
> same formats, blockdev, qemu-img)
>
> I'm just trying to picture exactly what's going to happen if we manage
> to resize the drive but then don't resize the bitmap -- say we expand
> the drive larger but fail to refresh (and fail to resize the bitmap.)
>
> if the block format module does not immediately and dutifully stop all
> further writes -- is there anything that stops us from writing to new
> sectors that were beyond EOF previously?
>
> 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
Author: Eric Blake <address@hidden>
Date: Mon Sep 11 16:13:20 2017 -0500
osdep.h: Prohibit disabling assert() in supported builds
>
>
> 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.
Fam
[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