| Message ID | 20170530081723.29205-10-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote: > It will be needed in following commits for persistent bitmaps. > If bitmap is loaded from read-only storage (and we can't mark it > "in use" in this storage) corresponding BdrvDirtyBitmap should be > read-only. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ > block/io.c | 8 ++++++++ > blockdev.c | 6 ++++++ > include/block/dirty-bitmap.h | 4 ++++ > 4 files changed, 46 insertions(+) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 90af37287f..733f19ca5e 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { > int64_t size; /* Size of the bitmap (Number of sectors) */ > bool disabled; /* Bitmap is read-only */ > int active_iterators; /* How many iterators are active */ > + bool readonly; /* Bitmap is read-only and may be changed only > + by deserialize* functions */ > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > > @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int64_t nr_sectors) > { > assert(bdrv_dirty_bitmap_enabled(bitmap)); > + assert(!bdrv_dirty_bitmap_readonly(bitmap)); Not reasonable to add the condition for !readonly into bdrv_dirty_bitmap_enabled? As is: If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is going to return ACTIVE for such bitmaps, but DISABLED might be more appropriate to indicate the read-only nature. If you add this condition into _enabled(), you can skip the extra assertions you've added here. > hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > } > > @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int64_t nr_sectors) > { > assert(bdrv_dirty_bitmap_enabled(bitmap)); > + assert(!bdrv_dirty_bitmap_readonly(bitmap)); > hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); > } > > void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) > { > assert(bdrv_dirty_bitmap_enabled(bitmap)); > + assert(!bdrv_dirty_bitmap_readonly(bitmap)); > if (!out) { > hbitmap_reset_all(bitmap->bitmap); > } else { > @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > if (!bdrv_dirty_bitmap_enabled(bitmap)) { > continue; > } > + assert(!bdrv_dirty_bitmap_readonly(bitmap)); > hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > } > } > @@ -540,3 +546,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) > { > return hbitmap_count(bitmap->meta); > } > + > +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) > +{ > + return bitmap->readonly; > +} > + > +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) > +{ > + bitmap->readonly = true; > +} > + > +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) > +{ > + BdrvDirtyBitmap *bm; > + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { > + if (bm->readonly) { > + return true; > + } > + } > + > + return false; > +} > diff --git a/block/io.c b/block/io.c > index fdd7485c22..0e28a1f595 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child, > uint64_t bytes_remaining = bytes; > int max_transfer; > > + if (bdrv_has_readonly_bitmaps(bs)) { > + return -EPERM; > + } > + Oh, hardcore. The original design for "disabled" was just that they would become invalid after writes; but in this case a read-only bitmap literally enforces the concept. I can envision usages for both. > assert(is_power_of_2(align)); > assert((offset & (align - 1)) == 0); > assert((bytes & (align - 1)) == 0); > @@ -2437,6 +2441,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, > return -ENOMEDIUM; > } > > + if (bdrv_has_readonly_bitmaps(bs)) { > + return -EPERM; > + } > + > ret = bdrv_check_byte_request(bs, offset, count); > if (ret < 0) { > return ret; > diff --git a/blockdev.c b/blockdev.c > index c63f4e82c7..2b397abf66 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2033,6 +2033,9 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, > } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { > error_setg(errp, "Cannot clear a disabled bitmap"); > return; > + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { > + error_setg(errp, "Cannot clear a readonly bitmap"); > + return; > } > Oh, I see -- perhaps you wanted a better error message? That makes sense to me.... ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something pretty similar here: we take the bitmap out of active RW state and put it into RO mode, it's just done under the name enabled/disabled instead of RO. As of this patch, nothing uses it yet. Is this patch necessary? Would setting a bitmap as "disabled" to mean "RO" be sufficient, or are the two flags truly semantically necessary? > bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); > @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, > "Bitmap '%s' is currently disabled and cannot be cleared", > name); > goto out; > + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { > + error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name); > + goto out; > } > > bdrv_clear_dirty_bitmap(bitmap, NULL); > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 1e17729ac2..c0c3ce9f85 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -75,4 +75,8 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, > bool finish); > void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); > > +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); > +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); > +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); > + > #endif > I realize I am being very bike-sheddy about this, so I might give an r-b with a light justification. --js
Hi John! Look at our discussion about this in v18 thread. Shortly: readonly is not the same as disabled. disabled= bitmap just ignores all writes. readonly= writes are not allowed at all. And I think, I'll try to go through way 2: "dirty" field instead of "readonly" (look at v18 discussion), as it a bit more flexible. On 01.06.2017 02:48, John Snow wrote: > > On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote: >> It will be needed in following commits for persistent bitmaps. >> If bitmap is loaded from read-only storage (and we can't mark it >> "in use" in this storage) corresponding BdrvDirtyBitmap should be >> read-only. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ >> block/io.c | 8 ++++++++ >> blockdev.c | 6 ++++++ >> include/block/dirty-bitmap.h | 4 ++++ >> 4 files changed, 46 insertions(+) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 90af37287f..733f19ca5e 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { >> int64_t size; /* Size of the bitmap (Number of sectors) */ >> bool disabled; /* Bitmap is read-only */ >> int active_iterators; /* How many iterators are active */ >> + bool readonly; /* Bitmap is read-only and may be changed only >> + by deserialize* functions */ >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> >> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, >> int64_t cur_sector, int64_t nr_sectors) >> { >> assert(bdrv_dirty_bitmap_enabled(bitmap)); >> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); > Not reasonable to add the condition for !readonly into > bdrv_dirty_bitmap_enabled? > > As is: > > If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is > going to return ACTIVE for such bitmaps, but DISABLED might be more > appropriate to indicate the read-only nature. > > If you add this condition into _enabled(), you can skip the extra > assertions you've added here. > >> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >> } >> >> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, >> int64_t cur_sector, int64_t nr_sectors) >> { >> assert(bdrv_dirty_bitmap_enabled(bitmap)); >> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >> } >> >> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) >> { >> assert(bdrv_dirty_bitmap_enabled(bitmap)); >> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >> if (!out) { >> hbitmap_reset_all(bitmap->bitmap); >> } else { >> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, >> if (!bdrv_dirty_bitmap_enabled(bitmap)) { >> continue; >> } >> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >> } >> } >> @@ -540,3 +546,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) >> { >> return hbitmap_count(bitmap->meta); >> } >> + >> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) >> +{ >> + return bitmap->readonly; >> +} >> + >> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) >> +{ >> + bitmap->readonly = true; >> +} >> + >> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) >> +{ >> + BdrvDirtyBitmap *bm; >> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { >> + if (bm->readonly) { >> + return true; >> + } >> + } >> + >> + return false; >> +} >> diff --git a/block/io.c b/block/io.c >> index fdd7485c22..0e28a1f595 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child, >> uint64_t bytes_remaining = bytes; >> int max_transfer; >> >> + if (bdrv_has_readonly_bitmaps(bs)) { >> + return -EPERM; >> + } >> + > Oh, hardcore. The original design for "disabled" was just that they > would become invalid after writes; but in this case a read-only bitmap > literally enforces the concept. > > I can envision usages for both. > >> assert(is_power_of_2(align)); >> assert((offset & (align - 1)) == 0); >> assert((bytes & (align - 1)) == 0); >> @@ -2437,6 +2441,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, >> return -ENOMEDIUM; >> } >> >> + if (bdrv_has_readonly_bitmaps(bs)) { >> + return -EPERM; >> + } >> + >> ret = bdrv_check_byte_request(bs, offset, count); >> if (ret < 0) { >> return ret; >> diff --git a/blockdev.c b/blockdev.c >> index c63f4e82c7..2b397abf66 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2033,6 +2033,9 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, >> } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { >> error_setg(errp, "Cannot clear a disabled bitmap"); >> return; >> + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { >> + error_setg(errp, "Cannot clear a readonly bitmap"); >> + return; >> } >> > Oh, I see -- perhaps you wanted a better error message? That makes sense > to me.... > > > ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something > pretty similar here: we take the bitmap out of active RW state and put > it into RO mode, it's just done under the name enabled/disabled instead > of RO. > > As of this patch, nothing uses it yet. Is this patch necessary? Would > setting a bitmap as "disabled" to mean "RO" be sufficient, or are the > two flags truly semantically necessary? > >> bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); >> @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, >> "Bitmap '%s' is currently disabled and cannot be cleared", >> name); >> goto out; >> + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { >> + error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name); >> + goto out; >> } >> >> bdrv_clear_dirty_bitmap(bitmap, NULL); >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >> index 1e17729ac2..c0c3ce9f85 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -75,4 +75,8 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, >> bool finish); >> void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); >> >> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); >> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); >> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); >> + >> #endif >> > I realize I am being very bike-sheddy about this, so I might give an r-b > with a light justification. > > --js
On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: > Hi John! > > Look at our discussion about this in v18 thread. > I'm batting zero today; I literally missed that entire subthread. Sigh, thanks, I'll go back and read.
On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: > Hi John! > > Look at our discussion about this in v18 thread. > > Shortly: readonly is not the same as disabled. disabled= bitmap just > ignores all writes. readonly= writes are not allowed at all. > > And I think, I'll try to go through way 2: "dirty" field instead of > "readonly" (look at v18 discussion), as it a bit more flexible. > Not sure which I prefer... Method 1 is attractive in that it is fairly simple, and enforces fairly loudly the inability to write to devices with RO bitmaps. It's a natural extension of your current approach. Method 2 is attractive in that it seems a little more efficient, and is a little more clever. A dirty flag lets us avoid flushing bitmaps we never even changed (though we still need to clean up the in_use flags.) What I wonder about #2 is what happens when a write sneaks in (due to a bug or a use case we didn't see) on a bitmap attached to a read-only node. We fail later on invalidate? It shouldn't happen in normal circumstances, but I worry that the failure mode is messier. Well, either way I will be happy for now I think -- pick whichever option feels easiest or best for you to implement. Thanks! > > On 01.06.2017 02:48, John Snow wrote: >> >> On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote: >>> It will be needed in following commits for persistent bitmaps. >>> If bitmap is loaded from read-only storage (and we can't mark it >>> "in use" in this storage) corresponding BdrvDirtyBitmap should be >>> read-only. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ >>> block/io.c | 8 ++++++++ >>> blockdev.c | 6 ++++++ >>> include/block/dirty-bitmap.h | 4 ++++ >>> 4 files changed, 46 insertions(+) >>> >>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>> index 90af37287f..733f19ca5e 100644 >>> --- a/block/dirty-bitmap.c >>> +++ b/block/dirty-bitmap.c >>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { >>> int64_t size; /* Size of the bitmap (Number of >>> sectors) */ >>> bool disabled; /* Bitmap is read-only */ >>> int active_iterators; /* How many iterators are active */ >>> + bool readonly; /* Bitmap is read-only and may be >>> changed only >>> + by deserialize* functions */ >>> QLIST_ENTRY(BdrvDirtyBitmap) list; >>> }; >>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap >>> *bitmap, >>> int64_t cur_sector, int64_t nr_sectors) >>> { >>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >> Not reasonable to add the condition for !readonly into >> bdrv_dirty_bitmap_enabled? >> >> As is: >> >> If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is >> going to return ACTIVE for such bitmaps, but DISABLED might be more >> appropriate to indicate the read-only nature. >> >> If you add this condition into _enabled(), you can skip the extra >> assertions you've added here. >> >>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>> } >>> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap >>> *bitmap, >>> int64_t cur_sector, int64_t nr_sectors) >>> { >>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >>> } >>> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) >>> { >>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>> if (!out) { >>> hbitmap_reset_all(bitmap->bitmap); >>> } else { >>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t >>> cur_sector, >>> if (!bdrv_dirty_bitmap_enabled(bitmap)) { >>> continue; >>> } >>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>> } >>> } >>> @@ -540,3 +546,25 @@ int64_t >>> bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) >>> { >>> return hbitmap_count(bitmap->meta); >>> } >>> + >>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) >>> +{ >>> + return bitmap->readonly; >>> +} >>> + >>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) >>> +{ >>> + bitmap->readonly = true; >>> +} >>> + >>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) >>> +{ >>> + BdrvDirtyBitmap *bm; >>> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { >>> + if (bm->readonly) { >>> + return true; >>> + } >>> + } >>> + >>> + return false; >>> +} >>> diff --git a/block/io.c b/block/io.c >>> index fdd7485c22..0e28a1f595 100644 >>> --- a/block/io.c >>> +++ b/block/io.c >>> @@ -1349,6 +1349,10 @@ static int coroutine_fn >>> bdrv_aligned_pwritev(BdrvChild *child, >>> uint64_t bytes_remaining = bytes; >>> int max_transfer; >>> + if (bdrv_has_readonly_bitmaps(bs)) { >>> + return -EPERM; >>> + } >>> + >> Oh, hardcore. The original design for "disabled" was just that they >> would become invalid after writes; but in this case a read-only bitmap >> literally enforces the concept. >> >> I can envision usages for both. >> >>> assert(is_power_of_2(align)); >>> assert((offset & (align - 1)) == 0); >>> assert((bytes & (align - 1)) == 0); >>> @@ -2437,6 +2441,10 @@ int coroutine_fn >>> bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, >>> return -ENOMEDIUM; >>> } >>> + if (bdrv_has_readonly_bitmaps(bs)) { >>> + return -EPERM; >>> + } >>> + >>> ret = bdrv_check_byte_request(bs, offset, count); >>> if (ret < 0) { >>> return ret; >>> diff --git a/blockdev.c b/blockdev.c >>> index c63f4e82c7..2b397abf66 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -2033,6 +2033,9 @@ static void >>> block_dirty_bitmap_clear_prepare(BlkActionState *common, >>> } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { >>> error_setg(errp, "Cannot clear a disabled bitmap"); >>> return; >>> + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { >>> + error_setg(errp, "Cannot clear a readonly bitmap"); >>> + return; >>> } >>> >> Oh, I see -- perhaps you wanted a better error message? That makes sense >> to me.... >> >> >> ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something >> pretty similar here: we take the bitmap out of active RW state and put >> it into RO mode, it's just done under the name enabled/disabled instead >> of RO. >> >> As of this patch, nothing uses it yet. Is this patch necessary? Would >> setting a bitmap as "disabled" to mean "RO" be sufficient, or are the >> two flags truly semantically necessary? >> >>> bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); >>> @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char >>> *node, const char *name, >>> "Bitmap '%s' is currently disabled and cannot be >>> cleared", >>> name); >>> goto out; >>> + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { >>> + error_setg(errp, "Bitmap '%s' is readonly and cannot be >>> cleared", name); >>> + goto out; >>> } >>> bdrv_clear_dirty_bitmap(bitmap, NULL); >>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>> index 1e17729ac2..c0c3ce9f85 100644 >>> --- a/include/block/dirty-bitmap.h >>> +++ b/include/block/dirty-bitmap.h >>> @@ -75,4 +75,8 @@ void >>> bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, >>> bool finish); >>> void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); >>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); >>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); >>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); >>> + >>> #endif >>> >> I realize I am being very bike-sheddy about this, so I might give an r-b >> with a light justification. >> >> --js >
02.06.2017 02:25, John Snow wrote: > > On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: >> Hi John! >> >> Look at our discussion about this in v18 thread. >>> Shortly: readonly is not the same as disabled. disabled= bitmap just >> ignores all writes. readonly= writes are not allowed at all. >> >> And I think, I'll try to go through way 2: "dirty" field instead of >> "readonly" (look at v18 discussion), as it a bit more flexible. >> > Not sure which I prefer... > > Method 1 is attractive in that it is fairly simple, and enforces fairly > loudly the inability to write to devices with RO bitmaps. It's a natural > extension of your current approach. For now I decided to realize this one, I think I'll publish it today. Also, I'm going to rename s/readonly/in_use - to avoid the confuse with disabled. So let this field just be mirror of IN_USE in the image and just say "persistent storage knows, that bitmap is in use and may be dirty". Also, optimization with 'dirty' flag may be added later. > > Method 2 is attractive in that it seems a little more efficient, and is > a little more clever. A dirty flag lets us avoid flushing bitmaps we > never even changed (though we still need to clean up the in_use flags.) > > What I wonder about #2 is what happens when a write sneaks in (due to a > bug or a use case we didn't see) on a bitmap attached to a read-only > node. We fail later on invalidate? It shouldn't happen in normal > circumstances, but I worry that the failure mode is messier. if bitmap is dirty - all ok, the problems will appear when we'll try to save it, but these problems are not fatal - bitmap should be marked 'in_use' in the image, so it will be lost (the worst case is when in_use not set and bitmap is incorrect - it may lead to data loss for user) if it is not dirty - we will fail to write 'in_use' before actual write and the whole write will fail. > > > Well, either way I will be happy for now I think -- pick whichever > option feels easiest or best for you to implement. > > Thanks! > >> On 01.06.2017 02:48, John Snow wrote: >>> On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> It will be needed in following commits for persistent bitmaps. >>>> If bitmap is loaded from read-only storage (and we can't mark it >>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be >>>> read-only. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ >>>> block/io.c | 8 ++++++++ >>>> blockdev.c | 6 ++++++ >>>> include/block/dirty-bitmap.h | 4 ++++ >>>> 4 files changed, 46 insertions(+) >>>> >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 90af37287f..733f19ca5e 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { >>>> int64_t size; /* Size of the bitmap (Number of >>>> sectors) */ >>>> bool disabled; /* Bitmap is read-only */ >>>> int active_iterators; /* How many iterators are active */ >>>> + bool readonly; /* Bitmap is read-only and may be >>>> changed only >>>> + by deserialize* functions */ >>>> QLIST_ENTRY(BdrvDirtyBitmap) list; >>>> }; >>>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap >>>> *bitmap, >>>> int64_t cur_sector, int64_t nr_sectors) >>>> { >>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>> Not reasonable to add the condition for !readonly into >>> bdrv_dirty_bitmap_enabled? >>> >>> As is: >>> >>> If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is >>> going to return ACTIVE for such bitmaps, but DISABLED might be more >>> appropriate to indicate the read-only nature. >>> >>> If you add this condition into _enabled(), you can skip the extra >>> assertions you've added here. >>> >>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>>> } >>>> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap >>>> *bitmap, >>>> int64_t cur_sector, int64_t nr_sectors) >>>> { >>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >>>> } >>>> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) >>>> { >>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>> if (!out) { >>>> hbitmap_reset_all(bitmap->bitmap); >>>> } else { >>>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t >>>> cur_sector, >>>> if (!bdrv_dirty_bitmap_enabled(bitmap)) { >>>> continue; >>>> } >>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>>> } >>>> } >>>> @@ -540,3 +546,25 @@ int64_t >>>> bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) >>>> { >>>> return hbitmap_count(bitmap->meta); >>>> } >>>> + >>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) >>>> +{ >>>> + return bitmap->readonly; >>>> +} >>>> + >>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) >>>> +{ >>>> + bitmap->readonly = true; >>>> +} >>>> + >>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) >>>> +{ >>>> + BdrvDirtyBitmap *bm; >>>> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { >>>> + if (bm->readonly) { >>>> + return true; >>>> + } >>>> + } >>>> + >>>> + return false; >>>> +} >>>> diff --git a/block/io.c b/block/io.c >>>> index fdd7485c22..0e28a1f595 100644 >>>> --- a/block/io.c >>>> +++ b/block/io.c >>>> @@ -1349,6 +1349,10 @@ static int coroutine_fn >>>> bdrv_aligned_pwritev(BdrvChild *child, >>>> uint64_t bytes_remaining = bytes; >>>> int max_transfer; >>>> + if (bdrv_has_readonly_bitmaps(bs)) { >>>> + return -EPERM; >>>> + } >>>> + >>> Oh, hardcore. The original design for "disabled" was just that they >>> would become invalid after writes; but in this case a read-only bitmap >>> literally enforces the concept. >>> >>> I can envision usages for both. >>> >>>> assert(is_power_of_2(align)); >>>> assert((offset & (align - 1)) == 0); >>>> assert((bytes & (align - 1)) == 0); >>>> @@ -2437,6 +2441,10 @@ int coroutine_fn >>>> bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, >>>> return -ENOMEDIUM; >>>> } >>>> + if (bdrv_has_readonly_bitmaps(bs)) { >>>> + return -EPERM; >>>> + } >>>> + >>>> ret = bdrv_check_byte_request(bs, offset, count); >>>> if (ret < 0) { >>>> return ret; >>>> diff --git a/blockdev.c b/blockdev.c >>>> index c63f4e82c7..2b397abf66 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -2033,6 +2033,9 @@ static void >>>> block_dirty_bitmap_clear_prepare(BlkActionState *common, >>>> } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { >>>> error_setg(errp, "Cannot clear a disabled bitmap"); >>>> return; >>>> + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { >>>> + error_setg(errp, "Cannot clear a readonly bitmap"); >>>> + return; >>>> } >>>> >>> Oh, I see -- perhaps you wanted a better error message? That makes sense >>> to me.... >>> >>> >>> ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something >>> pretty similar here: we take the bitmap out of active RW state and put >>> it into RO mode, it's just done under the name enabled/disabled instead >>> of RO. >>> >>> As of this patch, nothing uses it yet. Is this patch necessary? Would >>> setting a bitmap as "disabled" to mean "RO" be sufficient, or are the >>> two flags truly semantically necessary? >>> >>>> bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); >>>> @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char >>>> *node, const char *name, >>>> "Bitmap '%s' is currently disabled and cannot be >>>> cleared", >>>> name); >>>> goto out; >>>> + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { >>>> + error_setg(errp, "Bitmap '%s' is readonly and cannot be >>>> cleared", name); >>>> + goto out; >>>> } >>>> bdrv_clear_dirty_bitmap(bitmap, NULL); >>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>>> index 1e17729ac2..c0c3ce9f85 100644 >>>> --- a/include/block/dirty-bitmap.h >>>> +++ b/include/block/dirty-bitmap.h >>>> @@ -75,4 +75,8 @@ void >>>> bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, >>>> bool finish); >>>> void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); >>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); >>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); >>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); >>>> + >>>> #endif >>>> >>> I realize I am being very bike-sheddy about this, so I might give an r-b >>> with a light justification. >>> >>> --js
02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote: > 02.06.2017 02:25, John Snow wrote: >> >> On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: >>> Hi John! >>> >>> Look at our discussion about this in v18 thread. >>>> Shortly: readonly is not the same as disabled. disabled= bitmap just >>> ignores all writes. readonly= writes are not allowed at all. >>> >>> And I think, I'll try to go through way 2: "dirty" field instead of >>> "readonly" (look at v18 discussion), as it a bit more flexible. >>> >> Not sure which I prefer... >> >> Method 1 is attractive in that it is fairly simple, and enforces fairly >> loudly the inability to write to devices with RO bitmaps. It's a natural >> extension of your current approach. > > For now I decided to realize this one, I think I'll publish it today. > Also, I'm going to rename s/readonly/in_use - to avoid the confuse > with disabled. So let this field just be mirror of IN_USE in the image > and just say "persistent storage knows, that bitmap is in use and may > be dirty". > > Also, optimization with 'dirty' flag may be added later. And, also, I don't want to influence this "first write", on which we will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less performance-demanding place than write. > >> >> Method 2 is attractive in that it seems a little more efficient, and is >> a little more clever. A dirty flag lets us avoid flushing bitmaps we >> never even changed (though we still need to clean up the in_use flags.) >> >> What I wonder about #2 is what happens when a write sneaks in (due to a >> bug or a use case we didn't see) on a bitmap attached to a read-only >> node. We fail later on invalidate? It shouldn't happen in normal >> circumstances, but I worry that the failure mode is messier. > > if bitmap is dirty - all ok, the problems will appear when we'll try > to save it, but these problems are not fatal - bitmap should be marked > 'in_use' in the image, so it will be lost (the worst case is when > in_use not set and bitmap is incorrect - it may lead to data loss for > user) > > if it is not dirty - we will fail to write 'in_use' before actual > write and the whole write will fail. > >> >> >> Well, either way I will be happy for now I think -- pick whichever >> option feels easiest or best for you to implement. >> >> Thanks! >> >>> On 01.06.2017 02:48, John Snow wrote: >>>> On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>> It will be needed in following commits for persistent bitmaps. >>>>> If bitmap is loaded from read-only storage (and we can't mark it >>>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be >>>>> read-only. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>> <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ >>>>> block/io.c | 8 ++++++++ >>>>> blockdev.c | 6 ++++++ >>>>> include/block/dirty-bitmap.h | 4 ++++ >>>>> 4 files changed, 46 insertions(+) >>>>> >>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>>> index 90af37287f..733f19ca5e 100644 >>>>> --- a/block/dirty-bitmap.c >>>>> +++ b/block/dirty-bitmap.c >>>>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { >>>>> int64_t size; /* Size of the bitmap (Number of >>>>> sectors) */ >>>>> bool disabled; /* Bitmap is read-only */ >>>>> int active_iterators; /* How many iterators are >>>>> active */ >>>>> + bool readonly; /* Bitmap is read-only and may be >>>>> changed only >>>>> + by deserialize* functions */ >>>>> QLIST_ENTRY(BdrvDirtyBitmap) list; >>>>> }; >>>>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap >>>>> *bitmap, >>>>> int64_t cur_sector, int64_t nr_sectors) >>>>> { >>>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>> Not reasonable to add the condition for !readonly into >>>> bdrv_dirty_bitmap_enabled? >>>> >>>> As is: >>>> >>>> If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is >>>> going to return ACTIVE for such bitmaps, but DISABLED might be more >>>> appropriate to indicate the read-only nature. >>>> >>>> If you add this condition into _enabled(), you can skip the extra >>>> assertions you've added here. >>>> >>>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>>>> } >>>>> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap >>>>> *bitmap, >>>>> int64_t cur_sector, int64_t >>>>> nr_sectors) >>>>> { >>>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >>>>> } >>>>> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap >>>>> **out) >>>>> { >>>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>> if (!out) { >>>>> hbitmap_reset_all(bitmap->bitmap); >>>>> } else { >>>>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t >>>>> cur_sector, >>>>> if (!bdrv_dirty_bitmap_enabled(bitmap)) { >>>>> continue; >>>>> } >>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>>>> } >>>>> } >>>>> @@ -540,3 +546,25 @@ int64_t >>>>> bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) >>>>> { >>>>> return hbitmap_count(bitmap->meta); >>>>> } >>>>> + >>>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) >>>>> +{ >>>>> + return bitmap->readonly; >>>>> +} >>>>> + >>>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) >>>>> +{ >>>>> + bitmap->readonly = true; >>>>> +} >>>>> + >>>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) >>>>> +{ >>>>> + BdrvDirtyBitmap *bm; >>>>> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { >>>>> + if (bm->readonly) { >>>>> + return true; >>>>> + } >>>>> + } >>>>> + >>>>> + return false; >>>>> +} >>>>> diff --git a/block/io.c b/block/io.c >>>>> index fdd7485c22..0e28a1f595 100644 >>>>> --- a/block/io.c >>>>> +++ b/block/io.c >>>>> @@ -1349,6 +1349,10 @@ static int coroutine_fn >>>>> bdrv_aligned_pwritev(BdrvChild *child, >>>>> uint64_t bytes_remaining = bytes; >>>>> int max_transfer; >>>>> + if (bdrv_has_readonly_bitmaps(bs)) { >>>>> + return -EPERM; >>>>> + } >>>>> + >>>> Oh, hardcore. The original design for "disabled" was just that they >>>> would become invalid after writes; but in this case a read-only bitmap >>>> literally enforces the concept. >>>> >>>> I can envision usages for both. >>>> >>>>> assert(is_power_of_2(align)); >>>>> assert((offset & (align - 1)) == 0); >>>>> assert((bytes & (align - 1)) == 0); >>>>> @@ -2437,6 +2441,10 @@ int coroutine_fn >>>>> bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, >>>>> return -ENOMEDIUM; >>>>> } >>>>> + if (bdrv_has_readonly_bitmaps(bs)) { >>>>> + return -EPERM; >>>>> + } >>>>> + >>>>> ret = bdrv_check_byte_request(bs, offset, count); >>>>> if (ret < 0) { >>>>> return ret; >>>>> diff --git a/blockdev.c b/blockdev.c >>>>> index c63f4e82c7..2b397abf66 100644 >>>>> --- a/blockdev.c >>>>> +++ b/blockdev.c >>>>> @@ -2033,6 +2033,9 @@ static void >>>>> block_dirty_bitmap_clear_prepare(BlkActionState *common, >>>>> } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { >>>>> error_setg(errp, "Cannot clear a disabled bitmap"); >>>>> return; >>>>> + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { >>>>> + error_setg(errp, "Cannot clear a readonly bitmap"); >>>>> + return; >>>>> } >>>> Oh, I see -- perhaps you wanted a better error message? That makes >>>> sense >>>> to me.... >>>> >>>> >>>> ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something >>>> pretty similar here: we take the bitmap out of active RW state and put >>>> it into RO mode, it's just done under the name enabled/disabled >>>> instead >>>> of RO. >>>> >>>> As of this patch, nothing uses it yet. Is this patch necessary? Would >>>> setting a bitmap as "disabled" to mean "RO" be sufficient, or are the >>>> two flags truly semantically necessary? >>>> >>>>> bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); >>>>> @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char >>>>> *node, const char *name, >>>>> "Bitmap '%s' is currently disabled and >>>>> cannot be >>>>> cleared", >>>>> name); >>>>> goto out; >>>>> + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { >>>>> + error_setg(errp, "Bitmap '%s' is readonly and cannot be >>>>> cleared", name); >>>>> + goto out; >>>>> } >>>>> bdrv_clear_dirty_bitmap(bitmap, NULL); >>>>> diff --git a/include/block/dirty-bitmap.h >>>>> b/include/block/dirty-bitmap.h >>>>> index 1e17729ac2..c0c3ce9f85 100644 >>>>> --- a/include/block/dirty-bitmap.h >>>>> +++ b/include/block/dirty-bitmap.h >>>>> @@ -75,4 +75,8 @@ void >>>>> bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, >>>>> bool finish); >>>>> void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap >>>>> *bitmap); >>>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); >>>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); >>>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); >>>>> + >>>>> #endif >>>>> >>>> I realize I am being very bike-sheddy about this, so I might give >>>> an r-b >>>> with a light justification. >>>> >>>> --js > >
02.06.2017 12:01, Vladimir Sementsov-Ogievskiy wrote: > 02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote: >> 02.06.2017 02:25, John Snow wrote: >>> >>> On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: >>>> Hi John! >>>> >>>> Look at our discussion about this in v18 thread. >>>>> Shortly: readonly is not the same as disabled. disabled= bitmap just >>>> ignores all writes. readonly= writes are not allowed at all. >>>> >>>> And I think, I'll try to go through way 2: "dirty" field instead of >>>> "readonly" (look at v18 discussion), as it a bit more flexible. >>>> >>> Not sure which I prefer... >>> >>> Method 1 is attractive in that it is fairly simple, and enforces fairly >>> loudly the inability to write to devices with RO bitmaps. It's a >>> natural >>> extension of your current approach. >> >> For now I decided to realize this one, I think I'll publish it today. >> Also, I'm going to rename s/readonly/in_use - to avoid the confuse >> with disabled. So let this field just be mirror of IN_USE in the >> image and just say "persistent storage knows, that bitmap is in use >> and may be dirty". Finally it would be readonly. in_use is bad for created (not loaded) bitmaps. I'll add more descriptive comments for disabled and readonly. >> >> Also, optimization with 'dirty' flag may be added later. > > And, also, I don't want to influence this "first write", on which we > will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less > performance-demanding place than write. > > >> >>> >>> Method 2 is attractive in that it seems a little more efficient, and is >>> a little more clever. A dirty flag lets us avoid flushing bitmaps we >>> never even changed (though we still need to clean up the in_use flags.) >>> >>> What I wonder about #2 is what happens when a write sneaks in (due to a >>> bug or a use case we didn't see) on a bitmap attached to a read-only >>> node. We fail later on invalidate? It shouldn't happen in normal >>> circumstances, but I worry that the failure mode is messier. >> >> if bitmap is dirty - all ok, the problems will appear when we'll try >> to save it, but these problems are not fatal - bitmap should be >> marked 'in_use' in the image, so it will be lost (the worst case is >> when in_use not set and bitmap is incorrect - it may lead to data >> loss for user) >> >> if it is not dirty - we will fail to write 'in_use' before actual >> write and the whole write will fail. >> >>> >>> >>> Well, either way I will be happy for now I think -- pick whichever >>> option feels easiest or best for you to implement. >>> >>> Thanks! >>> >>>> On 01.06.2017 02:48, John Snow wrote: >>>>> On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>>> It will be needed in following commits for persistent bitmaps. >>>>>> If bitmap is loaded from read-only storage (and we can't mark it >>>>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be >>>>>> read-only. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>> <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ >>>>>> block/io.c | 8 ++++++++ >>>>>> blockdev.c | 6 ++++++ >>>>>> include/block/dirty-bitmap.h | 4 ++++ >>>>>> 4 files changed, 46 insertions(+) >>>>>> >>>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>>>> index 90af37287f..733f19ca5e 100644 >>>>>> --- a/block/dirty-bitmap.c >>>>>> +++ b/block/dirty-bitmap.c >>>>>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { >>>>>> int64_t size; /* Size of the bitmap (Number of >>>>>> sectors) */ >>>>>> bool disabled; /* Bitmap is read-only */ >>>>>> int active_iterators; /* How many iterators are >>>>>> active */ >>>>>> + bool readonly; /* Bitmap is read-only and may be >>>>>> changed only >>>>>> + by deserialize* functions */ >>>>>> QLIST_ENTRY(BdrvDirtyBitmap) list; >>>>>> }; >>>>>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap >>>>>> *bitmap, >>>>>> int64_t cur_sector, int64_t >>>>>> nr_sectors) >>>>>> { >>>>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>> Not reasonable to add the condition for !readonly into >>>>> bdrv_dirty_bitmap_enabled? >>>>> >>>>> As is: >>>>> >>>>> If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is >>>>> going to return ACTIVE for such bitmaps, but DISABLED might be more >>>>> appropriate to indicate the read-only nature. >>>>> >>>>> If you add this condition into _enabled(), you can skip the extra >>>>> assertions you've added here. >>>>> >>>>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>>>>> } >>>>>> @@ -443,12 +446,14 @@ void >>>>>> bdrv_reset_dirty_bitmap(BdrvDirtyBitmap >>>>>> *bitmap, >>>>>> int64_t cur_sector, int64_t >>>>>> nr_sectors) >>>>>> { >>>>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>>> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >>>>>> } >>>>>> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, >>>>>> HBitmap **out) >>>>>> { >>>>>> assert(bdrv_dirty_bitmap_enabled(bitmap)); >>>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>>> if (!out) { >>>>>> hbitmap_reset_all(bitmap->bitmap); >>>>>> } else { >>>>>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, >>>>>> int64_t >>>>>> cur_sector, >>>>>> if (!bdrv_dirty_bitmap_enabled(bitmap)) { >>>>>> continue; >>>>>> } >>>>>> + assert(!bdrv_dirty_bitmap_readonly(bitmap)); >>>>>> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >>>>>> } >>>>>> } >>>>>> @@ -540,3 +546,25 @@ int64_t >>>>>> bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) >>>>>> { >>>>>> return hbitmap_count(bitmap->meta); >>>>>> } >>>>>> + >>>>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) >>>>>> +{ >>>>>> + return bitmap->readonly; >>>>>> +} >>>>>> + >>>>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) >>>>>> +{ >>>>>> + bitmap->readonly = true; >>>>>> +} >>>>>> + >>>>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) >>>>>> +{ >>>>>> + BdrvDirtyBitmap *bm; >>>>>> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { >>>>>> + if (bm->readonly) { >>>>>> + return true; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return false; >>>>>> +} >>>>>> diff --git a/block/io.c b/block/io.c >>>>>> index fdd7485c22..0e28a1f595 100644 >>>>>> --- a/block/io.c >>>>>> +++ b/block/io.c >>>>>> @@ -1349,6 +1349,10 @@ static int coroutine_fn >>>>>> bdrv_aligned_pwritev(BdrvChild *child, >>>>>> uint64_t bytes_remaining = bytes; >>>>>> int max_transfer; >>>>>> + if (bdrv_has_readonly_bitmaps(bs)) { >>>>>> + return -EPERM; >>>>>> + } >>>>>> + >>>>> Oh, hardcore. The original design for "disabled" was just that they >>>>> would become invalid after writes; but in this case a read-only >>>>> bitmap >>>>> literally enforces the concept. >>>>> >>>>> I can envision usages for both. >>>>> >>>>>> assert(is_power_of_2(align)); >>>>>> assert((offset & (align - 1)) == 0); >>>>>> assert((bytes & (align - 1)) == 0); >>>>>> @@ -2437,6 +2441,10 @@ int coroutine_fn >>>>>> bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, >>>>>> return -ENOMEDIUM; >>>>>> } >>>>>> + if (bdrv_has_readonly_bitmaps(bs)) { >>>>>> + return -EPERM; >>>>>> + } >>>>>> + >>>>>> ret = bdrv_check_byte_request(bs, offset, count); >>>>>> if (ret < 0) { >>>>>> return ret; >>>>>> diff --git a/blockdev.c b/blockdev.c >>>>>> index c63f4e82c7..2b397abf66 100644 >>>>>> --- a/blockdev.c >>>>>> +++ b/blockdev.c >>>>>> @@ -2033,6 +2033,9 @@ static void >>>>>> block_dirty_bitmap_clear_prepare(BlkActionState *common, >>>>>> } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { >>>>>> error_setg(errp, "Cannot clear a disabled bitmap"); >>>>>> return; >>>>>> + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { >>>>>> + error_setg(errp, "Cannot clear a readonly bitmap"); >>>>>> + return; >>>>>> } >>>>> Oh, I see -- perhaps you wanted a better error message? That makes >>>>> sense >>>>> to me.... >>>>> >>>>> >>>>> ...Though, you know, bdrv_disable_dirty_bitmap accomplishes something >>>>> pretty similar here: we take the bitmap out of active RW state and >>>>> put >>>>> it into RO mode, it's just done under the name enabled/disabled >>>>> instead >>>>> of RO. >>>>> >>>>> As of this patch, nothing uses it yet. Is this patch necessary? Would >>>>> setting a bitmap as "disabled" to mean "RO" be sufficient, or are the >>>>> two flags truly semantically necessary? >>>>> >>>>>> bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); >>>>>> @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char >>>>>> *node, const char *name, >>>>>> "Bitmap '%s' is currently disabled and >>>>>> cannot be >>>>>> cleared", >>>>>> name); >>>>>> goto out; >>>>>> + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { >>>>>> + error_setg(errp, "Bitmap '%s' is readonly and cannot be >>>>>> cleared", name); >>>>>> + goto out; >>>>>> } >>>>>> bdrv_clear_dirty_bitmap(bitmap, NULL); >>>>>> diff --git a/include/block/dirty-bitmap.h >>>>>> b/include/block/dirty-bitmap.h >>>>>> index 1e17729ac2..c0c3ce9f85 100644 >>>>>> --- a/include/block/dirty-bitmap.h >>>>>> +++ b/include/block/dirty-bitmap.h >>>>>> @@ -75,4 +75,8 @@ void >>>>>> bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, >>>>>> bool finish); >>>>>> void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap >>>>>> *bitmap); >>>>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); >>>>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); >>>>>> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); >>>>>> + >>>>>> #endif >>>>>> >>>>> I realize I am being very bike-sheddy about this, so I might give >>>>> an r-b >>>>> with a light justification. >>>>> >>>>> --js >> >> >
On 06/02/2017 05:45 AM, Vladimir Sementsov-Ogievskiy wrote: > 02.06.2017 12:01, Vladimir Sementsov-Ogievskiy wrote: >> 02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote: >>> 02.06.2017 02:25, John Snow wrote: >>>> >>>> On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: >>>>> Hi John! >>>>> >>>>> Look at our discussion about this in v18 thread. >>>>>> Shortly: readonly is not the same as disabled. disabled= bitmap just >>>>> ignores all writes. readonly= writes are not allowed at all. >>>>> >>>>> And I think, I'll try to go through way 2: "dirty" field instead of >>>>> "readonly" (look at v18 discussion), as it a bit more flexible. >>>>> >>>> Not sure which I prefer... >>>> >>>> Method 1 is attractive in that it is fairly simple, and enforces fairly >>>> loudly the inability to write to devices with RO bitmaps. It's a >>>> natural >>>> extension of your current approach. >>> >>> For now I decided to realize this one, I think I'll publish it today. >>> Also, I'm going to rename s/readonly/in_use - to avoid the confuse >>> with disabled. So let this field just be mirror of IN_USE in the >>> image and just say "persistent storage knows, that bitmap is in use >>> and may be dirty". > > Finally it would be readonly. in_use is bad for created (not loaded) > bitmaps. I'll add more descriptive comments for disabled and readonly. > Makes sense. It sounds like "readonly" is simply a stricter superset of "disabled," where "disabled" doesn't care if the bitmap gets out of sync with the data, but "readonly" attempts to preserve that semantic relationship. >>> >>> Also, optimization with 'dirty' flag may be added later. >> Yes, I agree. >> And, also, I don't want to influence this "first write", on which we >> will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less >> performance-demanding place than write. >> "And, also," -- I think you've been reading my emails too much, you're picking up my 'isms ;) Thanks, --John
On 02.06.2017 21:46, John Snow wrote: > > On 06/02/2017 05:45 AM, Vladimir Sementsov-Ogievskiy wrote: >> 02.06.2017 12:01, Vladimir Sementsov-Ogievskiy wrote: >>> 02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote: >>>> 02.06.2017 02:25, John Snow wrote: >>>>> On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote: >>>>>> Hi John! >>>>>> >>>>>> Look at our discussion about this in v18 thread. >>>>>>> Shortly: readonly is not the same as disabled. disabled= bitmap just >>>>>> ignores all writes. readonly= writes are not allowed at all. >>>>>> >>>>>> And I think, I'll try to go through way 2: "dirty" field instead of >>>>>> "readonly" (look at v18 discussion), as it a bit more flexible. >>>>>> >>>>> Not sure which I prefer... >>>>> >>>>> Method 1 is attractive in that it is fairly simple, and enforces fairly >>>>> loudly the inability to write to devices with RO bitmaps. It's a >>>>> natural >>>>> extension of your current approach. >>>> For now I decided to realize this one, I think I'll publish it today. >>>> Also, I'm going to rename s/readonly/in_use - to avoid the confuse >>>> with disabled. So let this field just be mirror of IN_USE in the >>>> image and just say "persistent storage knows, that bitmap is in use >>>> and may be dirty". >> Finally it would be readonly. in_use is bad for created (not loaded) >> bitmaps. I'll add more descriptive comments for disabled and readonly. >> > Makes sense. It sounds like "readonly" is simply a stricter superset of > "disabled," where "disabled" doesn't care if the bitmap gets out of sync > with the data, but "readonly" attempts to preserve that semantic > relationship. should we add separate "READONLY" bitmap status for qapi? I'm sure that I don't want to call them "disabled" as it is not the same. "active" is better (as when image is RW they are active, when image is RO, they are active in RO image - not bad too I think. So, may be such addition to qapi would be redundant. > >>>> Also, optimization with 'dirty' flag may be added later. > Yes, I agree. > >>> And, also, I don't want to influence this "first write", on which we >>> will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less >>> performance-demanding place than write. >>> > "And, also," -- I think you've been reading my emails too much, you're > picking up my 'isms ;) > > Thanks, > --John
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 90af37287f..733f19ca5e 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap { int64_t size; /* Size of the bitmap (Number of sectors) */ bool disabled; /* Bitmap is read-only */ int active_iterators; /* How many iterators are active */ + bool readonly; /* Bitmap is read-only and may be changed only + by deserialize* functions */ QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t cur_sector, int64_t nr_sectors) { assert(bdrv_dirty_bitmap_enabled(bitmap)); + assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); } @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t cur_sector, int64_t nr_sectors) { assert(bdrv_dirty_bitmap_enabled(bitmap)); + assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); } void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) { assert(bdrv_dirty_bitmap_enabled(bitmap)); + assert(!bdrv_dirty_bitmap_readonly(bitmap)); if (!out) { hbitmap_reset_all(bitmap->bitmap); } else { @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, if (!bdrv_dirty_bitmap_enabled(bitmap)) { continue; } + assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); } } @@ -540,3 +546,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap) { return hbitmap_count(bitmap->meta); } + +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) +{ + return bitmap->readonly; +} + +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap) +{ + bitmap->readonly = true; +} + +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) +{ + BdrvDirtyBitmap *bm; + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { + if (bm->readonly) { + return true; + } + } + + return false; +} diff --git a/block/io.c b/block/io.c index fdd7485c22..0e28a1f595 100644 --- a/block/io.c +++ b/block/io.c @@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child, uint64_t bytes_remaining = bytes; int max_transfer; + if (bdrv_has_readonly_bitmaps(bs)) { + return -EPERM; + } + assert(is_power_of_2(align)); assert((offset & (align - 1)) == 0); assert((bytes & (align - 1)) == 0); @@ -2437,6 +2441,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, return -ENOMEDIUM; } + if (bdrv_has_readonly_bitmaps(bs)) { + return -EPERM; + } + ret = bdrv_check_byte_request(bs, offset, count); if (ret < 0) { return ret; diff --git a/blockdev.c b/blockdev.c index c63f4e82c7..2b397abf66 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2033,6 +2033,9 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { error_setg(errp, "Cannot clear a disabled bitmap"); return; + } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { + error_setg(errp, "Cannot clear a readonly bitmap"); + return; } bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); @@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, "Bitmap '%s' is currently disabled and cannot be cleared", name); goto out; + } else if (bdrv_dirty_bitmap_readonly(bitmap)) { + error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name); + goto out; } bdrv_clear_dirty_bitmap(bitmap, NULL); diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 1e17729ac2..c0c3ce9f85 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -75,4 +75,8 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, bool finish); void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap); +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); + #endif
It will be needed in following commits for persistent bitmaps. If bitmap is loaded from read-only storage (and we can't mark it "in use" in this storage) corresponding BdrvDirtyBitmap should be read-only. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/dirty-bitmap.c | 28 ++++++++++++++++++++++++++++ block/io.c | 8 ++++++++ blockdev.c | 6 ++++++ include/block/dirty-bitmap.h | 4 ++++ 4 files changed, 46 insertions(+)