Message-ID: <aO1Klyk0OWx_UFpz@bfoster>
Date: Mon, 13 Oct 2025 14:53:11 -0400
From: Brian Foster <bfoster@...hat.com>
To: Miklos Szeredi <miklos@...redi.hu>
Cc: lu gu <giveme.gulu@...il.com>, Joanne Koong <joannelkoong@...il.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Bernd Schubert <bernd@...ernd.com>
Subject: Re: [PATCH 5.15] fuse: Fix race condition in writethrough path A race

On Mon, Oct 13, 2025 at 08:23:39PM +0200, Miklos Szeredi wrote:
> On Mon, 13 Oct 2025 at 19:40, Brian Foster <bfoster@...hat.com> wrote:
> 
> > If I follow the report correctly, we're basically producing an internal
> > inconsistency between mtime and cache state that falsely presents as a
> > remote change, so one of these attr change checks can race with a write
> > in progress and invalidate cache. Do I have that right?
> 
> Yes.
> 
> >
> > But still a few questions..
> >
> > 1. Do we know where exactly the mtime update comes from? Is it the write
> > in progress that updates the file mtime on the backend and creates the
> > inconsistency?
> 
> It can be a previous write.  A write will set STATX_MTIME in
> fi->inval_mask, indicating that the value cached in i_mtime is
> invalid.  But the auto_inval code will ignore that and use  cached
> mtime to compare against the new value.
> 
> We could skip data invalidation if the cached value of mtime is not
> valid, but this could easily result in remote changes being missed.
> 

Hrm Ok. But even if we did miss remote changes, whose to say we can even
resolve that correctly from the kernel anyways..? Like if there happens
to be dirty data in cache and a remote change at the same time, that
kind of sounds like a policy decision for userspace. Maybe the fuse
position should be something like "expose mechanisms to manage this,
otherwise we'll just pick a side." Or "we'll never toss dirty data
unless explicitly asked by userspace."

> >
> > 2. Is it confirmed that auto_inval is the culprit here? It seems logical
> > to me, but it can also be disabled dynamically so couldn't hurt to
> > confirm that if there's a reproducer.
> 
> Yes, reproducer has auto_inval_data turned on (libfuse turns it on by default).
> 

I was more wondering if the problem goes away if it were disabled..

> >
> > 3. I don't think we should be able to invalidate "dirty" folios like
> > this. On a quick look though, it seems we don't mark folios dirty in
> > this write path. Is that right?
> 
> Correct.
> 
> >
> > If so, I'm a little curious if that's more of a "no apparent need" thing
> > since the writeback occurs right in that path vs. that is an actual
> > wrong thing to do for some reason. Hm?
> 
> Good question.  I think it's wrong, since dirtying the pages would
> allow the witeback code to pick them up, which would be messy.
> 

Ah, yeah that makes sense. Though invalidate waits on writeback. Any
reason this path couldn't skip the dirty state but mark the pages as
under writeback across the op?

> > Agreed in general. IIUC, this is ultimately a heuristic that isn't
> > guaranteed to necessarily get things right for the backing fs. ISTM that
> > maybe fuse is trying too hard to handle the distributed case correctly
> > where the backing fs should be the one to implement this sort of thing
> > through exposed mechanisms. OTOH so long as the heuristic exists we
> > should probably at least work to make it internally consistent.
> 
> Yes, that's my problem.  How can we fix this without adding too much
> complexity and without breaking existing uses?
> 

I probably need to stare at the code some more. Sorry, it's been quite a
while since I've looked at this. Curious.. was there something wrong
with your unstable mtime idea, or just that it might not be fully
generic enough?

This might be a good question for any fuse based distributed fs
projects. I'm not sure if/how active glusterfs is these days..

Brian

> Thanks,
> Miklos
> 

Powered by blists - more mailing lists