Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/6] block: Fix dirty bitmap in bdrv_co_discard

[Fam Zheng]

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: address@hidden
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  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