Re: [PATCH 2/2] file-posix: Cache next hole

[Max Reitz <mreitz@redhat.com>]

On 12.02.21 11:25, Kevin Wolf wrote:
> Am 12.02.2021 um 10:14 hat Max Reitz geschrieben:
>> On 11.02.21 21:38, Vladimir Sementsov-Ogievskiy wrote:
>>> 11.02.2021 20:22, Max Reitz wrote:
>>>> We have repeatedly received reports that SEEK_HOLE and SEEK_DATA are
>>>> slow on certain filesystems and/or under certain circumstances.  That is
>>>> why we generally try to avoid it (which is why bdrv_co_block_status()
>>>> has the @want_zero parameter, and which is why qcow2 has a metadata
>>>> preallocation detection, so we do not fall through to the protocol layer
>>>> to discover which blocks are zero, unless that is really necessary
>>>> (i.e., for metadata-preallocated images)).
>>>>
>>>> In addition to those measures, we can also try to speed up zero
>>>> detection by letting file-posix cache some hole location information,
>>>> namely where the next hole after the most recently queried offset is.
>>>> This helps especially for images that are (nearly) fully allocated,
>>>> which is coincidentally also the case where querying for zero
>>>> information cannot gain us much.
>>>>
>>>> Note that this of course only works so long as we have no concurrent
>>>> writers to the image, which is the case when the WRITE capability is not
>>>> shared.
>>>>
>>>> Alternatively (or perhaps as an improvement in the future), we could let
>>>> file-posix keep track of what it knows is zero and what it knows is
>>>> non-zero with bitmaps, which would help images that actually have a
>>>> significant number of holes (where this implementation here cannot do
>>>> much).  But for such images, SEEK_HOLE/DATA are generally faster (they
>>>> do not need to seek through the whole file), and the performance lost by
>>>> querying the block status does not feel as bad because it is outweighed
>>>> by the performance that can be saved by special-cases zeroed areas, so
>>>> focussing on images that are (nearly) fully allocated is more important.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> I'll look at it tomorrow... Just wanted to note that something similar
>>> was proposed by Kevin some time ago:
>>>
>>> <20190124141731.21509-1-kwolf@redhat.com>
>>> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06271.html
>>
>> Interesting.  The reasoning that it doesn’t matter whether anyone
>> writes to the assumed-data regions makes sense.
>>
>> I can’t see a real reason why it was kind of forgotten, apparently...
> 
> After qcow2 stopped recursively querying the file-posix layer, the
> relevant case under discussion was fixed anyway, so it didn't have the
> highest priority any more...
> 
> I think the open question (and possibly work) in the old thread was
> whether this should be moved out of file-posix into the generic block
> layer.
> 
> With your patch, I guess the other open question is whether we want to
> try and cache holes anyway. I assume that in the common case, you may
> have many consecutive data extents, but probably rarely many holes (I
> guess you can have more than one if some areas are unallocated and
> others are allocated, but unwritten?) Then it's probably not worth
> caching holes.

Yes, that’s what I think.  If you have many small holes, caching won’t 
help much anyway (at least this way, where we’d only cache information 
about a single hole).  So caching holes would mainly help when you have 
several large holes.  But in that case, finding them will surely speed 
up whatever it is the block-status caller is doing (e.g. mirroring), so 
a slow SEEK_HOLE/DATA will be tolerable.

Max