Eric Van Hensbergen wrote on Sat, Feb 18, 2023 at 12:33:22AM +0000:
> This fixes several detected problems from preivous
> patches when running with writeback mode. In
> particular this fixes issues with files which are opened
> as write only and getattr on files which dirty caches.
>
> This patch makes sure that cache behavior for an open file is stored in
> the client copy of fid->mode. This allows us to reflect cache behavior
> from mount flags, open mode, and information from the server to
> inform readahead and writeback behavior.
>
> This includes adding support for a 9p semantic that qid.version==0
> is used to mark a file as non-cachable which is important for
> synthetic files. This may have a side-effect of not supporting
> caching on certain legacy file servers that do not properly set
> qid.version. There is also now a mount flag which can disable
> the qid.version behavior.
>
> Signed-off-by: Eric Van Hensbergen <ericvh@xxxxxxxxxx>
Didn't have time to review it all thoroughly, sending what I have
anyway...
> diff --git a/Documentation/filesystems/9p.rst b/Documentation/filesystems/9p.rst
> index 0e800b8f73cc..0c2c7a181d85 100644
> --- a/Documentation/filesystems/9p.rst
> +++ b/Documentation/filesystems/9p.rst
> @@ -79,18 +79,14 @@ Options
>
> cache=mode specifies a caching policy. By default, no caches are used.
>
> - none
> - default no cache policy, metadata and data
> - alike are synchronous.
> - loose
> - no attempts are made at consistency,
> - intended for exclusive, read-only mounts
> - fscache
> - use FS-Cache for a persistent, read-only
> - cache backend.
> - mmap
> - minimal cache that is only used for read-write
> - mmap. Northing else is cached, like cache=none
> + ========= =============================================
> + none no cache of file or metadata
> + readahead readahead caching of files
> + writeback delayed writeback of files
> + mmap support mmap operations read/write with cache
> + loose meta-data and file cache with no coherency
> + fscache use FS-Cache for a persistent cache backend
> + ========= =============================================
perhaps a word saying the caches are incremental, only one can be used,
and listing them in order?
e.g. it's not clear from this that writeback also enables readahead,
and as a user I'd try to use cache=readahead,cache=writeback and wonder
why that doesn't work (well, I guess it would in that order...)
> diff --git a/fs/9p/fid.c b/fs/9p/fid.c
> index 805151114e96..8c1697619f3d 100644
> --- a/fs/9p/fid.c
> +++ b/fs/9p/fid.c
> @@ -41,14 +40,24 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid **pfid)
> *pfid = NULL;
> }
>
> +static bool v9fs_is_writeable(int mode)
> +{
> + if ((mode & P9_OWRITE) || (mode & P9_ORDWR))
(style) that's usually written 'if (mode & (P9_OWRITE | P9_ORDWR))'
(I don't really care, the compiler will likely generate the same more
efficient check)
> @@ -32,4 +34,33 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
> p9_fid_put(fid);
> return nfid;
> }
> +/**
> + * v9fs_fid_addmodes - add cache flags to fid mode (for client use only)
> + * @fid: fid to augment
> + * @s_flags: session info mount flags
> + * @s_cache: session info cache flags
> + * @f_flags: unix open flags
> + *
> + * make sure mode reflects flags of underlying mounts
> + * also qid.version == 0 reflects a synthetic or legacy file system
> + * NOTE: these are set after open so only reflect 9p client not
> + * underlying file system on server.
Ok, so ignore my comment about that in other commit; but that note
really should also be in the header or commits should make sense in
order...
Rand aside, what's the point? It saves a lookup for the session in
v9fs_file_read/write_iter ? We don't support changing cache mode for new
fids with `mount -o remount` do we...
Ah, I see you're adding DIRECT to the mode if you fail opening the
writeback fid; ok that makes more sense.
I'd appreciate a comment as well for that, around the enum definition
rather than here, if you want to humor me on this.
> v9fs_file.c
> @@ -59,7 +59,19 @@ int v9fs_file_open(struct inode *inode, struct file *file)
> if (IS_ERR(fid))
> return PTR_ERR(fid);
>
> - err = p9_client_open(fid, omode);
> + if ((v9ses->cache >= CACHE_WRITEBACK) && (omode & P9_OWRITE)) {
> + int writeback_omode = (omode & !P9_OWRITE) | P9_ORDWR;
omode & ~P9_OWRITE ?
`!P9_OWRITE` will be 0...
> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
> index 5fc6a945bfff..797f717e1a91 100644
> --- a/fs/9p/vfs_super.c
> +++ b/fs/9p/vfs_super.c
> @@ -323,16 +327,17 @@ static int v9fs_write_inode_dotl(struct inode *inode,
> */
> v9inode = V9FS_I(inode);
> p9_debug(P9_DEBUG_VFS, "%s: inode %p, writeback_fid %p\n",
> - __func__, inode, v9inode->writeback_fid);
> - if (!v9inode->writeback_fid)
> - return 0;
> + __func__, inode, fid);
> + if (!fid)
> + return -EINVAL;
Hmm, what happens if we return EINVAL here?
Might want a WARN_ONCE or something?
