lookup_one_qstr_excl() is used for lookups prior to directory
modifications, whether create, unlink, rename, or whatever.
To prepare for allowing modification to happen in parallel, change
lookup_one_qstr_excl() to use d_alloc_parallel().
To reflect this, name is changed to lookup_one_qtr() - as the directory
may be locked shared.
If any for the "intent" LOOKUP flags are passed, the caller must ensure
d_lookup_done() is called at an appropriate time. If none are passed
then we can be sure ->lookup() will do a real lookup and d_lookup_done()
is called internally.
Signed-off-by: NeilBrown <neilb@suse.de>
---
fs/namei.c | 47 +++++++++++++++++++++++++------------------
fs/smb/server/vfs.c | 7 ++++---
include/linux/namei.h | 9 ++++++---
3 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 5cdbd2eb4056..d684102d873d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1665,15 +1665,13 @@ static struct dentry *lookup_dcache(const struct qstr *name,
}
/*
- * Parent directory has inode locked exclusive. This is one
- * and only case when ->lookup() gets called on non in-lookup
- * dentries - as the matter of fact, this only gets called
- * when directory is guaranteed to have no in-lookup children
- * at all.
+ * Parent directory has inode locked: exclusive or shared.
+ * If @flags contains any LOOKUP_INTENT_FLAGS then d_lookup_done()
+ * must be called after the intended operation is performed - or aborted.
*/
-struct dentry *lookup_one_qstr_excl(const struct qstr *name,
- struct dentry *base,
- unsigned int flags)
+struct dentry *lookup_one_qstr(const struct qstr *name,
+ struct dentry *base,
+ unsigned int flags)
{
struct dentry *dentry = lookup_dcache(name, base, flags);
struct dentry *old;
@@ -1686,18 +1684,25 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
if (unlikely(IS_DEADDIR(dir)))
return ERR_PTR(-ENOENT);
- dentry = d_alloc(base, name);
- if (unlikely(!dentry))
+ dentry = d_alloc_parallel(base, name);
+ if (unlikely(IS_ERR_OR_NULL(dentry)))
return ERR_PTR(-ENOMEM);
+ if (!d_in_lookup(dentry))
+ /* Raced with another thread which did the lookup */
+ return dentry;
old = dir->i_op->lookup(dir, dentry, flags);
if (unlikely(old)) {
+ d_lookup_done(dentry);
dput(dentry);
dentry = old;
}
+ if ((flags & LOOKUP_INTENT_FLAGS) == 0)
+ /* ->lookup must have given final answer */
+ d_lookup_done(dentry);
return dentry;
}
-EXPORT_SYMBOL(lookup_one_qstr_excl);
+EXPORT_SYMBOL(lookup_one_qstr);
/**
* lookup_fast - do fast lockless (but racy) lookup of a dentry
@@ -2739,7 +2744,7 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
return ERR_PTR(-EINVAL);
}
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
- d = lookup_one_qstr_excl(&last, path->dentry, 0);
+ d = lookup_one_qstr(&last, path->dentry, 0);
if (IS_ERR(d)) {
inode_unlock(path->dentry->d_inode);
path_put(path);
@@ -4078,8 +4083,8 @@ static struct dentry *filename_create(int dfd, struct filename *name,
if (last.name[last.len] && !want_dir)
create_flags = 0;
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
- dentry = lookup_one_qstr_excl(&last, path->dentry,
- reval_flag | create_flags);
+ dentry = lookup_one_qstr(&last, path->dentry,
+ reval_flag | create_flags);
if (IS_ERR(dentry))
goto unlock;
@@ -4103,6 +4108,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
}
return dentry;
fail:
+ d_lookup_done(dentry);
dput(dentry);
dentry = ERR_PTR(error);
unlock:
@@ -4508,7 +4514,7 @@ int do_rmdir(int dfd, struct filename *name)
goto exit2;
inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
- dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
+ dentry = lookup_one_qstr(&last, path.dentry, lookup_flags);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto exit3;
@@ -4641,7 +4647,7 @@ int do_unlinkat(int dfd, struct filename *name)
goto exit2;
retry_deleg:
inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
- dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
+ dentry = lookup_one_qstr(&last, path.dentry, lookup_flags);
error = PTR_ERR(dentry);
if (!IS_ERR(dentry)) {
@@ -5231,8 +5237,8 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
goto exit_lock_rename;
}
- old_dentry = lookup_one_qstr_excl(&old_last, old_path.dentry,
- lookup_flags);
+ old_dentry = lookup_one_qstr(&old_last, old_path.dentry,
+ lookup_flags);
error = PTR_ERR(old_dentry);
if (IS_ERR(old_dentry))
goto exit3;
@@ -5240,8 +5246,8 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
error = -ENOENT;
if (d_is_negative(old_dentry))
goto exit4;
- new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
- lookup_flags | target_flags);
+ new_dentry = lookup_one_qstr(&new_last, new_path.dentry,
+ lookup_flags | target_flags);
error = PTR_ERR(new_dentry);
if (IS_ERR(new_dentry))
goto exit4;
@@ -5292,6 +5298,7 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
rd.flags = flags;
error = vfs_rename(&rd);
exit5:
+ d_lookup_done(new_dentry);
dput(new_dentry);
exit4:
dput(old_dentry);
diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index 4e580bb7baf8..89b3823f6405 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -109,7 +109,7 @@ static int ksmbd_vfs_path_lookup_locked(struct ksmbd_share_config *share_conf,
}
inode_lock_nested(parent_path->dentry->d_inode, I_MUTEX_PARENT);
- d = lookup_one_qstr_excl(&last, parent_path->dentry, 0);
+ d = lookup_one_qstr(&last, parent_path->dentry, 0);
if (IS_ERR(d))
goto err_out;
@@ -726,8 +726,8 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
ksmbd_fd_put(work, parent_fp);
}
- new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
- lookup_flags | LOOKUP_RENAME_TARGET);
+ new_dentry = lookup_one_qstr(&new_last, new_path.dentry,
+ lookup_flags | LOOKUP_RENAME_TARGET);
if (IS_ERR(new_dentry)) {
err = PTR_ERR(new_dentry);
goto out3;
@@ -771,6 +771,7 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
ksmbd_debug(VFS, "vfs_rename failed err %d\n", err);
out4:
+ d_lookup_done(new_dentry);
dput(new_dentry);
out3:
dput(old_parent);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 8ec8fed3bce8..06bb3ea65beb 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -34,6 +34,9 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
#define LOOKUP_EXCL 0x0400 /* ... in exclusive creation */
#define LOOKUP_RENAME_TARGET 0x0800 /* ... in destination of rename() */
+#define LOOKUP_INTENT_FLAGS (LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_EXCL | \
+ LOOKUP_RENAME_TARGET)
+
/* internal use only */
#define LOOKUP_PARENT 0x0010
@@ -52,9 +55,9 @@ extern int path_pts(struct path *path);
extern int user_path_at(int, const char __user *, unsigned, struct path *);
-struct dentry *lookup_one_qstr_excl(const struct qstr *name,
- struct dentry *base,
- unsigned int flags);
+struct dentry *lookup_one_qstr(const struct qstr *name,
+ struct dentry *base,
+ unsigned int flags);
extern int kern_path(const char *, unsigned, struct path *);
extern struct dentry *kern_path_create(int, const char *, struct path *, unsigned int);
--
2.47.1On Thu, Feb 06, 2025 at 04:42:40PM +1100, NeilBrown wrote: > - dentry = d_alloc(base, name); > - if (unlikely(!dentry)) > + dentry = d_alloc_parallel(base, name); > + if (unlikely(IS_ERR_OR_NULL(dentry))) > return ERR_PTR(-ENOMEM); Huh? When does d_alloc_parallel() return NULL and why do you play with explicit ERR_PTR(-ENOMEM) here? > + if ((flags & LOOKUP_INTENT_FLAGS) == 0) Yecchh... Thank you (from all reviewers, I suspect) for the exciting opportunity to verify what values are possible in lookup_flags in various callers and which are guaranteed to intersect with your LOOKUP_INTENT_FLAGS mask. > +#define LOOKUP_INTENT_FLAGS (LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_EXCL | \ > + LOOKUP_RENAME_TARGET) > + ... as well as figuring out WTF do LOOKUP_OPEN and LOOKUP_EXCL fit into that.
On Thu, 2025-02-06 at 16:42 +1100, NeilBrown wrote:
> lookup_one_qstr_excl() is used for lookups prior to directory
> modifications, whether create, unlink, rename, or whatever.
>
> To prepare for allowing modification to happen in parallel, change
> lookup_one_qstr_excl() to use d_alloc_parallel().
>
> To reflect this, name is changed to lookup_one_qtr() - as the directory
> may be locked shared.
>
> If any for the "intent" LOOKUP flags are passed, the caller must ensure
> d_lookup_done() is called at an appropriate time. If none are passed
> then we can be sure ->lookup() will do a real lookup and d_lookup_done()
> is called internally.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/namei.c | 47 +++++++++++++++++++++++++------------------
> fs/smb/server/vfs.c | 7 ++++---
> include/linux/namei.h | 9 ++++++---
> 3 files changed, 37 insertions(+), 26 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 5cdbd2eb4056..d684102d873d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1665,15 +1665,13 @@ static struct dentry *lookup_dcache(const struct qstr *name,
> }
>
> /*
> - * Parent directory has inode locked exclusive. This is one
> - * and only case when ->lookup() gets called on non in-lookup
> - * dentries - as the matter of fact, this only gets called
> - * when directory is guaranteed to have no in-lookup children
> - * at all.
> + * Parent directory has inode locked: exclusive or shared.
> + * If @flags contains any LOOKUP_INTENT_FLAGS then d_lookup_done()
> + * must be called after the intended operation is performed - or aborted.
> */
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> - struct dentry *base,
> - unsigned int flags)
> +struct dentry *lookup_one_qstr(const struct qstr *name,
> + struct dentry *base,
> + unsigned int flags)
> {
> struct dentry *dentry = lookup_dcache(name, base, flags);
> struct dentry *old;
> @@ -1686,18 +1684,25 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> if (unlikely(IS_DEADDIR(dir)))
> return ERR_PTR(-ENOENT);
>
> - dentry = d_alloc(base, name);
> - if (unlikely(!dentry))
> + dentry = d_alloc_parallel(base, name);
> + if (unlikely(IS_ERR_OR_NULL(dentry)))
> return ERR_PTR(-ENOMEM);
> + if (!d_in_lookup(dentry))
> + /* Raced with another thread which did the lookup */
> + return dentry;
>
> old = dir->i_op->lookup(dir, dentry, flags);
> if (unlikely(old)) {
> + d_lookup_done(dentry);
> dput(dentry);
> dentry = old;
> }
> + if ((flags & LOOKUP_INTENT_FLAGS) == 0)
> + /* ->lookup must have given final answer */
> + d_lookup_done(dentry);
This is kind of an ugly thing for the callers to get right. I think it
would be cleaner to just push the d_lookup_done() into all of the
callers that don't pass any intent flags, and do away with this.
> return dentry;
> }
> -EXPORT_SYMBOL(lookup_one_qstr_excl);
> +EXPORT_SYMBOL(lookup_one_qstr);
>
> /**
> * lookup_fast - do fast lockless (but racy) lookup of a dentry
> @@ -2739,7 +2744,7 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
> return ERR_PTR(-EINVAL);
> }
> inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> - d = lookup_one_qstr_excl(&last, path->dentry, 0);
> + d = lookup_one_qstr(&last, path->dentry, 0);
> if (IS_ERR(d)) {
> inode_unlock(path->dentry->d_inode);
> path_put(path);
> @@ -4078,8 +4083,8 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> if (last.name[last.len] && !want_dir)
> create_flags = 0;
> inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> - dentry = lookup_one_qstr_excl(&last, path->dentry,
> - reval_flag | create_flags);
> + dentry = lookup_one_qstr(&last, path->dentry,
> + reval_flag | create_flags);
> if (IS_ERR(dentry))
> goto unlock;
>
> @@ -4103,6 +4108,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> }
> return dentry;
> fail:
> + d_lookup_done(dentry);
> dput(dentry);
> dentry = ERR_PTR(error);
> unlock:
> @@ -4508,7 +4514,7 @@ int do_rmdir(int dfd, struct filename *name)
> goto exit2;
>
> inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
> - dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
> + dentry = lookup_one_qstr(&last, path.dentry, lookup_flags);
> error = PTR_ERR(dentry);
> if (IS_ERR(dentry))
> goto exit3;
> @@ -4641,7 +4647,7 @@ int do_unlinkat(int dfd, struct filename *name)
> goto exit2;
> retry_deleg:
> inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
> - dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
> + dentry = lookup_one_qstr(&last, path.dentry, lookup_flags);
> error = PTR_ERR(dentry);
> if (!IS_ERR(dentry)) {
>
> @@ -5231,8 +5237,8 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> goto exit_lock_rename;
> }
>
> - old_dentry = lookup_one_qstr_excl(&old_last, old_path.dentry,
> - lookup_flags);
> + old_dentry = lookup_one_qstr(&old_last, old_path.dentry,
> + lookup_flags);
> error = PTR_ERR(old_dentry);
> if (IS_ERR(old_dentry))
> goto exit3;
> @@ -5240,8 +5246,8 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> error = -ENOENT;
> if (d_is_negative(old_dentry))
> goto exit4;
> - new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
> - lookup_flags | target_flags);
> + new_dentry = lookup_one_qstr(&new_last, new_path.dentry,
> + lookup_flags | target_flags);
> error = PTR_ERR(new_dentry);
> if (IS_ERR(new_dentry))
> goto exit4;
> @@ -5292,6 +5298,7 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> rd.flags = flags;
> error = vfs_rename(&rd);
> exit5:
> + d_lookup_done(new_dentry);
> dput(new_dentry);
> exit4:
> dput(old_dentry);
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index 4e580bb7baf8..89b3823f6405 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -109,7 +109,7 @@ static int ksmbd_vfs_path_lookup_locked(struct ksmbd_share_config *share_conf,
> }
>
> inode_lock_nested(parent_path->dentry->d_inode, I_MUTEX_PARENT);
> - d = lookup_one_qstr_excl(&last, parent_path->dentry, 0);
> + d = lookup_one_qstr(&last, parent_path->dentry, 0);
> if (IS_ERR(d))
> goto err_out;
>
> @@ -726,8 +726,8 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> ksmbd_fd_put(work, parent_fp);
> }
>
> - new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
> - lookup_flags | LOOKUP_RENAME_TARGET);
> + new_dentry = lookup_one_qstr(&new_last, new_path.dentry,
> + lookup_flags | LOOKUP_RENAME_TARGET);
> if (IS_ERR(new_dentry)) {
> err = PTR_ERR(new_dentry);
> goto out3;
> @@ -771,6 +771,7 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> ksmbd_debug(VFS, "vfs_rename failed err %d\n", err);
>
> out4:
> + d_lookup_done(new_dentry);
> dput(new_dentry);
> out3:
> dput(old_parent);
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 8ec8fed3bce8..06bb3ea65beb 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -34,6 +34,9 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
> #define LOOKUP_EXCL 0x0400 /* ... in exclusive creation */
> #define LOOKUP_RENAME_TARGET 0x0800 /* ... in destination of rename() */
>
> +#define LOOKUP_INTENT_FLAGS (LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_EXCL | \
> + LOOKUP_RENAME_TARGET)
> +
> /* internal use only */
> #define LOOKUP_PARENT 0x0010
>
> @@ -52,9 +55,9 @@ extern int path_pts(struct path *path);
>
> extern int user_path_at(int, const char __user *, unsigned, struct path *);
>
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> - struct dentry *base,
> - unsigned int flags);
> +struct dentry *lookup_one_qstr(const struct qstr *name,
> + struct dentry *base,
> + unsigned int flags);
> extern int kern_path(const char *, unsigned, struct path *);
>
> extern struct dentry *kern_path_create(int, const char *, struct path *, unsigned int);
--
Jeff Layton <jlayton@kernel.org>
On Fri, 07 Feb 2025, Jeff Layton wrote:
> On Thu, 2025-02-06 at 16:42 +1100, NeilBrown wrote:
> > lookup_one_qstr_excl() is used for lookups prior to directory
> > modifications, whether create, unlink, rename, or whatever.
> >
> > To prepare for allowing modification to happen in parallel, change
> > lookup_one_qstr_excl() to use d_alloc_parallel().
> >
> > To reflect this, name is changed to lookup_one_qtr() - as the directory
> > may be locked shared.
> >
> > If any for the "intent" LOOKUP flags are passed, the caller must ensure
> > d_lookup_done() is called at an appropriate time. If none are passed
> > then we can be sure ->lookup() will do a real lookup and d_lookup_done()
> > is called internally.
> >
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > fs/namei.c | 47 +++++++++++++++++++++++++------------------
> > fs/smb/server/vfs.c | 7 ++++---
> > include/linux/namei.h | 9 ++++++---
> > 3 files changed, 37 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 5cdbd2eb4056..d684102d873d 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1665,15 +1665,13 @@ static struct dentry *lookup_dcache(const struct qstr *name,
> > }
> >
> > /*
> > - * Parent directory has inode locked exclusive. This is one
> > - * and only case when ->lookup() gets called on non in-lookup
> > - * dentries - as the matter of fact, this only gets called
> > - * when directory is guaranteed to have no in-lookup children
> > - * at all.
> > + * Parent directory has inode locked: exclusive or shared.
> > + * If @flags contains any LOOKUP_INTENT_FLAGS then d_lookup_done()
> > + * must be called after the intended operation is performed - or aborted.
> > */
> > -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > - struct dentry *base,
> > - unsigned int flags)
> > +struct dentry *lookup_one_qstr(const struct qstr *name,
> > + struct dentry *base,
> > + unsigned int flags)
> > {
> > struct dentry *dentry = lookup_dcache(name, base, flags);
> > struct dentry *old;
> > @@ -1686,18 +1684,25 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > if (unlikely(IS_DEADDIR(dir)))
> > return ERR_PTR(-ENOENT);
> >
> > - dentry = d_alloc(base, name);
> > - if (unlikely(!dentry))
> > + dentry = d_alloc_parallel(base, name);
> > + if (unlikely(IS_ERR_OR_NULL(dentry)))
> > return ERR_PTR(-ENOMEM);
> > + if (!d_in_lookup(dentry))
> > + /* Raced with another thread which did the lookup */
> > + return dentry;
> >
> > old = dir->i_op->lookup(dir, dentry, flags);
> > if (unlikely(old)) {
> > + d_lookup_done(dentry);
> > dput(dentry);
> > dentry = old;
> > }
> > + if ((flags & LOOKUP_INTENT_FLAGS) == 0)
> > + /* ->lookup must have given final answer */
> > + d_lookup_done(dentry);
>
> This is kind of an ugly thing for the callers to get right. I think it
> would be cleaner to just push the d_lookup_done() into all of the
> callers that don't pass any intent flags, and do away with this.
I don't understand your concern. This does not impose on callers,
rather it relieves them of a burden. d_lookup_done() is fully
idempotent so if a caller does call it, there is no harm done.
In the final result of my series there are 4 callers of this function.
1/ lookup_and_lock() which must always be balanced with
done_lookup_and_lock(), which calls d_lookup_done()
2/ lookup_and_lock_rename() which is similarly balance with
done_lookup_and_lock_rename().
3/ ksmbd_vfs_path_lookup_locked() which passes zero for the flags and so
doesn't need d_lookup_done()
4/ ksmbd_vfs_rename() which calls d_lookup_done() as required.
So if I dropped this code it would only affect one caller which would
need to add a call to d_lookup_done() probably immediately after the
successful return of lookup_one_qstr().
While that wouldn't hurt much, I don't see that it would help much
either.
Thanks,
NeilBrown
On Fri, 2025-02-07 at 11:04 +1100, NeilBrown wrote:
> On Fri, 07 Feb 2025, Jeff Layton wrote:
> > On Thu, 2025-02-06 at 16:42 +1100, NeilBrown wrote:
> > > lookup_one_qstr_excl() is used for lookups prior to directory
> > > modifications, whether create, unlink, rename, or whatever.
> > >
> > > To prepare for allowing modification to happen in parallel, change
> > > lookup_one_qstr_excl() to use d_alloc_parallel().
> > >
> > > To reflect this, name is changed to lookup_one_qtr() - as the directory
> > > may be locked shared.
> > >
> > > If any for the "intent" LOOKUP flags are passed, the caller must ensure
> > > d_lookup_done() is called at an appropriate time. If none are passed
> > > then we can be sure ->lookup() will do a real lookup and d_lookup_done()
> > > is called internally.
> > >
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > > fs/namei.c | 47 +++++++++++++++++++++++++------------------
> > > fs/smb/server/vfs.c | 7 ++++---
> > > include/linux/namei.h | 9 ++++++---
> > > 3 files changed, 37 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 5cdbd2eb4056..d684102d873d 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -1665,15 +1665,13 @@ static struct dentry *lookup_dcache(const struct qstr *name,
> > > }
> > >
> > > /*
> > > - * Parent directory has inode locked exclusive. This is one
> > > - * and only case when ->lookup() gets called on non in-lookup
> > > - * dentries - as the matter of fact, this only gets called
> > > - * when directory is guaranteed to have no in-lookup children
> > > - * at all.
> > > + * Parent directory has inode locked: exclusive or shared.
> > > + * If @flags contains any LOOKUP_INTENT_FLAGS then d_lookup_done()
> > > + * must be called after the intended operation is performed - or aborted.
> > > */
> > > -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > > - struct dentry *base,
> > > - unsigned int flags)
> > > +struct dentry *lookup_one_qstr(const struct qstr *name,
> > > + struct dentry *base,
> > > + unsigned int flags)
> > > {
> > > struct dentry *dentry = lookup_dcache(name, base, flags);
> > > struct dentry *old;
> > > @@ -1686,18 +1684,25 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > > if (unlikely(IS_DEADDIR(dir)))
> > > return ERR_PTR(-ENOENT);
> > >
> > > - dentry = d_alloc(base, name);
> > > - if (unlikely(!dentry))
> > > + dentry = d_alloc_parallel(base, name);
> > > + if (unlikely(IS_ERR_OR_NULL(dentry)))
> > > return ERR_PTR(-ENOMEM);
> > > + if (!d_in_lookup(dentry))
> > > + /* Raced with another thread which did the lookup */
> > > + return dentry;
> > >
> > > old = dir->i_op->lookup(dir, dentry, flags);
> > > if (unlikely(old)) {
> > > + d_lookup_done(dentry);
> > > dput(dentry);
> > > dentry = old;
> > > }
> > > + if ((flags & LOOKUP_INTENT_FLAGS) == 0)
> > > + /* ->lookup must have given final answer */
> > > + d_lookup_done(dentry);
> >
> > This is kind of an ugly thing for the callers to get right. I think it
> > would be cleaner to just push the d_lookup_done() into all of the
> > callers that don't pass any intent flags, and do away with this.
>
> I don't understand your concern. This does not impose on callers,
> rather it relieves them of a burden. d_lookup_done() is fully
> idempotent so if a caller does call it, there is no harm done.
>
> In the final result of my series there are 4 callers of this function.
> 1/ lookup_and_lock() which must always be balanced with
> done_lookup_and_lock(), which calls d_lookup_done()
> 2/ lookup_and_lock_rename() which is similarly balance with
> done_lookup_and_lock_rename().
> 3/ ksmbd_vfs_path_lookup_locked() which passes zero for the flags and so
> doesn't need d_lookup_done()
> 4/ ksmbd_vfs_rename() which calls d_lookup_done() as required.
>
> So if I dropped this code it would only affect one caller which would
> need to add a call to d_lookup_done() probably immediately after the
> successful return of lookup_one_qstr().
> While that wouldn't hurt much, I don't see that it would help much
> either.
>
My concern is about the complex return handling. If the flags are 0,
then I don't need to call d_lookup_done(), but if they aren't 0, then I
do. That's just an easy opportunity to get it wrong if new callers are
added.
My preference would be that the caller must always call d_lookup_done()
on a successful return. If ksmbd_vfs_path_lookup_locked() has to call
it immediately afterward, then that's fine. No need for this special
handling in a generic function, just for a single caller.
--
Jeff Layton <jlayton@kernel.org>
© 2016 - 2025 Red Hat, Inc.