On Tue, 05/05 15:06, Paolo Bonzini wrote: > > > On 05/05/2015 14:46, 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. > > This is not enough, you also have to do the discard in block/mirror.c, > otherwise the destination image could even become fully provisioned!
I wasn't sure what if src and dest have different can_write_zeroes_with_unmap value, but your argument is stronger. I will add discard in mirror. Besides that, do we need to compare the can_write_zeroes_with_unmap? Fam > > Paolo > > > 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; > >