On Thu, Sep 21, 2023 at 11:09 AM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 9/21/23 08:16, Amir Goldstein wrote:
> > On Thu, Sep 21, 2023 at 12:48 AM Bernd Schubert <bschubert@xxxxxxx> wrote:
> >>
> >> This was suggested by Miklos based on review from the previous
> >> patch that introduced atomic open for positive dentries.
> >> In open_last_lookups() the dentry was not used anymore when atomic
> >> revalidate was available, which required to release the dentry,
> >> then code fall through to lookup_open was done, which resulted
> >> in another d_lookup and also d_revalidate. All of that can
> >> be avoided by the new atomic_revalidate_open() function.
> >>
> >> Another included change is the introduction of an enum as
> >> d_revalidate return code.
> >>
> >> Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
> >> Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> >> Cc: Dharmendra Singh <dsingh@xxxxxxx>
> >> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> >> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> >> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> >> ---
> >> fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 43 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/namei.c b/fs/namei.c
> >> index f01b278ac0ef..8ad7a0dfa596 100644
> >> --- a/fs/namei.c
> >> +++ b/fs/namei.c
> >> @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> >> return dentry;
> >> }
> >>
> >> +static struct dentry *atomic_revalidate_open(struct dentry *dentry,
> >> + struct nameidata *nd,
> >> + struct file *file,
> >> + const struct open_flags *op,
> >> + bool *got_write)
> >> +{
> >> + struct mnt_idmap *idmap;
> >> + struct dentry *dir = nd->path.dentry;
> >> + struct inode *dir_inode = dir->d_inode;
> >> + int open_flag = op->open_flag;
> >> + umode_t mode = op->mode;
> >> +
> >> + if (unlikely(IS_DEADDIR(dir_inode)))
> >> + return ERR_PTR(-ENOENT);
> >> +
> >> + file->f_mode &= ~FMODE_CREATED;
> >> +
> >> + if (unlikely(open_flag & O_CREAT)) {
> >> + WARN_ON(1);
> >
> > if (WARN_ON_ONCE(open_flag & O_CREAT)) {
> >
> > is more compact and produces a nicer print
>
> Thanks a lot for your review Amir! Nice, I hadn't noticed that
> these macros actually return a value. Also updated the related
> fuse patch, which had a similar construct.
>
> >
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR))
> >> + *got_write = !mnt_want_write(nd->path.mnt);
> >> + else
> >> + *got_write = false;
> >> +
> >> + if (!got_write)
> >> + open_flag &= ~O_TRUNC;
> >> +
> >> + inode_lock_shared(dir->d_inode);
> >> + dentry = atomic_open(nd, dentry, file, open_flag, mode);
> >> + inode_unlock_shared(dir->d_inode);
> >> +
> >> + return dentry;
> >> +
> >> +}
> >> +
> >> /*
> >> * Look up and maybe create and open the last component.
> >> *
> >> @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd,
> >> if (IS_ERR(dentry))
> >> return ERR_CAST(dentry);
> >> if (dentry && unlikely(atomic_revalidate)) {
> >> - dput(dentry);
> >> - dentry = NULL;
> >> + BUG_ON(nd->flags & LOOKUP_RCU);
> >
> > The assertion means that someone wrote bad code
> > it does not mean that the kernel internal state is hopelessly corrupted.
> > Please avoid BUG_ON and use WARN_ON_ONCE and if possible
> > (seems to be the case here) also take the graceful error path.
> > It's better to fail an open than to crash the kernel.
>
> Thanks, updated. I had used BUG_ON because there is a similar BUG_ON a
> few lines below.
checkpatch strictly forbids new BUG_ON:
"Do not crash the kernel unless it is absolutely unavoidable-- use
WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants"
But it's true that vfs code has several of those.
> Added another RFC patch to covert that one as well.
> I'm going to wait a few days for possible other reviews and will then send
> out the new version. The updated v10 branch is here
>
> https://github.com/bsbernd/linux/tree/atomic-open-for-6.5-v10
>
IIUC, patches 3,4 are independent vfs optimization.
Is that correct?
Since you are going to need review of vfs maintainers
and since Christian would probably want to merge them
via the vfs tree, I think it would be better for you to post
them separately from your series if possible, so Miklos
would be able to pick up the fuse patches when they are
reviewed.
Thanks,
Amir.
