On Mon, 2021-06-07 at 12:53 -0500, Eric W. Biederman wrote:
> Ian Kent <raven@xxxxxxxxxx> writes:
>
> > Add a revision counter to kernfs directory nodes so it can be used
> > to detect if a directory node has changed.
> >
> > 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 | 8 ++++++++
> > fs/kernfs/kernfs-internal.h | 24 ++++++++++++++++++++++++
> > include/linux/kernfs.h | 5 +++++
> > 3 files changed, 37 insertions(+)
> >
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 33166ec90a112..b88432c48851f 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);
> > @@ -1105,6 +1107,12 @@ static struct dentry
> > *kernfs_iop_lookup(struct inode *dir,
> >
> > /* instantiate and hash dentry */
> > ret = d_splice_alias(inode, dentry);
> > + if (!IS_ERR(ret)) {
> > + if (unlikely(ret))
> > + kernfs_set_rev(parent, ret);
> > + else
> > + kernfs_set_rev(parent, dentry);
>
> Do we care about d_time on non-NULL dentries?
Would we ever need to use it avoid a search for any other cases?
Probably not ... those export ops mean that some dentries might
not have d_time set.
Maybe it's best to put a comment in about only using it for
negative dentries and set it unconditionally in ->lookup() as
you describe.
>
> For d_splice_alias to return a different dentry implies
> that the dentry was non-NULL.
>
> I am wondering if having a guarantee that d_time never changes could
> help simplify the implementation. For never changing it would see to
> make sense to call kernfs_set_rev before d_splice_alias on dentry,
> and
> simply not worry about it after d_splice_alias.
Yes, I was tempted to do that.
>
> > + }
> > out_unlock:
> > mutex_unlock(&kernfs_mutex);
> > return ret;
> > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-
> > internal.h
> > index ccc3b44f6306f..1536002584fc4 100644
> > --- a/fs/kernfs/kernfs-internal.h
> > +++ b/fs/kernfs/kernfs-internal.h
> > @@ -81,6 +81,30 @@ 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) {
> > + /* Not really a time bit it does what's needed */
> > + if (time_after(kn->dir.rev, dentry->d_time))
> > + return true;
>
> Why not simply make this:
> if (kn->dir.rev != dentry->d_time)
> return true;
>
> I don't see what is gained by not counting as changed something in
> the
> wrong half of the values.
Yes, it was like that originally and really shouldn't make
any difference. I'll change it back.
Ian
>
> > + }
> > + 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..7947acb1163d7 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 revalidation.
> > + */
> > + unsigned long rev;
> > };
> >
> > struct kernfs_elem_symlink {
>
> Eric
