Negative dentries support on case-insensitive ext4/f2fs will require
access to the name under lookup to ensure it matches the dentry. This
adds an optional new flavor of cached dentry revalidation hook to expose
this extra parameter.
I'm fine with extending d_revalidate instead of adding a new hook, if
it is considered cleaner and the approach is accepted. I wrote a new
hook to simplify reviewing.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
fs/dcache.c | 2 +-
fs/namei.c | 23 ++++++++++++++---------
include/linux/dcache.h | 1 +
3 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 93f4f5ee07bf..a0fe9e3676fb 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1928,7 +1928,7 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
dentry->d_flags |= DCACHE_OP_HASH;
if (op->d_compare)
dentry->d_flags |= DCACHE_OP_COMPARE;
- if (op->d_revalidate)
+ if (op->d_revalidate || op->d_revalidate_name)
dentry->d_flags |= DCACHE_OP_REVALIDATE;
if (op->d_weak_revalidate)
dentry->d_flags |= DCACHE_OP_WEAK_REVALIDATE;
diff --git a/fs/namei.c b/fs/namei.c
index 1f28d3f463c3..c01bb19723db 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -842,11 +842,16 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi
return false;
}
-static inline int d_revalidate(struct dentry *dentry, unsigned int flags)
+static inline int d_revalidate(struct dentry *dentry,
+ const struct qstr *name,
+ unsigned int flags)
{
- if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE))
+
+ if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) {
+ if (dentry->d_op->d_revalidate_name)
+ return dentry->d_op->d_revalidate_name(dentry, name, flags);
return dentry->d_op->d_revalidate(dentry, flags);
- else
+ } else
return 1;
}
@@ -1563,7 +1568,7 @@ static struct dentry *lookup_dcache(const struct qstr *name,
{
struct dentry *dentry = d_lookup(dir, name);
if (dentry) {
- int error = d_revalidate(dentry, flags);
+ int error = d_revalidate(dentry, name, flags);
if (unlikely(error <= 0)) {
if (!error)
d_invalidate(dentry);
@@ -1647,19 +1652,19 @@ static struct dentry *lookup_fast(struct nameidata *nd,
return ERR_PTR(-ECHILD);
*seqp = seq;
- status = d_revalidate(dentry, nd->flags);
+ status = d_revalidate(dentry, &nd->last, nd->flags);
if (likely(status > 0))
return dentry;
if (!try_to_unlazy_next(nd, dentry, seq))
return ERR_PTR(-ECHILD);
if (status == -ECHILD)
/* we'd been told to redo it in non-rcu mode */
- status = d_revalidate(dentry, nd->flags);
+ status = d_revalidate(dentry, &nd->last, nd->flags);
} else {
dentry = __d_lookup(parent, &nd->last);
if (unlikely(!dentry))
return NULL;
- status = d_revalidate(dentry, nd->flags);
+ status = d_revalidate(dentry, &nd->last, nd->flags);
}
if (unlikely(status <= 0)) {
if (!status)
@@ -1687,7 +1692,7 @@ static struct dentry *__lookup_slow(const struct qstr *name,
if (IS_ERR(dentry))
return dentry;
if (unlikely(!d_in_lookup(dentry))) {
- int error = d_revalidate(dentry, flags);
+ int error = d_revalidate(dentry, name, flags);
if (unlikely(error <= 0)) {
if (!error) {
d_invalidate(dentry);
@@ -3302,7 +3307,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
if (d_in_lookup(dentry))
break;
- error = d_revalidate(dentry, nd->flags);
+ error = d_revalidate(dentry, &nd->last, nd->flags);
if (likely(error > 0))
break;
if (error)
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index f5bba51480b2..871f65c8ef7f 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -126,6 +126,7 @@ enum dentry_d_lock_class
struct dentry_operations {
int (*d_revalidate)(struct dentry *, unsigned int);
+ int (*d_revalidate_name)(struct dentry *, const struct qstr *, unsigned int);
int (*d_weak_revalidate)(struct dentry *, unsigned int);
int (*d_hash)(const struct dentry *, struct qstr *);
int (*d_compare)(const struct dentry *,
--
2.36.1
On Wed, Jun 22, 2022 at 03:45:57PM -0400, Gabriel Krisman Bertazi wrote:
> Negative dentries support on case-insensitive ext4/f2fs will require
> access to the name under lookup to ensure it matches the dentry. This
> adds an optional new flavor of cached dentry revalidation hook to expose
> this extra parameter.
>
> I'm fine with extending d_revalidate instead of adding a new hook, if
> it is considered cleaner and the approach is accepted. I wrote a new
> hook to simplify reviewing.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
Reviewed-by: Theodore Ts'o <[email protected]>
Al, could you take a look and see if you have any objections?
Also, any thoughts about adding the new d_revalidate_name() callback
as opposed to change d_revalidate() to add an extra parameter? It
looks like there are some 33 d_revalidate callbacks, from 24 file
sysetms, that would have to be changed.
Cheers,
- Ted
On Thu, Mar 23, 2023 at 10:33:20AM -0400, Theodore Ts'o wrote:
> On Wed, Jun 22, 2022 at 03:45:57PM -0400, Gabriel Krisman Bertazi wrote:
> > Negative dentries support on case-insensitive ext4/f2fs will require
> > access to the name under lookup to ensure it matches the dentry. This
> > adds an optional new flavor of cached dentry revalidation hook to expose
> > this extra parameter.
> >
> > I'm fine with extending d_revalidate instead of adding a new hook, if
> > it is considered cleaner and the approach is accepted. I wrote a new
> > hook to simplify reviewing.
> >
> > Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
>
> Reviewed-by: Theodore Ts'o <[email protected]>
>
> Al, could you take a look and see if you have any objections?
Ping, Al, any objsections if I take Gabriel's patch series via the
ext4 tree?
- Ted
On Sat, Mar 25, 2023 at 09:33:10AM -0400, Theodore Ts'o wrote:
> On Thu, Mar 23, 2023 at 10:33:20AM -0400, Theodore Ts'o wrote:
> > On Wed, Jun 22, 2022 at 03:45:57PM -0400, Gabriel Krisman Bertazi wrote:
> > > Negative dentries support on case-insensitive ext4/f2fs will require
> > > access to the name under lookup to ensure it matches the dentry. This
> > > adds an optional new flavor of cached dentry revalidation hook to expose
> > > this extra parameter.
> > >
> > > I'm fine with extending d_revalidate instead of adding a new hook, if
> > > it is considered cleaner and the approach is accepted. I wrote a new
> > > hook to simplify reviewing.
> > >
> > > Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> >
> > Reviewed-by: Theodore Ts'o <[email protected]>
> >
> > Al, could you take a look and see if you have any objections?
>
> Ping, Al, any objsections if I take Gabriel's patch series via the
> ext4 tree?
The really subtle part is ->d_name stability in there. We probably are OK
as it is with the current tree (at least I hope so), but it really needs
to be documented - the proof of correctness is not straightforward and it's
going to be brittle; it's not obvious that this memcmp() relies upon the
parent being locked in all cases when we get to calling it. And if that
ever becomes not true, we have a hard-to-debug source of occasional oopsen ;-/
It can be done without reliance on locking - take a look at the vicinity of
dentry_cmp() in fs/dcache.c for example of such, but it's very much not
a blind memcmp(). And I suspect that it would be an overkill here.
In any case, that needs to be discussed in commit message and clearly
spelled out. Otherwise it's a trouble waiting to happen.