Re: [PATCH 5.15] fuse: Fix race condition in writethrough path A race

[Bernd Schubert <bernd@bsbernd.com>]

On 10/13/25 22:27, Joanne Koong wrote:
> On Mon, Oct 13, 2025 at 1:16 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>
>> On 10/13/25 15:39, Miklos Szeredi wrote:
>>> On Fri, 10 Oct 2025 at 10:46, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>
>>>> My idea is to introduce FUSE_I_MTIME_UNSTABLE (which would work
>>>> similarly to FUSE_I_SIZE_UNSTABLE) and when fetching old_mtime, verify
>>>> that it hasn't been invalidated.  If old_mtime is invalid or if
>>>> FUSE_I_MTIME_UNSTABLE signals that a write is in progress, the page
>>>> cache is not invalidated.
>>>
>>> [Adding Brian Foster, the author of FUSE_AUTO_INVAL_DATA patches.
>>> Link to complete thread:
>>> https://lore.kernel.org/all/20251009110623.3115511-1-giveme.gulu@gmail.com/#r]
>>>
>>> In summary: auto_inval_data invalidates data cache even if the
>>> modification was done in a cache consistent manner (i.e. write
>>> through). This is not generally a consistency problem, because the
>>> backing file and the cache should be in sync.  The exception is when
>>> the writeback to the backing file hasn't yet finished and a getattr()
>>> call triggers invalidation (mtime change could be from a previous
>>> write), and the not yet written data is invalidated and replaced with
>>> stale data.
>>>
>>> The proposed fix was to exclude concurrent reads and writes to the same region.
>>>
>>> But the real issue here is that mtime changes triggered by this client
>>> should not cause data to be invalidated.  It's not only racy, but it's
>>> fundamentally wrong.  Unfortunately this is hard to do this correctly.
>>> Best I can come up with is that any request that expects mtime to be
>>> modified returns the mtime after the request has completed.
>>>
>>> This would be much easier to implement in the fuse server: perform the
>>> "file changed remotely" check when serving a FUSE_GETATTR request and
>>> return a flag indicating whether the data needs to be invalidated or
>>> not.
>>
>> For an intelligent server maybe, but let's say one uses
>> <libfuse>/example/passthrough*, in combination with some external writes
>> to the underlying file system outside of fuse. How would passthrough*
>> know about external changes?
>>
>> The part I don't understand yet is why invalidate_inode_pages2() causes
>> an issue - it has folio_wait_writeback()?
>>
> 
> This issue is for the writethrough path which doesn't use writeback.


Oh right. So we need some kind of fuse_invalidate_pages(), that would
wait for for all current fuse_send_write_pages() to complete? Is that
what you meant with 'fi->writectr bias'?

Thanks,
Bernd