[Qemu-devel] mirror questions

[Qemu-devel] mirror questions

[Vladimir Sementsov-Ogievskiy]

Hi!

A question about s->cow_bitmap, introduced in far b812f6719c
     "mirror: perform COW if the cluster size is bigger than the granularity"

cow_bitmap is just a bitmap which tracks, what clusters of target are allocated, to
prevent COW in target, if target backing is not ready yet. And it is done by just
aligning request to be larget and cover unaligned border clusters.

so, questions:

1. Is it correct that we align only on COPY operation in mirror_co_read? Misaligned
write_zero can also lead to COW, I think? Discard seems safe in this way, is it?

2. I don't see, how is it handled, if we align operation so that offset is decreases,
then it may intersect with previous mirror operation request, which may be not yet finished?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] mirror questions

[Kevin Wolf]

Am 26.02.2019 um 09:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi!
> 
> A question about s->cow_bitmap, introduced in far b812f6719c
>      "mirror: perform COW if the cluster size is bigger than the granularity"
> 
> cow_bitmap is just a bitmap which tracks, what clusters of target are allocated, to
> prevent COW in target, if target backing is not ready yet. And it is done by just
> aligning request to be larget and cover unaligned border clusters.
> 
> so, questions:
> 
> 1. Is it correct that we align only on COPY operation in
> mirror_co_read? Misaligned write_zero can also lead to COW, I think?
> Discard seems safe in this way, is it?

It looks a bit odd at least.

There is a bdrv_round_to_clusters() call in mirror_iteration() before
MIRROR_METHOD_ZERO is set. I think this might avoid the COW. However,
how is rounding up the range even correct for write_zeroes and discard?
Can't we destroy data in adjacent sectors this way?

Might be worth writing some test cases and trying to reproduce a bug.

> 2. I don't see, how is it handled, if we align operation so that offset is decreases,
> then it may intersect with previous mirror operation request, which may be not yet finished?

You mean the mirror_cow_align() in mirror_co_read()?

I think it's safe as long as the bitmap granularity is such that
rounding can never affect another bit. This seems to be the case with
default granularity and 64k cluster size in the target image.

If we can't rely on this, it might indeed be unsafe. Another thing worth
trying to reproduce.

Kevin

Re: [Qemu-devel] mirror questions

[Vladimir Sementsov-Ogievskiy]

26.02.2019 12:28, Kevin Wolf wrote:
> Am 26.02.2019 um 09:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi!
>>
>> A question about s->cow_bitmap, introduced in far b812f6719c
>>       "mirror: perform COW if the cluster size is bigger than the granularity"
>>
>> cow_bitmap is just a bitmap which tracks, what clusters of target are allocated, to
>> prevent COW in target, if target backing is not ready yet. And it is done by just
>> aligning request to be larget and cover unaligned border clusters.
>>
>> so, questions:
>>
>> 1. Is it correct that we align only on COPY operation in
>> mirror_co_read? Misaligned write_zero can also lead to COW, I think?
>> Discard seems safe in this way, is it?
> 
> It looks a bit odd at least.
> 
> There is a bdrv_round_to_clusters() call in mirror_iteration() before
> MIRROR_METHOD_ZERO is set. I think this might avoid the COW. However,
> how is rounding up the range even correct for write_zeroes and discard?
> Can't we destroy data in adjacent sectors this way?

hmm, it's OK, as it calculate rounded range in local variables, compare them
with originals and only if not changed do ZERO or DISCARD.. May be not very
efficient but safe.

> 
> Might be worth writing some test cases and trying to reproduce a bug.
> 
>> 2. I don't see, how is it handled, if we align operation so that offset is decreases,
>> then it may intersect with previous mirror operation request, which may be not yet finished?
> 
> You mean the mirror_cow_align() in mirror_co_read()?
> 
> I think it's safe as long as the bitmap granularity is such that
> rounding can never affect another bit. This seems to be the case with
> default granularity and 64k cluster size in the target image.
> 
> If we can't rely on this, it might indeed be unsafe. Another thing worth
> trying to reproduce.


Hmm, not it seems like if we are going to round range than all previous requests were rounded too
so we can't intersect.

But at least, I can easily trigger an assertion in mirror_cow_align():

set granularity=512 for mirror qmp command in TestSingleDrive.test_large_cluster in 041 iotest, and
it fails with

#3  0x00007f6d3b3e20d2 in __GI___assert_fail
     (assertion=0x55b682588351 "ret >= 0", file=0x55b682588342 "block/mirror.c", line=276, function=0x55b6825887c0 <__PRETTY_FUNCTION__.25113> "mirror_cow_align") at assert.c:101
#4  0x000055b682273f01 in mirror_cow_align
     (s=0x55b684aa7400, offset=0x55b684094eb0, bytes=0x55b684094eb8) at block/mirror.c:276
#5  0x000055b682274192 in mirror_co_read (opaque=0x55b684094e90) at block/mirror.c:333


-- 
Best regards,
Vladimir