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. 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 Thanks, Bernd
