Re: [Qemu-devel] [RFC 2/5] block: Generic truncation fallback

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3620 bytes --]

On 12.07.19 13:17, Kevin Wolf wrote:
> Am 12.07.2019 um 12:58 hat Max Reitz geschrieben:
>> On 12.07.19 11:49, Kevin Wolf wrote:
>>> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>>>> If a protocol driver does not support truncation, we call fall back to
>>>> effectively not doing anything if the new size is less than the actual
>>>> file size.  This is what we have been doing for some host device drivers
>>>> already.
>>>
>>> Specifically, we're doing it for drivers that access a fixed-size image,
>>> i.e. block devices rather than regular files. We don't want to do this
>>> for drivers where the file size could be changed, but just didn't
>>> implement it.
>>>
>>> So I would suggest calling the function more specifically something like
>>> bdrv_co_truncate_blockdev(), and not using it as an automatic fallback
>>> in bdrv_co_truncate(), but just make it the BlockDriver.bdrv_co_truncate
>>> implementation for those drivers where it makes sense.
>>
>> I was thinking about this, but the problem is that .bdrv_co_truncate()
>> does not get a BdrvChild, so an implementation for it cannot generically
>> zero the first sector (without bypassing the permission system, which
>> would be wrong).
> 
> Hm, I see. The next best thing would then probably having a bool in
> BlockDriver that enables the fallback.
> 
>> So the function pointer would actually need to be set to something like
>> (int (*)(BlockDriverState *, int64_t, PreallocMode, Error **))42ul, or a
>> dummy function that just aborts, and then bdrv_co_truncate() would
>> recognize this magic constant.  But that seemed so weird to me that I
>> decided just not to do it, mostly because I was wondering what would be
>> so bad about treating images whose size we cannot change because we
>> haven’t implemented it exactly like fixed-size images.
>>
>> (Also, “fixed-size” is up to interpretation.  You can change an LVM
>> volume’s size.  qemu doesn’t do it, obviously.  But that is the reason
>> for the warning qemu-img resize emits when it sees that the file size
>> did not change.)
>>
>>> And of course, we only need these fake implementations because qemu-img
>>> (or .bdrv_co_create_opts) always wants to create the protocol level. If
>>> we could avoid this, then we wouldn't need any of this.
>>
>> It’s trivial to avoid this.  I mean, patch 4 does exactly that.
>>
>> So it isn’t about avoiding creating the protocol level, it’s about
>> avoiding the truncation there.  But why would you do that?
> 
> Because we can't actually truncate there. We can only do the fake thing
> and claim we have truncated even though the size didn't change.

You’re right.  I actually didn’t realize that we have no drivers that
support truncation, but not image creation.

Yes, then it’s stupid.

I thought it was a reasonable thing to do for such drivers.

So I suppose the best thing is to do what I hinted at in my reply to
your reply to patch 3, which is to drop patches 2 and 3 and instead make
the creation fallback work around blk_truncate() failures.

Max

> But actually, while avoiding the fake truncate outside of image creation
> would avoid confusing situations where the user requested image
> shrinking, gets success, but nothing happened, it would also break
> actual resizes where the volume is resized externally and block_resize
> is only called to notify qemu to pick up the size change.
> 
> So after all, we probably do need to keep the hack in bdrv_co_truncate()
> instead of moving it to image creation only.
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]