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

From: ebiederm@xmission.com (Eric W. Biederman)
To: Ian Kent <raven@themaw.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Tejun Heo <tj@kernel.org>, Eric Sandeen <sandeen@sandeen.net>,
	Fox Chen <foxhlchen@gmail.com>,
	Brice Goglin <brice.goglin@gmail.com>,
	Al Viro <viro@ZenIV.linux.org.uk>,
	Rick Lindsley <ricklind@linux.vnet.ibm.com>,
	David Howells <dhowells@redhat.com>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	Carlos Maiolino <cmaiolino@redhat.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/6] kernfs: add a revision to identify directory node changes
Date: Mon, 07 Jun 2021 12:53:37 -0500	[thread overview]
Message-ID: <87a6o1k1cu.fsf@disp2133> (raw)
In-Reply-To: <162306071065.69474.8064509709844383785.stgit@web.messagingengine.com> (Ian Kent's message of "Mon, 07 Jun 2021 18:11:50 +0800")

Ian Kent <raven@themaw.net> 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@themaw.net>
> ---
>  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?

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.

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

> +	}
> +	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