qcow2 merge_cow() question

qcow2 merge_cow() question

[Vladimir Sementsov-Ogievskiy]

Hi!

I'm sorry, I feel I already asked about it, but can't now find the answer.

What are these ifs for?

           /* The data (middle) region must be immediately after the
            * start region */
           if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
               continue;
           }
                                                                                                           
           /* The end region must be immediately after the data (middle)
            * region */
           if (m->offset + m->cow_end.offset != offset + bytes) {
               continue;
           }

How is it possible that data doesn't immediately follow start cow region or
end cow region doesn't immediately follow data region?


-- 
Best regards,
Vladimir

Re: qcow2 merge_cow() question

[Alberto Garcia]

On Fri 21 Aug 2020 02:32:00 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
>
> I'm sorry, I feel I already asked about it, but can't now find the answer.
>
> What are these ifs for?
>
>            /* The data (middle) region must be immediately after the
>             * start region */
>            if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
>                continue;
>            }
>                                                                                                            
>            /* The end region must be immediately after the data (middle)
>             * region */
>            if (m->offset + m->cow_end.offset != offset + bytes) {
>                continue;
>            }
>
> How is it possible that data doesn't immediately follow start cow region or
> end cow region doesn't immediately follow data region?

They are sanity checks. They maybe cannot happen in practice and in that
case I suppose they should be replaced with assertions but this should
be checked carefully. If I remember correctly I was wary of overlooking
a case where this could happen.

In particular, that function receives only one data region but a list of
QCowL2Meta objects. I think you can get more than one QCowL2Meta if the
same request involves a mix of copied and newly allocated clusters, but
that shouldn't be a problem either.

Berto

Re: qcow2 merge_cow() question

[Vladimir Sementsov-Ogievskiy]

21.08.2020 15:52, Alberto Garcia wrote:
> On Fri 21 Aug 2020 02:32:00 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> Hi!
>>
>> I'm sorry, I feel I already asked about it, but can't now find the answer.
>>
>> What are these ifs for?
>>
>>             /* The data (middle) region must be immediately after the
>>              * start region */
>>             if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
>>                 continue;
>>             }
>>                                                                                                             
>>             /* The end region must be immediately after the data (middle)
>>              * region */
>>             if (m->offset + m->cow_end.offset != offset + bytes) {
>>                 continue;
>>             }
>>
>> How is it possible that data doesn't immediately follow start cow region or
>> end cow region doesn't immediately follow data region?
> 
> They are sanity checks. They maybe cannot happen in practice and in that
> case I suppose they should be replaced with assertions but this should
> be checked carefully. If I remember correctly I was wary of overlooking
> a case where this could happen.
> 
> In particular, that function receives only one data region but a list of
> QCowL2Meta objects. I think you can get more than one QCowL2Meta if the
> same request involves a mix of copied and newly allocated clusters, but
> that shouldn't be a problem either.
> 
> Berto
> 

OK, thanks. So, intuitively it shouldn't happen, but there should be
some careful investigation to change them to assertions.

I think that the list actually may contain not more than 2 entries.
So the whole diversity is that we may have both cow regions in one
meta, or in two separate metas.

-- 
Best regards,
Vladimir

Re: qcow2 merge_cow() question

[Alberto Garcia]

On Fri 21 Aug 2020 03:42:29 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>> What are these ifs for?
>>>
>>>             /* The data (middle) region must be immediately after the
>>>              * start region */
>>>             if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
>>>                 continue;
>>>             }
>>>                                                                                                             
>>>             /* The end region must be immediately after the data (middle)
>>>              * region */
>>>             if (m->offset + m->cow_end.offset != offset + bytes) {
>>>                 continue;
>>>             }
>>>
>>> How is it possible that data doesn't immediately follow start cow
>>> region or end cow region doesn't immediately follow data region?
>> 
>> They are sanity checks. They maybe cannot happen in practice and in
>> that case I suppose they should be replaced with assertions but this
>> should be checked carefully. If I remember correctly I was wary of
>> overlooking a case where this could happen.
>> 
>> In particular, that function receives only one data region but a list
>> of QCowL2Meta objects. I think you can get more than one QCowL2Meta
>> if the same request involves a mix of copied and newly allocated
>> clusters, but that shouldn't be a problem either.
>
> OK, thanks. So, intuitively it shouldn't happen, but there should be
> some careful investigation to change them to assertions.

I was having a look at this and here's a simple example of how this can
happen:

qemu-img create -f qcow2 -o cluster_size=1k img.qcow2 1M
qemu-io -c 'write 0 3k' img.qcow2
qemu-io -c 'discard 0 1k' img.qcow2
qemu-io -c 'discard 2k 1k' img.qcow2
qemu-io -c 'write 512 2k' img.qcow2

The last write request can be performed with one single write operation
but it needs to allocate clusters #0 and #2.

This means that merge_cow() is called with offset=512, bytes=2k and two
QCowL2Meta structures:

  - The first one with cow_start={0, 512} and cow_end={1k, 0} 
  - The second one with cow_start={2k, 0} and cow_end={2560, 512}

In theory it should be possible to combine both into one that has
cow_start={0, 512} and cow_end={2560, 512}, but I don't think this
situation happens very often so I wouldn't go that way.

In any case, the checks have to remain and they cannot be turned into
assertions.

Berto

Re: qcow2 merge_cow() question

[Vladimir Sementsov-Ogievskiy]

29.09.2020 18:02, Alberto Garcia wrote:
> On Fri 21 Aug 2020 03:42:29 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>>> What are these ifs for?
>>>>
>>>>              /* The data (middle) region must be immediately after the
>>>>               * start region */
>>>>              if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
>>>>                  continue;
>>>>              }
>>>>                                                                                                              
>>>>              /* The end region must be immediately after the data (middle)
>>>>               * region */
>>>>              if (m->offset + m->cow_end.offset != offset + bytes) {
>>>>                  continue;
>>>>              }
>>>>
>>>> How is it possible that data doesn't immediately follow start cow
>>>> region or end cow region doesn't immediately follow data region?
>>>
>>> They are sanity checks. They maybe cannot happen in practice and in
>>> that case I suppose they should be replaced with assertions but this
>>> should be checked carefully. If I remember correctly I was wary of
>>> overlooking a case where this could happen.
>>>
>>> In particular, that function receives only one data region but a list
>>> of QCowL2Meta objects. I think you can get more than one QCowL2Meta
>>> if the same request involves a mix of copied and newly allocated
>>> clusters, but that shouldn't be a problem either.
>>
>> OK, thanks. So, intuitively it shouldn't happen, but there should be
>> some careful investigation to change them to assertions.
> 
> I was having a look at this and here's a simple example of how this can
> happen:
> 
> qemu-img create -f qcow2 -o cluster_size=1k img.qcow2 1M
> qemu-io -c 'write 0 3k' img.qcow2
> qemu-io -c 'discard 0 1k' img.qcow2
> qemu-io -c 'discard 2k 1k' img.qcow2
> qemu-io -c 'write 512 2k' img.qcow2
> 
> The last write request can be performed with one single write operation
> but it needs to allocate clusters #0 and #2.
> 
> This means that merge_cow() is called with offset=512, bytes=2k and two
> QCowL2Meta structures:
> 
>    - The first one with cow_start={0, 512} and cow_end={1k, 0}
>    - The second one with cow_start={2k, 0} and cow_end={2560, 512}
> 
> In theory it should be possible to combine both into one that has
> cow_start={0, 512} and cow_end={2560, 512}, but I don't think this
> situation happens very often so I wouldn't go that way.
> 
> In any case, the checks have to remain and they cannot be turned into
> assertions.
> 

Hmm, very interesting, thanks! So, at least "empty" cow regions are possible inside data region. It's good to keep in mind. I'm sure that some good refactoring is possible here.. Maybe in the distant bright future :)


-- 
Best regards,
Vladimir