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

[NeilBrown]

On Thu, 01 Oct 2026, Amir Goldstein wrote:
> 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.

NFS can certainly support O_CREATE|O_DIRECTORY.  The MKDIR request
creates a directory and returns the file-handle of the directory that
was created.  I think other network filesystems return the identity of
the created thing - or fail if it already existed.  That is enough for
full support.

The only cases where I think I think there is any doubt of support is
kernfs and tracefs because they don't return the inode.  tracefs is
interesting because it drops and retakes the parent lock so it isn't
immediately clear what atomicity is available, though it doesn't support
rename at all so maybe there is no interesting race.

> 
> This gating criteria is not dependent on cache state,
> which is what we wanted.
> 
> 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).

The idea of using ->d_revalidate as a gate is certainly interesting.
Apart from the interaction with case-insensitivity, if we initially only
supported filesystems that don't have ->d_revalidate, I think we would get
coverage for enough filesystems to be interesting.  We could then take a
bit more time to think through the rest of the picture.

I don't think O_EXCL is at all problematic.  ->mkdir() is already
required to return -EEXIST if the directory already exists.

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

Presumably the fs would provide two dentry_operations structures and
choose which to pass to set_default_d_op() when creating the superblock.
Doing that would even provide slightly better performance in the
case where no d_revalidate is needed.

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
>