Re: [PATCH v2 0/6] kernfs: proposed locking and concurrency improvement

[Tejun Heo <tj@kernel.org>]

Hello,

On Sat, Dec 19, 2020 at 03:08:13PM +0800, Ian Kent wrote:
> And looking further I see there's a race that kernfs can't do anything
> about between kernfs_refresh_inode() and fs/inode.c:update_times().

Do kernfs files end up calling into that path tho? Doesn't look like it to
me but if so yeah we'd need to override the update_time for kernfs.

> +static int kernfs_iop_update_time(struct inode *inode, struct timespec64 *time, int flags)
>  {
> -	struct inode *inode = d_inode(path->dentry);
>  	struct kernfs_node *kn = inode->i_private;
> +	struct kernfs_iattrs *attrs;
>  
>  	mutex_lock(&kernfs_mutex);
> +	attrs = kernfs_iattrs(kn);
> +	if (!attrs) {
> +		mutex_unlock(&kernfs_mutex);
> +		return -ENOMEM;
> +	}
> +
> +	/* Which kernfs node attributes should be updated from
> +	 * time?
> +	 */
> +
>  	kernfs_refresh_inode(kn, inode);
>  	mutex_unlock(&kernfs_mutex);

I don't see how this would reflect the changes from kernfs_setattr() into
the attached inode. This would actually make the attr updates obviously racy
- the userland visible attrs would be stale until the inode gets reclaimed
and then when it gets reinstantiated it'd show the latest information.

That said, if you wanna take the direction where attr updates are reflected
to the associated inode when the change occurs, which makes sense, the right
thing to do would be making kernfs_setattr() update the associated inode if
existent.

Thanks.

-- 
tejun