Re: [PATCH v4 03/11] fs/9p: Consolidate file operations and add readahead and writeback

From: asmadeus@codewreck.org
To: Eric Van Hensbergen <ericvh@kernel.org>
Cc: v9fs-developer@lists.sourceforge.net, rminnich@gmail.com,
	lucho@ionkov.net, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux_oss@crudebyte.com
Subject: Re: [PATCH v4 03/11] fs/9p: Consolidate file operations and add readahead and writeback
Date: Sat, 18 Feb 2023 18:24:36 +0900	[thread overview]
Message-ID: <Y/CZVEQPFFo0zMjo@codewreck.org> (raw)
In-Reply-To: <20230218003323.2322580-4-ericvh@kernel.org>

Eric Van Hensbergen wrote on Sat, Feb 18, 2023 at 12:33:15AM +0000:
> We had 3 different sets of file operations across 2 different protocol
> variants differentiated by cache which really only changed 3
> functions.  But the real problem is that certain file modes, mount
> options, and other factors weren't being considered when we
> decided whether or not to use caches.
> 
> This consolidates all the operations and switches
> to conditionals within a common set to decide whether or not
> to do different aspects of caching.
> 
> Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>

Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>

> ---
>  struct inode *v9fs_alloc_inode(struct super_block *sb);
> diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
> index 59b0e8948f78..bd31593437f3 100644
> --- a/fs/9p/vfs_dir.c
> +++ b/fs/9p/vfs_dir.c
> @@ -214,6 +214,9 @@ int v9fs_dir_release(struct inode *inode, struct file *filp)
>  	p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n",
>  		 inode, filp, fid ? fid->fid : -1);
>  	if (fid) {
> +		if ((fid->qid.type == P9_QTFILE) && (filp->f_mode & FMODE_WRITE))
> +			v9fs_flush_inode_writeback(inode);
> +

Ok so this bugged me to no end; that seems to be because we use the same
v9fs_dir_release for v9fs_file_operations's .release and not just
v9fs_dir_operations... So it's to be expected we'll get files here.

At this point I'd suggest to use two functions, but that's probably
overdoing it.
Let's check S_ISREG(inode->i_mode) instead of fid->qid though; it
shouldn't make any difference but that's what you use in other parts of
the code and it will be easier to understand for people familiar with
the vfs.

> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 33e521c60e2c..8ffa6631b1fd 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -219,6 +219,35 @@ v9fs_blank_wstat(struct p9_wstat *wstat)
>  	wstat->extension = NULL;
>  }
>  
> +/**
> + * v9fs_flush_inode_writeback - writeback any data associated with inode
> + * @inode: inode to writeback
> + *
> + * This is used to make sure anything that needs to be written
> + * to server gets flushed before we do certain operations (setattr, getattr, close)
> + *
> + */
> +
> +int v9fs_flush_inode_writeback(struct inode *inode)
> +{
> +	struct writeback_control wbc = {
> +		.nr_to_write = LONG_MAX,
> +		.sync_mode = WB_SYNC_ALL,
> +		.range_start = 0,
> +		.range_end = -1,
> +	};
> +
> +	int retval = filemap_fdatawrite_wbc(inode->i_mapping, &wbc);

Hmm, that function only starts the writeback, but doesn't wait for it.

Wasn't the point to replace 'filemap_write_and_wait' with
v9fs_flush_inode_writeback?
I don't think it's a good idea to remove the wait before setattrs and
the like; if you don't want to wait on close()'s release (but we
probably should too) perhaps split this in two?

> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index bff37a312e64..4f01808c3bae 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -593,9 +602,14 @@ int v9fs_vfs_setattr_dotl(struct user_namespace *mnt_userns,
>  		return retval;
>  	}
>  
> -	if ((iattr->ia_valid & ATTR_SIZE) &&
> -	    iattr->ia_size != i_size_read(inode))
> +	if ((iattr->ia_valid & ATTR_SIZE) && iattr->ia_size !=
> +		 i_size_read(inode)) {
>  		truncate_setsize(inode, iattr->ia_size);
> +		if (v9ses->cache == CACHE_FSCACHE)
> +			fscache_resize_cookie(v9fs_inode_cookie(v9inode), iattr->ia_size);
> +		else
> +			invalidate_mapping_pages(&inode->i_data, 0, -1);

Hm, I don't think these are exclusive; resize_cookie doesn't seem to do
much about the page cache.

However, truncate_setsize calls trucate_pagecache which I believe does
the right thing; and I don't see why we would need to invalidate
[0;size[ here? We didn't before.

. . . . . . .
Ah, you've replaced it preciesly with that in "fs/9p: writeback mode
fixes"; this is annoying to review :/

So with that problem gone, I think I'm ok with this patch with the
exception of the flush inode writeback that doesn't wait (and the
nitpick on S_ISREG)

-- 
Dominique