On 24.12.2015 04:15, Fam Zheng wrote:
> The "pnum < nb_sectors" condition in deciding whether to actually copy
> data is unnecessarily strict, and the qiov initialization is
> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
>
> Rewrite mirror_iteration to fix both flaws.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> block/mirror.c | 344
> +++++++++++++++++++++++++++++++++++----------------------
> trace-events | 1 -
> 2 files changed, 213 insertions(+), 132 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index f201f2b..0081c2e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -46,7 +46,6 @@ typedef struct MirrorBlockJob {
> BlockdevOnError on_source_error, on_target_error;
> bool synced;
> bool should_complete;
> - int64_t sector_num;
> int64_t granularity;
> size_t buf_size;
> int64_t bdev_length;
> @@ -63,6 +62,8 @@ typedef struct MirrorBlockJob {
> int ret;
> bool unmap;
> bool waiting_for_io;
> + int target_cluster_sectors;
> + int max_iov;
> } MirrorBlockJob;
>
> typedef struct MirrorOp {
> @@ -158,115 +159,90 @@ static void mirror_read_complete(void *opaque, int ret)
> mirror_write_complete, op);
> }
>
> -static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> +/* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
> + * return the offset of the adjusted tail sector against original. */
> +static int mirror_cow_align(MirrorBlockJob *s,
> + int64_t *sector_num,
> + int *nb_sectors)
> +{
> + bool head_need_cow, tail_need_cow;
> + int diff = 0;
> + int chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
> +
> + head_need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
> + tail_need_cow = !test_bit((*sector_num + *nb_sectors - 1) /
> chunk_sectors,
> + s->cow_bitmap);
> + if (head_need_cow || tail_need_cow) {
> + int64_t align_sector_num;
> + int align_nb_sectors;
> + bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
> + &align_sector_num, &align_nb_sectors);
> + if (tail_need_cow) {
> + diff = align_sector_num + align_nb_sectors
> + - (*sector_num + *nb_sectors);
> + assert(diff >= 0);
> + *nb_sectors += diff;
> + }
> + if (head_need_cow) {
> + int d = *sector_num - align_sector_num;
> + assert(d >= 0);
> + *sector_num = align_sector_num;
> + *nb_sectors += d;
> + }
> + }
> +
> + /* If the resulting chunks are more than max_iov, we have to shrink it
> + * under the alignment restriction. */
> + if (*nb_sectors / chunk_sectors > s->max_iov) {
> + int shrink = *nb_sectors / chunk_sectors - s->max_iov;
Isn't this missing a "shrink *= chunk_sectors"? Because after this line,
shrink's unit seems to be chunks, but the following code seems to presume it
> + if (tail_need_cow) {
> + shrink -= shrink % s->target_cluster_sectors;
> + }
> + diff -= shrink;
> + *nb_sectors -= shrink;
> + }
Max
(The rest looks fine.)
signature.asc
Description: OpenPGP digital signature
