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

[Amir Goldstein]

On Thu, Oct 1, 2026 at 12:08 PM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> 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.

Yes, either that or clear sb->s_d_flags & DCACHE_OP_REVALIDATE
when d_revalidate is not going to be used.

> Doing that would even provide slightly better performance in the
> case where no d_revalidate is needed.
>

Very slightly, so this should not be a reason, but advertising corrects
contracts to vfs.

For example, if overlayfs would want to install:

static const struct dentry_operations ovl_dentry_real_operations = {
        .d_real = ovl_d_real,
};

It needs to know that none of the underlying dentries is expected to have
a d_revalidate op at super fill time.

Thanks,
Amir.