| Message ID | 20190725155512.9827-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Make various formats' block_status recurse again | expand |
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); >
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..
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
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);
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(-)