Re: [PATCH v6 07/12] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[Jori Koolstra]

> Op 01-10-2026 11:29 CEST schreef Amir Goldstein <amir73il@xxxxxxxxx>:
> 
>  
> On Thu, Oct 1, 2026 at 12:16 AM Jori Koolstra <jkoolstra@xxxxxxxxx> wrote:
> >
> >
> > > Op 30-09-2026 05:45 EDT schreef Amir Goldstein <amir73il@xxxxxxxxx>:
> > >
> > >
> > > On Wed, Sep 30, 2026 at 12:15 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
> > > >
> ...
> > > > The problem with this approach is that open(.., O_CREAT|O_DIRECTORY)
> > > > might create the directory, then return -EOPNOTSUPP.  This is weird and
> > > > I'd rather it not be visible.
> > > >
> > > > Currently O_DIRECTORY|O_CREAT results in -EINVAL.  I would rather it
> > > > remain a -EINVAL on any filesystem which doesn't completely support
> > > > the functionality.
> > > >
> > >
> > > Joining late to this party so apologies in advance if my questions
> > > have already been addressed.
> > >
> > > I agree with Neil's statement above, but IMO, the atomic_open() fs
> > > match the description of "doesn't completely support the functionality."
> > > Therefore, I think that rather than success if directory exists, they
> > > should also return -EINVAL/-EOPNOTSUPP consistently (see below).
> > >
> >
> > The issue with this is that if you want per fs atomic_open() opt-in (in
> > contrast to either implementing all instances in one release or disabling
> > all), you have the issue that your lookup now depends on the caching status
> > of the directory dentry.
> >
> > If that dentry is in cache, and positive, d_lookup() earlier in lookup_open()
> > makes it return early:
> >
> >         if (dentry->d_inode) {
> >                 /* Cached positive dentry: will open in do_open(). */
> >                 goto out;
> >         }
> >
> > So you get your lookup. But if the same dentry is not in cache, now you
> > suddenly get -EINVAL. I thought that behavior was more unwanted then
> > what I eventually settled on, namely to strip the O_CREAT bit.
> >
> 
> Maybe I am missing something, but I think you misunderstand me.
> What I mean is - if directory inode has a ->atomic_open() op,
> bail early with -EINVAL/-EOPNOTSUPP, because this is a network
> filesystem that does not support atomic O_CREATE|O_DIRECTORY
> and in most likelihood never will support it.
> 
> This gating criteria is not dependent on cache state,
> which is what we wanted.
> 

No in that case I think I've understood you (or maybe still not?) My
point is that we can't do that if we want to be able to individually
support O_CREAT|O_DIRECTORY for some ->atomic_open fs. If we bail early
the how can say only NFS support it at some point? You get into the
situation where everybody needs to have support or no one.

Does that make sense, or do I still misunderstand you point?

> The justification of using ->d_revalidate() as another opt-out
> is that existence of ->d_revalidate() means that the state known
> to dcache is only semi-reliable, so making atomic create/open
> promises is problematic (O_EXCL for example).
> 

I must admit I don't know too much about what the revalidate step is for.
What does semi-reliable mean here? That the dcache might have returned a
dentry that has timed out in some sense and does not reflect the actual
state of the fs?

But if we force O_EXCL like Neil suggested, is that a problem? You'd only
get an fd if you actually created the thing.

I should really start to learn and contribute to an actual fs that is used
instead of just staying at the VFS level. It would make understanding all
these subtleties easier...

> The problem is that some fs (overlayfs/ext4/f2fs) register
> a mostly-noop  ->d_revalidate() so I proposed how to deal with those.
> 
> Thanks,
> Amir.