Re: Re: Re: Re: [PATCH] fuse: fix inode initialization race

[Horst Birthelmer <horst@birthelmer.de>]

On Thu, Mar 26, 2026 at 11:00:54AM -0700, Joanne Koong wrote:
> On Thu, Mar 26, 2026 at 10:54 AM Horst Birthelmer <horst@birthelmer.de> wrote:
> >
> > On Thu, Mar 26, 2026 at 09:43:00AM -0700, Joanne Koong wrote:
> > > On Thu, Mar 26, 2026 at 8:48 AM Horst Birthelmer <horst@birthelmer.de> wrote:
> > > >
> > > > On Thu, Mar 26, 2026 at 04:19:24PM +0100, Miklos Szeredi wrote:
> > > > > On Thu, 26 Mar 2026 at 16:13, Bernd Schubert <bernd@bsbernd.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 3/26/26 15:26, Christian Brauner wrote:
> > > > > > > On Wed, Mar 25, 2026 at 08:54:57AM +0100, Bernd Schubert wrote:
> > > > > > >>
> > > > > > >>
> > > > > > >> On 3/18/26 14:43, Horst Birthelmer wrote:
> > > > > > >>> From: Horst Birthelmer <hbirthelmer@ddn.com>
> > > > >
> > > > > > >>>     fi->attr_version = atomic64_inc_return(&fc->attr_version);
> > > > > > >>> +   wake_up_all(&fc->attr_version_waitq);
> > > > > > >>>     fi->i_time = attr_valid;
> > > > > >
> > > > > >
> > > > > > While I'm looking at this again, wouldn't it make sense to make this
> > > > > > conditional? Because we wake this queue on every attr change for every
> > > > > > inode. And the conditional in fuse_iget() based on I_NEW?
> > > > >
> > > > > Right, should only wake if fi->attr_version old value was zero.
> > > > >
> > > > > BTW I have a hunch that there are better solutions, but it's simple
> > > > > enough as a stopgap measure.
> > > >
> > > > OK, I'll send a new version.
> > > >
> > > > Just out of curiosity, what would be a better solution?
> > >
> > > I'm probably missing something here but why can't we just call the
> > >
> > >         fi = get_fuse_inode(inode);
> > >         spin_lock(&fi->lock);
> > >         fi->nlookup++;
> > >         spin_unlock(&fi->lock);
> > >         fuse_change_attributes_i(inode, attr, NULL, attr_valid, attr_version,
> > >                                  evict_ctr);
> > >
> > > logic before releasing the inode lock (the unlock_new_inode() call) in
> > > fuse_iget() to address the race? unlock_new_inode() clears I_NEW so
> > > fuse_reverse_inval_inode()'s fuse_ilookup() would only get the inode
> > > after the attributes initialization has finished.
> > >
> > > As I understand it, fuse_change_attributes_i() would be pretty
> > > straightforward / fast for I_NEW inodes, as it doesn't send any
> > > synchronous requests and for the I_NEW case the
> > > invalidate_inode_pages2() and truncate_pagecache() calls would get
> > > skipped. (truncate_pagecache() getting skipped because inode->i_size
> > > is already attr->size from fuse_init_inode(), so "oldsize !=
> > > attr->size" is never true; and invalidate_inode_pages2() getting
> > > skipped because "oldsize != attr->size" is never true and "if
> > > (!timespec64_equal(&old_mtime, &new_mtime))" is never true because
> > > fuse_init_inode() initialized the inode's mtime to attr->mtime).
> >
> > You understand the pretty well, I think.
> > The problem I have there is that fuse_change_attributes_i() takes
> > its own lock.
> > That would be a pretty big operation to split that function.
> 
> I believe fuse_change_attribtues_i() takes the fi lock, not the inode
> lock, so this should be fine.
> 

Yes, I got confused there, sorry.
Still, a pretty big change for a corner case. Don't you think?

What would be the advantage to the current situation with the requested 
changes from Miklos and Bernd, of course? So that we only do the 
wakeup_all() call only if we have initialized a new inode?

> Thanks,
> Joanne
> >
> > Is that required for this small (as Miklos put it, rare) case?
> >
> > If you guys want me to do that, I will.
> >
> > Thanks,
> > Horst