Re: [PATCH 3/4] block/mirror: support unaligned write in active mirror

[Max Reitz]

On 12.09.19 17:13, Vladimir Sementsov-Ogievskiy wrote:
> Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
> region in the dirty bitmap, which means that we may not copy some bytes
> and assume them copied, which actually leads to producing corrupted
> target.
> 
> So 9adc1cb49af8d forced dirty bitmap granularity to be
> request_alignment for mirror-top filter, so we are not working with
> unaligned requests. However forcing large alignment obviously decreases
> performance of unaligned requests.
> 
> This commit provides another solution for the problem: if unaligned
> padding is already dirty, we can safely ignore it, as
> 1. It's dirty, it will be copied by mirror_iteration anyway
> 2. It's dirty, so skipping it now we don't increase dirtiness of the
>    bitmap and therefore don't damage "synchronicity" of the
>    write-blocking mirror.
> 
> If unaligned padding is not dirty, we just write it, no reason to touch
> dirty bitmap if we succeed (on failure we'll set the whole region
> ofcourse, but we loss "synchronicity" on failure anyway).
> 
> Note: we need to disable dirty_bitmap, otherwise we will not be able to
> see in do_sync_target_write bitmap state before current operation. We
> may of course check dirty bitmap before the operation in
> bdrv_mirror_top_do_write and remember it, but we don't need active
> dirty bitmap for write-blocking mirror anyway.
> 
> New code-path is unused until the following commit reverts
> 9adc1cb49af8d.
> 
> Suggested-by: Denis V. Lunev <address@hidden>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/mirror.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index d176bf5920..d192f6a96b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1204,6 +1204,39 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod 
> method,
>                       QEMUIOVector *qiov, int flags)
>  {
>      int ret;
> +    size_t qiov_offset = 0;
> +
> +    if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
> +        bdrv_dirty_bitmap_get(job->dirty_bitmap, offset)) {
> +            /*
> +             * Dirty unaligned padding
> +             * 1. It's already dirty, no damage to "actively_synced" if we 
> just
> +             *    skip unaligned part.
> +             * 2. If we copy it, we can't reset corresponding bit in
> +             *    dirty_bitmap as there may be some "dirty" bytes still not
> +             *    copied.
> +             * So, just ignore it.
> +             */
> +            qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - offset;
> +            if (bytes <= qiov_offset) {
> +                /* nothing to do after shrink */
> +                return;
> +            }
> +            offset += qiov_offset;
> +            bytes -= qiov_offset;
> +    }
> +
> +    if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
> +        bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
> +    {
> +        uint64_t tail = (offset + bytes) % job->granularity;
> +
> +        if (bytes <= tail) {
> +            /* nothing to do after shrink */
> +            return;
> +        }
> +        bytes -= tail;
> +    }
>  
>      bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);
>  

The bdrv_set_dirty_bitmap() in the error case below needs to use the
original offset/bytes, I suppose.

Apart from that, looks good to me.

Max

signature.asc
Description: OpenPGP digital signature