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 <d...@openvz.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> 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
