diff mbox series

[v6,2/7] kernfs: add a revision to identify directory node changes

Message ID 162322859985.361452.14110524195807923374.stgit@web.messagingengine.com (mailing list archive)
State New
Headers show
Series kernfs: proposed locking and concurrency improvement | expand

Commit Message

Ian Kent June 9, 2021, 8:49 a.m. UTC
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(+)

Comments

Miklos Szeredi June 11, 2021, 12:49 p.m. UTC | #1
On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote:
>
> 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?

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

Thanks,
Miklos
Ian Kent June 11, 2021, 12:56 p.m. UTC | #2
On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote:
> On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote:
> > 
> > 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.

> 
> 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.

Thanks
Ian
Greg Kroah-Hartman June 11, 2021, 1:11 p.m. UTC | #3
On Fri, Jun 11, 2021 at 08:56:18PM +0800, Ian Kent wrote:
> On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote:
> > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote:
> > > 
> > > 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.
> 
> > 
> > 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.

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.

thanks,

greg k-h
Ian Kent June 11, 2021, 1:31 p.m. UTC | #4
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:
> > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote:
> > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote:
> > > > 
> > > > 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.
> > 
> > > 
> > > 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
Greg Kroah-Hartman June 11, 2021, 2:05 p.m. UTC | #5
On Fri, Jun 11, 2021 at 09:31:36PM +0800, Ian Kent wrote:
> 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:
> > > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote:
> > > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote:
> > > > > 
> > > > > 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.
> > > 
> > > > 
> > > > 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?

I do not understand the question here, sorry.  It's a static function,
you control the callers, who can "misuse" it?

thanks,

greg k-h
Ian Kent June 11, 2021, 2:16 p.m. UTC | #6
On Fri, 2021-06-11 at 16:05 +0200, Greg Kroah-Hartman wrote:
> On Fri, Jun 11, 2021 at 09:31:36PM +0800, Ian Kent wrote:
> > 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:
> > > > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote:
> > > > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net>
> > > > > wrote:
> > > > > > 
> > > > > > 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.
> > > > 
> > > > > 
> > > > > 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?
> 
> I do not understand the question here, sorry.  It's a static
> function,
> you control the callers, who can "misuse" it?

Yes, I'll drop the test and name the argument parent to make it
clear for readers.


Ian
diff mbox series

Patch

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) {
+		if (kn->dir.rev != dentry->d_time)
+			return true;
+	}
+	return false;
+}
+
 extern const struct super_operations kernfs_sops;
 extern struct kmem_cache *kernfs_node_cache, *kernfs_iattrs_cache;
 
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 9e8ca8743c268..d7e0160fce6df 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -98,6 +98,11 @@  struct kernfs_elem_dir {
 	 * better directly in kernfs_node but is here to save space.
 	 */
 	struct kernfs_root	*root;
+	/*
+	 * Monotonic revision counter, used to identify if a directory
+	 * node has changed during negative dentry revalidation.
+	 */
+	unsigned long rev;
 };
 
 struct kernfs_elem_symlink {