| Message ID | 20220622194603.102655-2-krisman@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support negative dentries on case-insensitive directories | expand |
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 <krisman@collabora.com> Reviewed-by: Theodore Ts'o <tytso@mit.edu> 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 <krisman@collabora.com> > > Reviewed-by: Theodore Ts'o <tytso@mit.edu> > > 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 <krisman@collabora.com> > > > > Reviewed-by: Theodore Ts'o <tytso@mit.edu> > > > > 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.
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 *,
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 <krisman@collabora.com> --- fs/dcache.c | 2 +- fs/namei.c | 23 ++++++++++++++--------- include/linux/dcache.h | 1 + 3 files changed, 16 insertions(+), 10 deletions(-)