[Qemu-devel] callout to *file in bdrv_co_get_block_status

[Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Hi,


I tried to debug why qemu-img convert with a VMDK source laying on a tmpfs is horrible slow.

For some reason a lseek on a tmpfs is slow. Strictly speaking the lseek in find_allocation in file-posix.c

is slow.


When qemu-img convert iterates over all sectors of a VMDK file to check their allocation status it ends

up checking allocation status of all allocated sectors due to the following condition in bdrv_co_get_block_status:


    if (*file && *file != bs &&
        (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
        (ret & BDRV_BLOCK_OFFSET_VALID)) {
        BlockDriverState *file2;
        int file_pnum;
        ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
                                        *pnum, &file_pnum, &file2);
        if (ret2 >= 0) {
            /* Ignore errors.  This is just providing extra information, it
             * is useful but not necessary.
             */
            if (!file_pnum) {
                /* !file_pnum indicates an offset at or beyond the EOF; it is
                 * perfectly valid for the format block driver to point to such
                 * offsets, so catch it and mark everything as zero */
                ret |= BDRV_BLOCK_ZERO;
            } else {
                /* Limit request to the range reported by the protocol driver */
                *pnum = file_pnum;
                ret |= (ret2 & BDRV_BLOCK_ZERO);
            }
        }
    }


Does anybody remember for what circumstances this case this was added? In case of an container format

like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?


Thanks,

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 17/03/2017 11:45, Peter Lieven wrote:
> Hi,
> 
> 
> I tried to debug why qemu-img convert with a VMDK source laying on a tmpfs is horrible slow.
> 
> For some reason a lseek on a tmpfs is slow. Strictly speaking the lseek in find_allocation in file-posix.c
> 
> is slow.
> 
> 
> When qemu-img convert iterates over all sectors of a VMDK file to check their allocation status it ends
> 
> up checking allocation status of all allocated sectors due to the following condition in bdrv_co_get_block_status:
> 
> 
>     if (*file && *file != bs &&
>         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
>         (ret & BDRV_BLOCK_OFFSET_VALID)) {
>         BlockDriverState *file2;
>         int file_pnum;
>         ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
>                                         *pnum, &file_pnum, &file2);
>         if (ret2 >= 0) {
>             /* Ignore errors.  This is just providing extra information, it
>              * is useful but not necessary.
>              */
>             if (!file_pnum) {
>                 /* !file_pnum indicates an offset at or beyond the EOF; it is
>                  * perfectly valid for the format block driver to point to such
>                  * offsets, so catch it and mark everything as zero */
>                 ret |= BDRV_BLOCK_ZERO;
>             } else {
>                 /* Limit request to the range reported by the protocol driver */
>                 *pnum = file_pnum;
>                 ret |= (ret2 & BDRV_BLOCK_ZERO);
>             }
>         }
>     }
> 
> 
> Does anybody remember for what circumstances this case this was added? In case of an container format
> 
> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?

It provides additional information, for example it works better with
prealloc=metadata.

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 17.03.2017 um 11:59 schrieb Paolo Bonzini:
>
> On 17/03/2017 11:45, Peter Lieven wrote:
>> Hi,
>>
>>
>> I tried to debug why qemu-img convert with a VMDK source laying on a tmpfs is horrible slow.
>>
>> For some reason a lseek on a tmpfs is slow. Strictly speaking the lseek in find_allocation in file-posix.c
>>
>> is slow.
>>
>>
>> When qemu-img convert iterates over all sectors of a VMDK file to check their allocation status it ends
>>
>> up checking allocation status of all allocated sectors due to the following condition in bdrv_co_get_block_status:
>>
>>
>>     if (*file && *file != bs &&
>>         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
>>         (ret & BDRV_BLOCK_OFFSET_VALID)) {
>>         BlockDriverState *file2;
>>         int file_pnum;
>>         ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
>>                                         *pnum, &file_pnum, &file2);
>>         if (ret2 >= 0) {
>>             /* Ignore errors.  This is just providing extra information, it
>>              * is useful but not necessary.
>>              */
>>             if (!file_pnum) {
>>                 /* !file_pnum indicates an offset at or beyond the EOF; it is
>>                  * perfectly valid for the format block driver to point to such
>>                  * offsets, so catch it and mark everything as zero */
>>                 ret |= BDRV_BLOCK_ZERO;
>>             } else {
>>                 /* Limit request to the range reported by the protocol driver */
>>                 *pnum = file_pnum;
>>                 ret |= (ret2 & BDRV_BLOCK_ZERO);
>>             }
>>         }
>>     }
>>
>>
>> Does anybody remember for what circumstances this case this was added? In case of an container format
>>
>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
> It provides additional information, for example it works better with
> prealloc=metadata.

Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 17/03/2017 12:11, Peter Lieven wrote:
>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>> It provides additional information, for example it works better with
>> prealloc=metadata.
> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?

If the metadata is preallocated, cluster will (or should) show up as
zero, speeding up the copy.

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>
> On 17/03/2017 12:11, Peter Lieven wrote:
>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>> It provides additional information, for example it works better with
>>> prealloc=metadata.
>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
> If the metadata is preallocated, cluster will (or should) show up as
> zero, speeding up the copy.

Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
So where does it actually help?

The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))

So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
BDRV_BLOCK_ZERO.

This can only happen if I partially write to a cluster, or am I wrong here?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Fam Zheng]

On Fri, 03/17 12:16, Paolo Bonzini wrote:
> 
> 
> On 17/03/2017 12:11, Peter Lieven wrote:
> >>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
> >> It provides additional information, for example it works better with
> >> prealloc=metadata.
> > Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
> > lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
> > it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
> 
> If the metadata is preallocated, cluster will (or should) show up as
> zero, speeding up the copy.

I think from qemu-img convert's perspective, it doesn't care about the *file
status if the metadata already speaks, because, like you said, the data shows up
as zeroes.

In other words I think this can be optimized.

Fam

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 17/03/2017 12:24, Fam Zheng wrote:
> On Fri, 03/17 12:16, Paolo Bonzini wrote:
>>
>>
>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>> It provides additional information, for example it works better with
>>>> prealloc=metadata.
>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>
>> If the metadata is preallocated, cluster will (or should) show up as
>> zero, speeding up the copy.
> 
> I think from qemu-img convert's perspective, it doesn't care about the *file
> status if the metadata already speaks, because, like you said, the data shows up
> as zeroes.

That's already the case: *file is only examined if the metadata  says
BDRV_BLOCK_DATA=1, BDRV_BLOCK_ZERO=0.

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 17.03.2017 um 15:51 schrieb Paolo Bonzini:
>
> On 17/03/2017 12:24, Fam Zheng wrote:
>> On Fri, 03/17 12:16, Paolo Bonzini wrote:
>>>
>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>> It provides additional information, for example it works better with
>>>>> prealloc=metadata.
>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>> If the metadata is preallocated, cluster will (or should) show up as
>>> zero, speeding up the copy.
>> I think from qemu-img convert's perspective, it doesn't care about the *file
>> status if the metadata already speaks, because, like you said, the data shows up
>> as zeroes.
> That's already the case: *file is only examined if the metadata  says
> BDRV_BLOCK_DATA=1, BDRV_BLOCK_ZERO=0.

Maybe Fam meant in qemu-img this info is not that necessary because it will skip zeroes inside
a datablock anyway. I don't know why the lseek is soo slow, but the optimization could be to identify
the case where this extra info of the *file containing zeroes is really useful and then only call out for
it in those cases.

Peter

>
> Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Fam Zheng]

On Fri, 03/17 12:20, Peter Lieven wrote:
> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
> >
> > On 17/03/2017 12:11, Peter Lieven wrote:
> >>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
> >>> It provides additional information, for example it works better with
> >>> prealloc=metadata.
> >> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
> >> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
> >> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
> > If the metadata is preallocated, cluster will (or should) show up as
> > zero, speeding up the copy.
> 
> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
> So where does it actually help?
> 
> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
> 
> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
> BDRV_BLOCK_ZERO.
> 
> This can only happen if I partially write to a cluster, or am I wrong here?

I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
know.

Fam

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 20/03/2017 03:46, Fam Zheng wrote:
> On Fri, 03/17 12:20, Peter Lieven wrote:
>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>
>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>> It provides additional information, for example it works better with
>>>>> prealloc=metadata.
>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>> If the metadata is preallocated, cluster will (or should) show up as
>>> zero, speeding up the copy.
>>
>> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
>> So where does it actually help?
>>
>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
>>
>> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
>> BDRV_BLOCK_ZERO.
>>
>> This can only happen if I partially write to a cluster, or am I wrong here?
> 
> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
> protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
> know.

That's true of qcow2, but many formats (including raw :)) don't know
about BDRV_BLOCK_ZERO.

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Fam Zheng]

On Mon, 03/20 12:21, Paolo Bonzini wrote:
> 
> 
> On 20/03/2017 03:46, Fam Zheng wrote:
> > On Fri, 03/17 12:20, Peter Lieven wrote:
> >> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
> >>>
> >>> On 17/03/2017 12:11, Peter Lieven wrote:
> >>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
> >>>>> It provides additional information, for example it works better with
> >>>>> prealloc=metadata.
> >>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
> >>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
> >>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
> >>> If the metadata is preallocated, cluster will (or should) show up as
> >>> zero, speeding up the copy.
> >>
> >> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
> >> So where does it actually help?
> >>
> >> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
> >>
> >> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
> >> BDRV_BLOCK_ZERO.
> >>
> >> This can only happen if I partially write to a cluster, or am I wrong here?
> > 
> > I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
> > protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
> > know.
> 
> That's true of qcow2, but many formats (including raw :)) don't know
> about BDRV_BLOCK_ZERO.

Raw is a little special, it could have forwarded the call to *file in its
BlockDriver callback. Most formats with metadata stores zero/nonzero information
in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta data on
zero/nonzero.

Fam

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 12:49 schrieb Fam Zheng:
> On Mon, 03/20 12:21, Paolo Bonzini wrote:
>>
>> On 20/03/2017 03:46, Fam Zheng wrote:
>>> On Fri, 03/17 12:20, Peter Lieven wrote:
>>>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>>>> It provides additional information, for example it works better with
>>>>>>> prealloc=metadata.
>>>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>>>> If the metadata is preallocated, cluster will (or should) show up as
>>>>> zero, speeding up the copy.
>>>> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
>>>> So where does it actually help?
>>>>
>>>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
>>>>
>>>> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
>>>> BDRV_BLOCK_ZERO.
>>>>
>>>> This can only happen if I partially write to a cluster, or am I wrong here?
>>> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
>>> protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
>>> know.
>> That's true of qcow2, but many formats (including raw :)) don't know
>> about BDRV_BLOCK_ZERO.
> Raw is a little special, it could have forwarded the call to *file in its
> BlockDriver callback. Most formats with metadata stores zero/nonzero information

I thinks thats the check *file != bs for, right?


> in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta data on
> zero/nonzero.

So what are the really Formats that we don't trust? And is it worth that
we add this (maybe expensive) callout to lseek via find_allocation for any
format altough we already know the answer from the metadata?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 12:49 schrieb Fam Zheng:
> On Mon, 03/20 12:21, Paolo Bonzini wrote:
>>
>> On 20/03/2017 03:46, Fam Zheng wrote:
>>> On Fri, 03/17 12:20, Peter Lieven wrote:
>>>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>>>> It provides additional information, for example it works better with
>>>>>>> prealloc=metadata.
>>>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>>>> If the metadata is preallocated, cluster will (or should) show up as
>>>>> zero, speeding up the copy.
>>>> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
>>>> So where does it actually help?
>>>>
>>>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
>>>>
>>>> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
>>>> BDRV_BLOCK_ZERO.
>>>>
>>>> This can only happen if I partially write to a cluster, or am I wrong here?
>>> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
>>> protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
>>> know.
>> That's true of qcow2, but many formats (including raw :)) don't know
>> about BDRV_BLOCK_ZERO.
> Raw is a little special, it could have forwarded the call to *file in its
> BlockDriver callback. Most formats with metadata stores zero/nonzero information
> in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta data on
> zero/nonzero.
>
> Fam


BTW, the extra check was added in


commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Sep 4 19:00:38 2013 +0200

    block: look for zero blocks in bs->file
   
    Reviewed-by: Eric Blake <eblake@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>


It was introduced while introducing bdv_get_block_status. I don't know what the real

issue was that was addressed with this patch?


Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 13:47 schrieb Peter Lieven:
> Am 20.03.2017 um 12:49 schrieb Fam Zheng:
>> On Mon, 03/20 12:21, Paolo Bonzini wrote:
>>> On 20/03/2017 03:46, Fam Zheng wrote:
>>>> On Fri, 03/17 12:20, Peter Lieven wrote:
>>>>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>>>>> It provides additional information, for example it works better with
>>>>>>>> prealloc=metadata.
>>>>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>>>>> If the metadata is preallocated, cluster will (or should) show up as
>>>>>> zero, speeding up the copy.
>>>>> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
>>>>> So where does it actually help?
>>>>>
>>>>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
>>>>>
>>>>> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
>>>>> BDRV_BLOCK_ZERO.
>>>>>
>>>>> This can only happen if I partially write to a cluster, or am I wrong here?
>>>> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
>>>> protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
>>>> know.
>>> That's true of qcow2, but many formats (including raw :)) don't know
>>> about BDRV_BLOCK_ZERO.
>> Raw is a little special, it could have forwarded the call to *file in its
>> BlockDriver callback. Most formats with metadata stores zero/nonzero information
>> in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta data on
>> zero/nonzero.
>>
>> Fam
>
> BTW, the extra check was added in
>
>
> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Wed Sep 4 19:00:38 2013 +0200
>
>     block: look for zero blocks in bs->file
>    
>     Reviewed-by: Eric Blake <eblake@redhat.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
>
> It was introduced while introducing bdv_get_block_status. I don't know what the real
>
> issue was that was addressed with this patch?

Is it possible that this optimization was added especially for RAW? I was believing that
raw would forward the get_block_status call to bs->file, but it looks it doesn't.
If this one here was for RAW would it be an option to move this callout to the raw-format driver
and remove it from the generic code?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 20/03/2017 14:13, Peter Lieven wrote:
> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>> Am 20.03.2017 um 12:49 schrieb Fam Zheng:
>>> On Mon, 03/20 12:21, Paolo Bonzini wrote:
>>>> On 20/03/2017 03:46, Fam Zheng wrote:
>>>>> On Fri, 03/17 12:20, Peter Lieven wrote:
>>>>>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>>>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>>>>>> It provides additional information, for example it works better with
>>>>>>>>> prealloc=metadata.
>>>>>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>>>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>>>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>>>>>> If the metadata is preallocated, cluster will (or should) show up as
>>>>>>> zero, speeding up the copy.
>>>>>> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
>>>>>> So where does it actually help?
>>>>>>
>>>>>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
>>>>>>
>>>>>> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
>>>>>> BDRV_BLOCK_ZERO.
>>>>>>
>>>>>> This can only happen if I partially write to a cluster, or am I wrong here?
>>>>> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
>>>>> protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
>>>>> know.
>>>> That's true of qcow2, but many formats (including raw :)) don't know
>>>> about BDRV_BLOCK_ZERO.
>>> Raw is a little special, it could have forwarded the call to *file in its
>>> BlockDriver callback. Most formats with metadata stores zero/nonzero information
>>> in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta data on
>>> zero/nonzero.
>>>
>>> Fam
>>
>> BTW, the extra check was added in
>>
>>
>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>
>>     block: look for zero blocks in bs->file
>>    
>>     Reviewed-by: Eric Blake <eblake@redhat.com>
>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>>
>> It was introduced while introducing bdv_get_block_status. I don't know what the real
>>
>> issue was that was addressed with this patch?
> 
> Is it possible that this optimization was added especially for RAW? I was believing that
> raw would forward the get_block_status call to bs->file, but it looks it doesn't.
> If this one here was for RAW would it be an option to move this callout to the raw-format driver
> and remove it from the generic code?

It was meant for both raw and qcow2.

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 14:23 schrieb Paolo Bonzini:
>
> On 20/03/2017 14:13, Peter Lieven wrote:
>> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>>> Am 20.03.2017 um 12:49 schrieb Fam Zheng:
>>>> On Mon, 03/20 12:21, Paolo Bonzini wrote:
>>>>> On 20/03/2017 03:46, Fam Zheng wrote:
>>>>>> On Fri, 03/17 12:20, Peter Lieven wrote:
>>>>>>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>>>>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 tables in the VMDK or QCOW2?
>>>>>>>>>> It provides additional information, for example it works better with
>>>>>>>>>> prealloc=metadata.
>>>>>>>>> Okay, understood. Can you imagine of a away to conditionally avoid this second callout? In my case we have an additional
>>>>>>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls to lseek. And if the file has no preallocated metadata
>>>>>>>>> it will likely not improve anything. And even if the metadata is prealloced what is the allocation status of the clusters?
>>>>>>>> If the metadata is preallocated, cluster will (or should) show up as
>>>>>>>> zero, speeding up the copy.
>>>>>>> Okay, in this case the second call out to *file will not happen. It only happens if the metadata says it contains data.
>>>>>>> So where does it actually help?
>>>>>>>
>>>>>>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID))
>>>>>>>
>>>>>>> So from my view it can only have any effect if the metadata returns BDRV_BLOCK_DATA, but the protocol driver returns
>>>>>>> BDRV_BLOCK_ZERO.
>>>>>>>
>>>>>>> This can only happen if I partially write to a cluster, or am I wrong here?
>>>>>> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO if
>>>>>> protocol would say BDRV_BLOCK_ZERO - there is no reason the format driver cannot
>>>>>> know.
>>>>> That's true of qcow2, but many formats (including raw :)) don't know
>>>>> about BDRV_BLOCK_ZERO.
>>>> Raw is a little special, it could have forwarded the call to *file in its
>>>> BlockDriver callback. Most formats with metadata stores zero/nonzero information
>>>> in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta data on
>>>> zero/nonzero.
>>>>
>>>> Fam
>>> BTW, the extra check was added in
>>>
>>>
>>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>>
>>>     block: look for zero blocks in bs->file
>>>    
>>>     Reviewed-by: Eric Blake <eblake@redhat.com>
>>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>
>>>
>>> It was introduced while introducing bdv_get_block_status. I don't know what the real
>>>
>>> issue was that was addressed with this patch?
>> Is it possible that this optimization was added especially for RAW? I was believing that
>> raw would forward the get_block_status call to bs->file, but it looks it doesn't.
>> If this one here was for RAW would it be an option to move this callout to the raw-format driver
>> and remove it from the generic code?
> It was meant for both raw and qcow2.

Okay, but as Fam mentioned qcow2 Metadata should know that a cluster is zero. Do you remember
what the issue was?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 20/03/2017 14:35, Peter Lieven wrote:
> Am 20.03.2017 um 14:23 schrieb Paolo Bonzini:
>> On 20/03/2017 14:13, Peter Lieven wrote:
>>> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>>>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>>>
>>>>     block: look for zero blocks in bs->file
>>>>    
>>>>     Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>
>>>>
>>>> It was introduced while introducing bdv_get_block_status. I don't know what the real
>>>>
>>>> issue was that was addressed with this patch?
>>> Is it possible that this optimization was added especially for RAW? I was believing that
>>> raw would forward the get_block_status call to bs->file, but it looks it doesn't.
>>> If this one here was for RAW would it be an option to move this callout to the raw-format driver
>>> and remove it from the generic code?
>> It was meant for both raw and qcow2.
> 
> Okay, but as Fam mentioned qcow2 Metadata should know that a cluster is zero. Do you remember
> what the issue was?

I said that already---preallocated metadata.  Also, at the time
pre-qcow2v3 was more important.

Are you using libiscsi, block devices or files?

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 15:05 schrieb Paolo Bonzini:
>
> On 20/03/2017 14:35, Peter Lieven wrote:
>> Am 20.03.2017 um 14:23 schrieb Paolo Bonzini:
>>> On 20/03/2017 14:13, Peter Lieven wrote:
>>>> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>>>>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>>>>
>>>>>     block: look for zero blocks in bs->file
>>>>>    
>>>>>     Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>
>>>>>
>>>>> It was introduced while introducing bdv_get_block_status. I don't know what the real
>>>>>
>>>>> issue was that was addressed with this patch?
>>>> Is it possible that this optimization was added especially for RAW? I was believing that
>>>> raw would forward the get_block_status call to bs->file, but it looks it doesn't.
>>>> If this one here was for RAW would it be an option to move this callout to the raw-format driver
>>>> and remove it from the generic code?
>>> It was meant for both raw and qcow2.
>> Okay, but as Fam mentioned qcow2 Metadata should know that a cluster is zero. Do you remember
>> what the issue was?
> I said that already---preallocated metadata.  Also, at the time
> pre-qcow2v3 was more important.

Yes, but Fam said that with preallocated metadata the clusters should be zero, or was that
not true before qcow2v3?

>
> Are you using libiscsi, block devices or files?

Its a mixture. raw with libiscsi or lvm and qcow2 and vmdk either with libnfs or on local storage.

I stumbled across the issue with lseek on a tmpfs because in the build process for our templates
I temporarily have vmdks on a tmpfs and it takes ages before qemu-img convert starts to run (it iterates
over every 64kb cluster with that callout to find_allocation and for some reason lseek is very slow on tmpfs).

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 20/03/2017 17:43, Peter Lieven wrote:
> Am 20.03.2017 um 15:05 schrieb Paolo Bonzini:
>>
>> On 20/03/2017 14:35, Peter Lieven wrote:
>>> Am 20.03.2017 um 14:23 schrieb Paolo Bonzini:
>>>> On 20/03/2017 14:13, Peter Lieven wrote:
>>>>> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>>>>>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>>>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>>>>>
>>>>>>     block: look for zero blocks in bs->file
>>>>>>    
>>>>>>     Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>>
>>>>>>
>>>>>> It was introduced while introducing bdv_get_block_status. I don't know what the real
>>>>>>
>>>>>> issue was that was addressed with this patch?
>>>>> Is it possible that this optimization was added especially for RAW? I was believing that
>>>>> raw would forward the get_block_status call to bs->file, but it looks it doesn't.
>>>>> If this one here was for RAW would it be an option to move this callout to the raw-format driver
>>>>> and remove it from the generic code?
>>>> It was meant for both raw and qcow2.
>>> Okay, but as Fam mentioned qcow2 Metadata should know that a cluster is zero. Do you remember
>>> what the issue was?
>> I said that already---preallocated metadata.  Also, at the time
>> pre-qcow2v3 was more important.
> 
> Yes, but Fam said that with preallocated metadata the clusters should be zero, or was that
> not true before qcow2v3?

Zero clusters didn't exist before qcow2v3 I think.

>> Are you using libiscsi, block devices or files?
> 
> Its a mixture. raw with libiscsi or lvm and qcow2 and vmdk either with libnfs or on local storage.
> 
> I stumbled across the issue with lseek on a tmpfs because in the build process for our templates
> I temporarily have vmdks on a tmpfs and it takes ages before qemu-img convert starts to run (it iterates
> over every 64kb cluster with that callout to find_allocation and for some reason lseek is very slow on tmpfs).

Ok, thanks.  Perhaps it's worth benchmarking tmpfs specifically.  Apart
from the system call overhead (which does not really matter if you're
going to do a read), lseek on other filesystems should not be any slower
than read.

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 17:56 schrieb Paolo Bonzini:
>
> On 20/03/2017 17:43, Peter Lieven wrote:
>> Am 20.03.2017 um 15:05 schrieb Paolo Bonzini:
>>> On 20/03/2017 14:35, Peter Lieven wrote:
>>>> Am 20.03.2017 um 14:23 schrieb Paolo Bonzini:
>>>>> On 20/03/2017 14:13, Peter Lieven wrote:
>>>>>> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>>>>>>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>>>>>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>>>>>>
>>>>>>>      block: look for zero blocks in bs->file
>>>>>>>     
>>>>>>>      Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>>      Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>>>
>>>>>>>
>>>>>>> It was introduced while introducing bdv_get_block_status. I don't know what the real
>>>>>>>
>>>>>>> issue was that was addressed with this patch?
>>>>>> Is it possible that this optimization was added especially for RAW? I was believing that
>>>>>> raw would forward the get_block_status call to bs->file, but it looks it doesn't.
>>>>>> If this one here was for RAW would it be an option to move this callout to the raw-format driver
>>>>>> and remove it from the generic code?
>>>>> It was meant for both raw and qcow2.
>>>> Okay, but as Fam mentioned qcow2 Metadata should know that a cluster is zero. Do you remember
>>>> what the issue was?
>>> I said that already---preallocated metadata.  Also, at the time
>>> pre-qcow2v3 was more important.
>> Yes, but Fam said that with preallocated metadata the clusters should be zero, or was that
>> not true before qcow2v3?
> Zero clusters didn't exist before qcow2v3 I think.
>
>>> Are you using libiscsi, block devices or files?
>> Its a mixture. raw with libiscsi or lvm and qcow2 and vmdk either with libnfs or on local storage.
>>
>> I stumbled across the issue with lseek on a tmpfs because in the build process for our templates
>> I temporarily have vmdks on a tmpfs and it takes ages before qemu-img convert starts to run (it iterates
>> over every 64kb cluster with that callout to find_allocation and for some reason lseek is very slow on tmpfs).
> Ok, thanks.  Perhaps it's worth benchmarking tmpfs specifically.  Apart
> from the system call overhead (which does not really matter if you're
> going to do a read), lseek on other filesystems should not be any slower
> than read.

Okay, but the even the read is not really necessary if the metadata is correct?
Would it be an idea to introduce an inverse flag live BDRV_BLOCK_NOT_ZERO for
cases where we know that there is really DATA and thus can avoid the second callout?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 27/03/2017 15:21, Peter Lieven wrote:
>>>
>>> I stumbled across the issue with lseek on a tmpfs because in the
>>> build process for our templates
>>> I temporarily have vmdks on a tmpfs and it takes ages before qemu-img
>>> convert starts to run (it iterates
>>> over every 64kb cluster with that callout to find_allocation and for
>>> some reason lseek is very slow on tmpfs).
>> Ok, thanks.  Perhaps it's worth benchmarking tmpfs specifically.  Apart
>> from the system call overhead (which does not really matter if you're
>> going to do a read), lseek on other filesystems should not be any slower
>> than read.
> 
> Okay, but the even the read is not really necessary if the metadata is
> correct?

Yeah, what I mean is:

- if you're going to do a read of non-zero blocks, the lseek you do
before reading those blocks should not matter.

- if you're going to skip the read of non-zero blocks, the lseek you do
is always going to be faster than reading them and then checking with
buffer_is_nonzero.

> Would it be an idea to introduce an inverse flag live BDRV_BLOCK_NOT_ZERO
> for cases where we know that there is really DATA and thus can avoid the
> second callout?

How would you know that a block is nonzero?

Paolo

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 27.03.2017 um 17:06 schrieb Paolo Bonzini:
>
> On 27/03/2017 15:21, Peter Lieven wrote:
>>>> I stumbled across the issue with lseek on a tmpfs because in the
>>>> build process for our templates
>>>> I temporarily have vmdks on a tmpfs and it takes ages before qemu-img
>>>> convert starts to run (it iterates
>>>> over every 64kb cluster with that callout to find_allocation and for
>>>> some reason lseek is very slow on tmpfs).
>>> Ok, thanks.  Perhaps it's worth benchmarking tmpfs specifically.  Apart
>>> from the system call overhead (which does not really matter if you're
>>> going to do a read), lseek on other filesystems should not be any slower
>>> than read.
>> Okay, but the even the read is not really necessary if the metadata is
>> correct?
> Yeah, what I mean is:
>
> - if you're going to do a read of non-zero blocks, the lseek you do
> before reading those blocks should not matter.
>
> - if you're going to skip the read of non-zero blocks, the lseek you do
> is always going to be faster than reading them and then checking with
> buffer_is_nonzero.
>
>> Would it be an idea to introduce an inverse flag live BDRV_BLOCK_NOT_ZERO
>> for cases where we know that there is really DATA and thus can avoid the
>> second callout?
> How would you know that a block is nonzero?

I would trust the metadata. At least for VMDK and QCOW2v3.
Bad idea?

Peter

Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Paolo Bonzini]

On 31/03/2017 09:55, Peter Lieven wrote:
>>> Would it be an idea to introduce an inverse flag live BDRV_BLOCK_NOT_ZERO
>>> for cases where we know that there is really DATA and thus can avoid the
>>> second callout?
>> How would you know that a block is nonzero?
> I would trust the metadata. At least for VMDK and QCOW2v3.
> Bad idea?

The metadata only tells you that a block is zero, not that it's nonzero.
 What you are suggesting is really the same as removing the recursion.
However, I still haven't understood clearly if it's a QEMU or tmpfs bug;
if it's a tmpfs bug your suggestion would not fix tmpfs slowness on raw
images.

Paolo