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@xxxxxxxxxx>
Reviewed-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> ---
> 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
