Re: [Qemu-devel] [PATCH v20 10/30] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap
On 03.06.2017 00:02, John Snow wrote:
On 06/02/2017 07:21 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
---
block/dirty-bitmap.c | 32
block/io.c | 8
blockdev.c | 6 ++
include/block/dirty-bitmap.h | 4
4 files changed, 50 insertions(+)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index f25428868c..1c9ffb292a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -45,6 +45,12 @@ struct BdrvDirtyBitmap {
bool disabled; /* Bitmap is disabled. It skips all writes to
the device */
int active_iterators; /* How many iterators are active */
+bool readonly; /* Bitmap is read-only and may be changed only
+ by deserialize* functions. This field blocks
In what way do the deserialize functions change the bitmaps, again?
Hmm, I mean loading bitmap from file, but actually, bitmap is set
readonly after loading, so this sentence half can be dropped and asserts
added to deserialize* functions too (however, deserialize should never
be applied to the bitmap already loaded, or created from qmp,
deserialize should be used with newly created bitmap, by the code like
bitmap-loading or bitmap-incoming-migration, so such asserts may be a
bit strange)
+ any changing operations on owning image
+ (writes and discards), if bitmap is readonly
+ such operations must fail and not change
+ image or this bitmap */
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -437,6 +443,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));
I still feel as if bdrv_dirty_bitmap_enabled() can return false if
bdrv_dirty_bitmap_readonly is true, and you wouldn't have to edit these
parts, but I don't care enough to press the issue.
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
@@ -444,12 +451,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 {
@@ -520,6 +529,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
cur_sector,
if (!bdrv_dirty_bitmap_enabled(bitmap)) {
continue;
}
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
Highlighting the difference in strictness between "disabled" and "readonly."
this case also show that we cant include !readonly-check into
bdrv_dirty_bitmap_enabled()
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
}
@@ -541,3 +551,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, bool value)
+{
+bitmap->readonly = value;
+}
+
+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;
+}
+
Should this be a dynamic error, or an assertion? We should probably
endeavor to never actually hit this circumstance (we should not have
readonly bitmaps on a RW node.)
if on reopening rw write of 'in_use=1' flag failed, we are stay with
readonly bitmaps in rw image. So this is possible.
two notes:
1. we can't fail bdrv_reopen_commit if we failed writing 'in_use=1' flag
into image
2. may be this is not bad: user can remove bitmaps to unblock write, or
he can retry operation leading to reopening rw which should lead to
r
Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap
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 -- Best regards, Vladimir.
Re: [Qemu-devel] [PATCH v20 13/30] block: new bdrv_reopen_bitmaps_rw interface
On 03.06.2017 01:17, John Snow wrote:
On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
Add format driver handler, which should mark loaded read-only
bitmaps as 'IN_USE' in the image and unset read_only field in
corresponding BdrvDirtyBitmap's.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 17 +
include/block/block_int.h | 7 +++
2 files changed, 24 insertions(+)
diff --git a/block.c b/block.c
index 04af7697dc..161db9e32a 100644
--- a/block.c
+++ b/block.c
@@ -2946,12 +2946,16 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
BlockDriverState *bs;
+bool old_can_write, new_can_write;
assert(reopen_state != NULL);
bs = reopen_state->bs;
drv = bs->drv;
assert(drv != NULL);
+old_can_write =
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+
/* If there are any driver level actions to take */
if (drv->bdrv_reopen_commit) {
drv->bdrv_reopen_commit(reopen_state);
@@ -2965,6 +2969,19 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
bdrv_refresh_limits(bs, NULL);
+
+new_can_write =
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
+Error *local_err = NULL;
+if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
+/* This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes.
+ */
+error_report_err(local_err);
+}
+}
}
/*
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..1dc6f2e90d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,6 +380,13 @@ struct BlockDriver {
uint64_t parent_perm, uint64_t parent_shared,
uint64_t *nperm, uint64_t *nshared);
+/**
+ * Bitmaps should be marked as 'IN_USE' in the image on reopening image
+ * as rw. This handler should realize it. It also should unset readonly
+ * field of BlockDirtyBitmap's in case of success.
+ */
+int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
+
Hmm, do we need a new top-level hook for this? We already have
.bdrv_reopen_commit and .bdrv_reopen_prepare which inform the
blockdriver that a reopen event is occurring.
Can't we just amend qcow2_reopen_prepare and qcow2_reopen_commit to do
the necessary tasks of either:
(A) Flushing the bitmap in preparation for reopening as RO, or
(B) Writing in_use and removing the RO flag in preparation for reopening
as RW
(A) is done (see patch about reopen RO)
(B) - We can't do it, as on this preparation the image is still RO, and
I've created new handler, write 'in_use' _after_ the point when new
flags was set.
QLIST_ENTRY(BlockDriver) list;
};
Well, design issues aside, I will at least confirm that I think this
patch should work as designed, and I will leave the design critiques to
Max and Kevin:
Reviewed-by: John Snow
--
Best regards,
Vladimir.
Re: [Qemu-devel] [PATCH 01/12] nbd: rename read_sync and friends
On 31.05.2017 21:46, Eric Blake wrote: On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote: Rename nbd_wr_syncv -> nbd_rwv read_sync -> nbd_read read_sync_eof -> nbd_read_eof write_sync -> nbd_write drop_sync -> nbd_drop 1. nbd_ prefix read_sync and write_sync are already shared, so it is good to have a namespace prefix. drop_sync will be shared, and read_sync_eof is related to read_sync, so let's rename them all. 2. _sync suffix _sync is related to the fact, that nbd_wr_syncv doesn't return if s/fact,/fact/ write to socket returns EAGAIN. In first implementation nbd_wr_syncv just loops while getting EAGAIN, current implementation yields in this case. As mentioned in your followup, you may want to rewrite this to: _sync was originally used (back in commit 7a5ca864 when it was named wr_sync) to indicate that we looped rather than returned on EAGAIN. But now we use qio_channel which yields on our behalf rather than giving us EAGAIN. hmm, I like my wording (with adding note "... implementaion nbd_wr_syncv (was wr_sync in 7a5ca8648b) just ...") more, because: 1. not only nbd_wr_syncv has that suffix, so nbd_wr_syncv should be mentioned (as we mention wr_sync) 2. I don't say about contrast between old and current, I say that they are similar. Why to get rid of it: - it is normal for r/w functions to be synchronous, so having additional suffix for it looks redundant (contrariwise, we have _aio suffix for async functions) - _sync suffix in block layer is used when function does flush (so using it for other thing is confusing a bit) - keep function names short after adding nbd_ prefix 3. for nbd_wr_syncv let's use more common notation 'rw' Signed-off-by: Vladimir Sementsov-Ogievskiy --- The maintainer may be willing to tweak the commit message without needing a v2. I hope for it) +++ b/nbd/nbd-internal.h @@ -94,14 +94,14 @@ #define NBD_ENOSPC 28 #define NBD_ESHUTDOWN 108 -/* read_sync_eof +/* nbd_read_eof * Tries to read @size bytes from @ioc. Returns number of bytes actually read. * May return a value >= 0 and < size only on EOF, i.e. when iteratively called - * qio_channel_readv() returns 0. So, there are no needs to call read_sync_eof + * qio_channel_readv() returns 0. So, there are no needs to call nbd_read_eof As long as you are touching this: s/are no needs/is no need/ Reviewed-by: Eric Blake -- Best regards, Vladimir.
Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap
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
---
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->b
Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap
On 01.06.2017 01:58, John Snow wrote:
On 05/19/2017 07:02 PM, John Snow wrote:
On 05/03/2017 08:25 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
---
block/dirty-bitmap.c | 16
include/block/dirty-bitmap.h | 3 +++
2 files changed, 19 insertions(+)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..ab6a95cf41 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));
Doesn't this change the nature of the assertion?
We're going from
return !(bitmap->disabled || bitmap->successor);
to
!bitmap->readonly
That makes me a little nervous to ACK this patch, because the
correctness depends on how readonly is updated and manipulated in the
future, which makes it more prone to error.
I must have been *REALLY* tired when I sent this; I had thought you were
replacing an assertion instead of just adding one.
I'm sorry about that.
I thought so, don't worry)
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,13 @@ 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;
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729ac2..0aab5841f5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -75,4 +75,7 @@ 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);
+
#endif
--
Best regards,
Vladimir.
