On Mon, 05/11 15:22, John Snow wrote:
On 05/06/2015 12:52 AM, Fam Zheng wrote:
Unsetting dirty globally with discard is not very correct. The discard may
zero
out sectors (depending on can_write_zeroes_with_unmap), we should replicate
this change to destinition side to make sure that the guest sees the same
data.
Calling bdrv_reset_dirty also troubles mirror job because the hbitmap
iterator
doesn't expect unsetting of bits after current position.
So let's do it the opposite way which fixes both problems: set the dirty
bits
if we are to discard it.
Reported-by: wangxiaol...@ucloud.cn
Signed-off-by: Fam Zheng f...@redhat.com
---
block/io.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/io.c b/block/io.c
index 1ce62c4..809688b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2343,8 +2343,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState
*bs, int64_t sector_num,
return -EROFS;
}
-bdrv_reset_dirty(bs, sector_num, nb_sectors);
-
/* Do nothing if disabled. */
if (!(bs-open_flags BDRV_O_UNMAP)) {
return 0;
@@ -2354,6 +2352,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState
*bs, int64_t sector_num,
return 0;
}
+bdrv_set_dirty(bs, sector_num, nb_sectors);
+
max_discard = MIN_NON_ZERO(bs-bl.max_discard,
BDRV_REQUEST_MAX_SECTORS);
while (nb_sectors 0) {
int ret;
For the clueless: will discard *always* change the data, or is it
possible that some implementations might do nothing?
It could be nop, partial discard, or an effective write same, depending on the
protocol. For raw, it depends on the host file system.
Is it possible to just omit a set/reset from this function altogether
and let whatever function that is called later (e.g. a write_zeroes
call) worry about setting the dirty bits?
What I wonder about: Is it possible that we are needlessly marking data
as dirty when it has not changed?
No. Because write zeroes path is not involved at all, it's this function that
changes image data. We have to set the dirty bitmap here.
Fam