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

[Greg Kroah-Hartman]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >  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