On 9/21/23 10:29, Amir Goldstein wrote:
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.
Yeah sorry, I had seen the warning, but ignored it, in favor of code
consistency.
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?
Hmm, not exactly. Patches 3 is a dependency for patch 5-7. So far
atomic open didn't handle positive dentries / revalidate.
Patch 3 adds support for that, the file system then needs to
signal in its ->d_revalidate return code that it wants
atomic_open to do the revalidation (which adds a bit complexity to
the atomic_open method). Patch 3 has then two cases,
O_CREATE and plain open without O_CREATE. Patch 4 is an
optimization for patch 3, for plain open. I actually start
to wonder if I shouldn't remove plain open from patch 3
and to add that in patch 4. Probably easier to read. Thanks for
making me think about that :)
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.
Only the new patch 8 is independent, I'm going to send that out
separately today.
Thanks,
Bernd
