On Fri, 2021-06-11 at 15:11 +0200, Greg Kroah-Hartman wrote:

On Fri, Jun 11, 2021 at 08:56:18PM +0800, Ian Kent wrote:

quoted

On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote:

quoted

On Wed, 9 Jun 2021 at 10:50, Ian Kent [off-list ref] wrote:

quoted

Add a revision counter to kernfs directory nodes so it can be
used
to detect if a directory node has changed during negative
dentry
revalidation.

There's an assumption that sizeof(unsigned long) <=
sizeof(pointer)
on all architectures and as far as I know that assumption
holds.

So adding a revision counter to the struct kernfs_elem_dir
variant
of
the kernfs_node type union won't increase the size of the
kernfs_node
struct. This is because struct kernfs_elem_dir is at least
sizeof(pointer) smaller than the largest union variant. It's
tempting
to make the revision counter a u64 but that would increase the
size
of
kernfs_node on archs where sizeof(pointer) is smaller than the
revision
counter.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/kernfs/dir.c             |    2 ++
 fs/kernfs/kernfs-internal.h |   23 +++++++++++++++++++++++
 include/linux/kernfs.h      |    5 +++++
 3 files changed, 30 insertions(+)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 33166ec90a112..b3d1bc0f317d0 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c

@@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct

kernfs_node *kn)
        /* successfully added, account subdir number */
        if (kernfs_type(kn) == KERNFS_DIR)
                kn->parent->dir.subdirs++;
+       kernfs_inc_rev(kn->parent);

        return 0;
 }

@@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct

kernfs_node *kn)

        if (kernfs_type(kn) == KERNFS_DIR)
                kn->parent->dir.subdirs--;
+       kernfs_inc_rev(kn->parent);

        rb_erase(&kn->rb, &kn->parent->dir.children);
        RB_CLEAR_NODE(&kn->rb);

diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-

internal.h
index ccc3b44f6306f..b4e7579e04799 100644

--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h

@@ -81,6 +81,29 @@ static inline struct kernfs_node

*kernfs_dentry_node(struct dentry *dentry)
        return d_inode(dentry)->i_private;
 }

+static inline void kernfs_set_rev(struct kernfs_node *kn,
+                                 struct dentry *dentry)
+{
+       if (kernfs_type(kn) == KERNFS_DIR)
+               dentry->d_time = kn->dir.rev;
+}
+
+static inline void kernfs_inc_rev(struct kernfs_node *kn)
+{
+       if (kernfs_type(kn) == KERNFS_DIR)
+               kn->dir.rev++;
+}
+
+static inline bool kernfs_dir_changed(struct kernfs_node *kn,
+                                     struct dentry *dentry)
+{
+       if (kernfs_type(kn) == KERNFS_DIR) {

Aren't these always be called on a KERNFS_DIR node?

Yes they are.

quoted

You could just reduce that to a WARN_ON, or remove the conditions
altogether then.

I was tempted to not use the check, a WARN_ON sounds better than
removing the check, I'll do that in a v7.

No, WARN_ON is not ok, as systems will crash if panic-on-warn is set.

Thanks Greg, understood.

If these are impossible to hit, great, let's not check this and we
can
just drop the code.  If they can be hit, then the above code is
correct
and it should stay.

It's a programming mistake to call these on a non-directory node.

I can remove the check but do you think there's any value in passing
the node and updating it's parent to avoid possible misuse?

Ian