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 <address@hidden>
---
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