04.10.2019 19:31, Max Reitz wrote: > 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.
No, because we shrink tail only if it is already dirty. And we've locked the region for in-flight operation, so nobody can clear the bitmap in a meantime. But still, here is something to do: for not-shrinked tails, if any, we should: 1. align down for reset 2. align up for set on failure > > Apart from that, looks good to me. > > Max > -- Best regards, Vladimir