vpc is not really a passthrough driver, even when using the fixed
subformat (where host and guest offsets are equal).  It should handle
preallocation like all other drivers do, namely by returning
DATA | RECURSE instead of RAW.

There is no tangible difference but the fact that bdrv_is_allocated() no
longer falls through to the protocol layer.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index d4776ee8a5..b25aab0425 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -737,7 +737,7 @@ static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
         *pnum = bytes;
         *map = offset;
         *file = bs->file->bs;
-        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
+        return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_RECURSE;
     }
 
     qemu_co_mutex_lock(&s->lock);
-- 
2.21.0

25.07.2019 18:55, Max Reitz wrote:
> vpc is not really a passthrough driver, even when using the fixed
> subformat (where host and guest offsets are equal).  It should handle
> preallocation like all other drivers do, namely by returning
> DATA | RECURSE instead of RAW.
> 
> There is no tangible difference but the fact that bdrv_is_allocated() no
> longer falls through to the protocol layer.

Hmm. Isn't a real bug (fixed by this patch) ?

Assume vpc->file is qcow2 with backing, which have "unallocated" region, which is
backed by actual data in backing file.

So, this region will be reported as not allocated and will be skipped by any copying
loop using block-status? Is it a bug of BDRV_BLOCK_RAW itself? Or I don't understand
something..

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/vpc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index d4776ee8a5..b25aab0425 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -737,7 +737,7 @@ static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
>           *pnum = bytes;
>           *map = offset;
>           *file = bs->file->bs;
> -        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
> +        return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_RECURSE;
>       }
>   
>       qemu_co_mutex_lock(&s->lock);
> 


-- 
Best regards,
Vladimir

On 12.08.19 17:33, Vladimir Sementsov-Ogievskiy wrote:
> 25.07.2019 18:55, Max Reitz wrote:
>> vpc is not really a passthrough driver, even when using the fixed
>> subformat (where host and guest offsets are equal).  It should handle
>> preallocation like all other drivers do, namely by returning
>> DATA | RECURSE instead of RAW.
>>
>> There is no tangible difference but the fact that bdrv_is_allocated() no
>> longer falls through to the protocol layer.
> 
> Hmm. Isn't a real bug (fixed by this patch) ?
> 
> Assume vpc->file is qcow2 with backing, which have "unallocated" region, which is
> backed by actual data in backing file.

Come on now.

> So, this region will be reported as not allocated and will be skipped by any copying
> loop using block-status? Is it a bug of BDRV_BLOCK_RAW itself? Or I don't understand
> something..

I think what you don’t understand is that if you have a vpc file inside
of a qcow2 file, you’re doing basically everything wrong. ;-)

But maybe we should drop BDRV_BLOCK_RAW...  Does it do anything good for
us in the raw driver?  Shouldn’t it too just return DATA | RECURSE?

Max

12.08.2019 18:56, Max Reitz wrote:
> On 12.08.19 17:33, Vladimir Sementsov-Ogievskiy wrote:
>> 25.07.2019 18:55, Max Reitz wrote:
>>> vpc is not really a passthrough driver, even when using the fixed
>>> subformat (where host and guest offsets are equal).  It should handle
>>> preallocation like all other drivers do, namely by returning
>>> DATA | RECURSE instead of RAW.
>>>
>>> There is no tangible difference but the fact that bdrv_is_allocated() no
>>> longer falls through to the protocol layer.
>>
>> Hmm. Isn't a real bug (fixed by this patch) ?
>>
>> Assume vpc->file is qcow2 with backing, which have "unallocated" region, which is
>> backed by actual data in backing file.
> 
> Come on now.
> 
>> So, this region will be reported as not allocated and will be skipped by any copying
>> loop using block-status? Is it a bug of BDRV_BLOCK_RAW itself? Or I don't understand
>> something..
> 
> I think what you don’t understand is that if you have a vpc file inside
> of a qcow2 file, you’re doing basically everything wrong. ;-)
> 
> But maybe we should drop BDRV_BLOCK_RAW...  Does it do anything good for
> us in the raw driver?  Shouldn’t it too just return DATA | RECURSE?
> 

And if I have raw driver above qcow2, it will not work, like I've described above..


-- 
Best regards,
Vladimir

On 12.08.19 18:50, Vladimir Sementsov-Ogievskiy wrote:
> 12.08.2019 18:56, Max Reitz wrote:
>> On 12.08.19 17:33, Vladimir Sementsov-Ogievskiy wrote:
>>> 25.07.2019 18:55, Max Reitz wrote:
>>>> vpc is not really a passthrough driver, even when using the fixed
>>>> subformat (where host and guest offsets are equal).  It should handle
>>>> preallocation like all other drivers do, namely by returning
>>>> DATA | RECURSE instead of RAW.
>>>>
>>>> There is no tangible difference but the fact that bdrv_is_allocated() no
>>>> longer falls through to the protocol layer.
>>>
>>> Hmm. Isn't a real bug (fixed by this patch) ?
>>>
>>> Assume vpc->file is qcow2 with backing, which have "unallocated" region, which is
>>> backed by actual data in backing file.
>>
>> Come on now.
>>
>>> So, this region will be reported as not allocated and will be skipped by any copying
>>> loop using block-status? Is it a bug of BDRV_BLOCK_RAW itself? Or I don't understand
>>> something..
>>
>> I think what you don’t understand is that if you have a vpc file inside
>> of a qcow2 file, you’re doing basically everything wrong. ;-)
>>
>> But maybe we should drop BDRV_BLOCK_RAW...  Does it do anything good for
>> us in the raw driver?  Shouldn’t it too just return DATA | RECURSE?
>>
> 
> And if I have raw driver above qcow2, it will not work, like I've described above..

Yep.  That’s why I was wondering.  (This is a more likely case, because
maybe you really want to use raw’s offset capability on top of qcow2.)

Max

Am 12.08.2019 um 17:56 hat Max Reitz geschrieben:
> On 12.08.19 17:33, Vladimir Sementsov-Ogievskiy wrote:
> > 25.07.2019 18:55, Max Reitz wrote:
> >> vpc is not really a passthrough driver, even when using the fixed
> >> subformat (where host and guest offsets are equal).  It should handle
> >> preallocation like all other drivers do, namely by returning
> >> DATA | RECURSE instead of RAW.
> >>
> >> There is no tangible difference but the fact that bdrv_is_allocated() no
> >> longer falls through to the protocol layer.
> > 
> > Hmm. Isn't a real bug (fixed by this patch) ?
> > 
> > Assume vpc->file is qcow2 with backing, which have "unallocated" region, which is
> > backed by actual data in backing file.
> 
> Come on now.
> 
> > So, this region will be reported as not allocated and will be skipped by any copying
> > loop using block-status? Is it a bug of BDRV_BLOCK_RAW itself? Or I don't understand
> > something..
> 
> I think what you don’t understand is that if you have a vpc file inside
> of a qcow2 file, you’re doing basically everything wrong. ;-)
> 
> But maybe we should drop BDRV_BLOCK_RAW...  Does it do anything good for
> us in the raw driver?  Shouldn’t it too just return DATA | RECURSE?

DATA | RECURSE is still DATA, i.e. marks the block as allocated. If you
do that unconditionally, we will never consider a block unallocated.
RECURSE doesn't undo this, the only thing it might do is settting ZERO
additionally.

So I would say unconditionally returning DATA | RECURSE is almost always
wrong.

Kevin

On 13.08.19 12:38, Kevin Wolf wrote:
> Am 12.08.2019 um 17:56 hat Max Reitz geschrieben:
>> On 12.08.19 17:33, Vladimir Sementsov-Ogievskiy wrote:
>>> 25.07.2019 18:55, Max Reitz wrote:
>>>> vpc is not really a passthrough driver, even when using the fixed
>>>> subformat (where host and guest offsets are equal).  It should handle
>>>> preallocation like all other drivers do, namely by returning
>>>> DATA | RECURSE instead of RAW.
>>>>
>>>> There is no tangible difference but the fact that bdrv_is_allocated() no
>>>> longer falls through to the protocol layer.
>>>
>>> Hmm. Isn't a real bug (fixed by this patch) ?
>>>
>>> Assume vpc->file is qcow2 with backing, which have "unallocated" region, which is
>>> backed by actual data in backing file.
>>
>> Come on now.
>>
>>> So, this region will be reported as not allocated and will be skipped by any copying
>>> loop using block-status? Is it a bug of BDRV_BLOCK_RAW itself? Or I don't understand
>>> something..
>>
>> I think what you don’t understand is that if you have a vpc file inside
>> of a qcow2 file, you’re doing basically everything wrong. ;-)
>>
>> But maybe we should drop BDRV_BLOCK_RAW...  Does it do anything good for
>> us in the raw driver?  Shouldn’t it too just return DATA | RECURSE?
> 
> DATA | RECURSE is still DATA, i.e. marks the block as allocated. If you
> do that unconditionally, we will never consider a block unallocated.

Which is correct for formats that do not have backing files.

> RECURSE doesn't undo this, the only thing it might do is settting ZERO
> additionally.
> 
> So I would say unconditionally returning DATA | RECURSE is almost always
> wrong.

I would say it’s always right when it is a format driver and there is no
backing file.

Max