Re: [PATCH v2 03/21] preallocate: Don't poll during permission updates

[Kevin Wolf]

Am 05.10.2023 um 21:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11.09.23 12:46, Kevin Wolf wrote:
> > When the permission related BlockDriver callbacks are called, we are in
> > the middle of an operation traversing the block graph. Polling in such a
> > place is a very bad idea because the graph could change in unexpected
> > ways. In the future, callers will also hold the graph lock, which is
> > likely to turn polling into a deadlock.
> > 
> > So we need to get rid of calls to functions like bdrv_getlength() or
> > bdrv_truncate() there as these functions poll internally. They are
> > currently used so that when no parent has write/resize permissions on
> > the image any more, the preallocate filter drops the extra preallocated
> > area in the image file and gives up write/resize permissions itself.
> > 
> > In order to achieve this without polling in .bdrv_check_perm, don't
> > immediately truncate the image, but only schedule a BH to do so. The
> > filter keeps the write/resize permissions a bit longer now until the BH
> > has executed.
> > 
> > There is one case in which delaying doesn't work: Reopening the image
> > read-only. In this case, bs->file will likely be reopened read-only,
> > too, so keeping write permissions a bit longer on it doesn't work. But
> > we can already cover this case in preallocate_reopen_prepare() and not
> > rely on the permission updates for it.
> 
> Hmm, now I found one more "future" case.
> 
> I now try to rebase my "[PATCH v7 0/7] blockdev-replace"
> https://patchew.org/QEMU/20230421114102.884457-1-vsementsov@yandex-team.ru/
> 
> And it breaks after this commit.
> 
> By accident, blockdev-replace series uses exactly "preallocate" filter
> to test insertion/removing of filters. And removing is broken now.
> 
> Removing is done as follows:
> 
> 1. We have filter inserted: disk0 -file-> filter -file-> file0
> 
> 2. blockdev-replace, replaces file child of disk0, so we should get the 
> picture*: disk0 -file-> file0 <-file- filter
> 
> 3. blockdev-del filter
> 
> 
> But step [2] fails, as now preallocate filter doesn't drop permissions
> during the operation (postponing this for a while) and the picture* is
> impossible. Permission check fails.
> 
> Hmmm... Any idea how blockdev-replace and preallocate filter should
> work :) ? Maybe, doing truncation in .drain_begin() will help? Will
> try

Hm... What preallocate tries to do is really tricky...

Of course, the error is correct, this is an invalid configuration if
preallocate can still resize the image. So it would have to truncate the
file earlier, but the first time that preallocate knows of the change is
already too late to run requests.

Truncating on drain_begin feels more like a hack, but as long as it does
the job... Of course, this will have the preallocation truncated away on
events that have nothing to do with removing the filter. It's not
necessarily a disaster because preallocation is only an optimisation,
but it doesn't feel great.

Maybe let's take a step back: Which scenario is the preallocate driver
meant for and why do we even need to truncate the image file after
removing the filter? I suppose the filter doesn't make sense with raw
images because these are fixed size anyway, and pretty much any other
image format should be able to tolerate a permanently rounded up file
size. As long as you don't write to the preallocated area, it shouldn't
take space either on any sane filesystem.

Hmm, actually both VHD and VMDK can have footers, better avoid it with
those... But if truncating the image file on close is critical, what do
you do on crashes? Maybe preallocate should just not be considered
compatible with these formats?

Kevin