Re: [Qemu-devel] [PATCH] mirror: Improve zero-write and discard with fragmented image

[Kevin Wolf]

Am 06.11.2015 um 11:22 hat Fam Zheng geschrieben:
> The "pnum < nb_sectors" condition in deciding whether to actually copy
> data is unnecessarily strict, and the qiov initialization is
> unnecessarily too, for both bdrv_aio_write_zeroes and bdrv_aio_discard
> branches.
> 
> Reorganize mirror_iteration flow so that we:
> 
>     1) Find the contiguous zero/discarded sectors with
>     bdrv_get_block_status_above() before deciding what to do. We query
>     s->buf_size sized blocks at a time.
> 
>     2) If the sectors in question are zeroed/discarded and aligned to
>     target cluster, issue zero write or discard accordingly. It's done
>     in mirror_do_zero_or_discard, where we don't add buffer to qiov.
> 
>     3) Otherwise, do the same loop as before in mirror_do_read.
> 
> Signed-off-by: Fam Zheng <address@hidden>

I'm not sure where in the patch to comment on this, so I'll just do it
here right in the beginning.

I'm concerned that we need to be more careful about races in this patch,
in particular regarding the bitmaps. I think the conditions for the two
bitmaps are:

* Dirty bitmap: We must clear the bit after finding the next piece of
  data to be mirrored, but before we yield after getting information
  that we use for the decision which kind of operation we need.

  In other words, we need to clear the dirty bitmap bit before calling
  bdrv_get_block_status_above(), because that's both the function that
  retrieves information about the next chunk and also a function that
  can yield.

  If after this point the data is written to, we need to mirror it
  again.

* In-flight bitmap: We need to make sure that we never mirror the same
  data twice at the same time as older data could overwrite newer data
  then.

  Strictly speaking, it looks to me as if this meant we can delay
  setting the bit until before we issue an AIO operation. It might be
  more obviously correct to set it at the same time as the dirty bitmap
  is cleared.

Otherwise, this looks good to me.

Kevin