Re: [Qemu-devel] [PATCH v2 2/6] block: Fix dirty bitmap in bdrv_co_discard
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
Re: [Qemu-devel] [PATCH v2 2/6] block: Fix dirty bitmap in bdrv_co_discard
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? 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?
[Qemu-devel] [PATCH v2 2/6] block: Fix dirty bitmap in bdrv_co_discard
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; -- 1.9.3