Re: [Qemu-block] [Qemu-devel] [PATCH 1/4] block: Fix dirty bitmap in bdrv_co_discard

[Fam Zheng]

On Wed, 05/06 12:21, Paolo Bonzini wrote:
> 
> 
> On 06/05/2015 11:50, Fam Zheng wrote:
> >  #   src can_write_zeroes_with_unmap       target 
> > can_write_zeroes_with_unmap
> > --------------------------------------------------------------------------------
> >  1              true                                   true
> >  2              true                                   false
> >  3              false                                  true
> >  4              false                                  false
> 
> 1 should replicate WRITE SAME, in case the unmap granularity of the
> target is different from that of the source.  In that case, a discard on
> the target might leave some sectors untouched.  Writing zeroes would
> ensure matching data between the source and the target.
> 
> 2 should _not_ discard: it should write zeroes even at the cost of
> making the target fully provisioned.  Perhaps you can optimize it by
> looking at bdrv_get_block_status for the target, and checking the answer
> for BDRV_ZERO.
> 
> 3 and 4 can use discard on the target.
> 
> So it looks like only the source setting matters.
> 
> We need to check the cost of bdrv_co_get_block_status for "raw", too.
> If it's too expensive, that can be a problem.

In my test, bdrv_is_allocated_above() takes 10~20us on ext4 raw image on my
laptop SATA.  And also note that:

        /*
         * ...
         *
         * 'pnum' is set to the number of sectors (including and immediately 
following
         * the specified sector) that are known to be in the same
         * allocated/unallocated state.
         *
         * 'nb_sectors' is the max value 'pnum' should be set to.  If 
nb_sectors goes
         * beyond the end of the disk image it will be clamped.
         */
        static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState 
*bs,

So we currently don't coalesce the sequential dirty sectors.

Was that intentional?

Fam

> 
> Paolo
> 
> > For case 2 & 3 it's probably better to mirror the actual reading of source.
> > 
> > I'm not sure about 4.
> 
> Even in case 1, discard could be "UNMAP" and write zeroes could be
> "WRITE SAME".  If the unmap granularity of the target is For unaligned
> sectors, UNMAP might leave some sectors aside while WRITE SAME will
> write with zeroes.