| Message ID | 20170919201910.25656-6-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | make dirty-bitmap byte-based | expand |
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 <eblake@redhat.com> > > --- > 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) 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. > + bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE); > + } > + bdrv_parent_cb_resize(bs); > + atomic_inc(&bs->write_gen); > return ret; > } > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 42a55e4a4b..ee164fb518 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -1,7 +1,7 @@ > /* > * Block Dirty Bitmap > * > - * Copyright (c) 2016 Red Hat. Inc > + * Copyright (c) 2016-2017 Red Hat. Inc > * > * Permission is hereby granted, free of charge, to any person obtaining a copy > * of this software and associated documentation files (the "Software"), to deal > @@ -302,10 +302,10 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > * Truncates _all_ bitmaps attached to a BDS. > * Called with BQL taken. > */ > -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) > +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) > { > BdrvDirtyBitmap *bitmap; > - uint64_t size = bdrv_nb_sectors(bs); > + int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE); > > bdrv_dirty_bitmaps_lock(bs); > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >
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 <eblake@redhat.com> > > > > --- > > 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 <eblake@redhat.com> 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
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;
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
19.09.2017 23:18, 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 <eblake@redhat.com> > > --- > 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); why not uint64_t as in following patches? > 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"); looks like a separate bug - we didn't set errp with <0 return value > + } else { > + bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE); > + } > + bdrv_parent_cb_resize(bs); > + atomic_inc(&bs->write_gen); > return ret; > } > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 42a55e4a4b..ee164fb518 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -1,7 +1,7 @@ > /* > * Block Dirty Bitmap > * > - * Copyright (c) 2016 Red Hat. Inc > + * Copyright (c) 2016-2017 Red Hat. Inc > * > * Permission is hereby granted, free of charge, to any person obtaining a copy > * of this software and associated documentation files (the "Software"), to deal > @@ -302,10 +302,10 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > * Truncates _all_ bitmaps attached to a BDS. > * Called with BQL taken. > */ > -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) > +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) > { > BdrvDirtyBitmap *bitmap; > - uint64_t size = bdrv_nb_sectors(bs); > + int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE); > > bdrv_dirty_bitmaps_lock(bs); > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { Looks like this all needs more work to make it really correct and safe (reading last John's comment).. And this patch don't really relate to the series, so if it will be the only obstacle for merging, can we merge all other patches first? I'll then rebase dirty bitmap migration series on master..
On 09/23/2017 07:04 AM, Vladimir Sementsov-Ogievskiy wrote: > 19.09.2017 23:18, 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 <eblake@redhat.com> >> >> --- >> 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); > > why not uint64_t as in following patches? Because off_t is signed, so you can never have more than 2^63 (and NOT 2^64) bytes for your size anyways. The following patches use int64_t, rather than uint64_t, both because of off_t, and because it leaves room for returning negative values on error. > >> 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"); > > looks like a separate bug - we didn't set errp with <0 return value Yes, it was a pre-existing bug. If I have to respin, I can update the commit message to mention it. > > Looks like this all needs more work to make it really correct and safe > (reading last John's comment).. > And this patch don't really relate to the series, so if it will be the > only obstacle for merging, can we > merge all other patches first? I'll then rebase dirty bitmap migration > series on master.. But it does relate, because I have to do something to avoid calling a failing bdrv_nb_sectors/bdrv_getlength.
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 { + bdrv_dirty_bitmap_truncate(bs, bs->total_sectors * BDRV_SECTOR_SIZE); + } + bdrv_parent_cb_resize(bs); + atomic_inc(&bs->write_gen); return ret; } diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 42a55e4a4b..ee164fb518 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -1,7 +1,7 @@ /* * Block Dirty Bitmap * - * Copyright (c) 2016 Red Hat. Inc + * Copyright (c) 2016-2017 Red Hat. Inc * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -302,10 +302,10 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, * Truncates _all_ bitmaps attached to a BDS. * Called with BQL taken. */ -void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) { BdrvDirtyBitmap *bitmap; - uint64_t size = bdrv_nb_sectors(bs); + int64_t size = DIV_ROUND_UP(bytes, BDRV_SECTOR_SIZE); bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
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 <eblake@redhat.com> --- 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(-)