Re: [PATCH v5] fs/9p: remove writeback fid and fix per-file modes

Christophe JAILLET
2023-04-25T07:11:13Z

Le 27/03/2023 à 04:59, Eric Van Hensbergen a écrit :
> This patch removes the creating of an additional writeback_fid
> for opened files. The patch addresses problems when files
> were opened write-only or getattr on files with dirty caches.
>
> This patch also incorporates information about cache behavior
> in the fid for every file. 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@kernel.org>
> ---
> fs/9p/fid.c | 48 +++++++++-------------
> fs/9p/fid.h | 33 ++++++++++++++-
> fs/9p/v9fs.h | 1 -
> fs/9p/vfs_addr.c | 22 +++++-----
> fs/9p/vfs_file.c | 91 ++++++++++++++----------------------------
> fs/9p/vfs_inode.c | 45 +++++++--------------
> fs/9p/vfs_inode_dotl.c | 48 +++++++++-------------
> fs/9p/vfs_super.c | 33 ++++-----------
> 8 files changed, 135 insertions(+), 186 deletions(-)
>

Hi,

this patch has already reached -next, but there is some spurious code.

As, I'm not sure what the real intent is, I prefer to reply here instead
of sending a patch.

[...]

> @@ -817,9 +814,14 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
>
> v9ses = v9fs_inode2v9ses(dir);
> perm = unixmode2p9mode(v9ses, mode);
> - fid = v9fs_create(v9ses, dir, dentry, NULL, perm,
> - v9fs_uflags2omode(flags,
> - v9fs_proto_dotu(v9ses)));
> + p9_omode = v9fs_uflags2omode(flags, v9fs_proto_dotu(v9ses));
> +
> + if ((v9ses->cache >= CACHE_WRITEBACK) && (p9_omode & P9_OWRITE)) {
> + p9_omode = (p9_omode & !P9_OWRITE) | P9_ORDWR;

This code looks strange.
P9_OWRITE is 0x01, so !P9_OWRITE is 0.
So the code is equivalent to "p9_omode = P9_ORDWR;"

Is it what is expexted?

Maybe
p9_omode = (p9_omode & ~P9_OWRITE) | P9_ORDWR;
?

> + p9_debug(P9_DEBUG_CACHE,
> + "write-only file with writeback enabled, creating w/ O_RDWR\n");
> + }
> + fid = v9fs_create(v9ses, dir, dentry, NULL, perm, p9_omode);
> if (IS_ERR(fid)) {
> err = PTR_ERR(fid);
> goto error;

[...]

> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index a28eb3aeab29..4b9488cb7a56 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -232,12 +232,12 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
> int err = 0;
> kgid_t gid;
> umode_t mode;
> + int p9_omode = v9fs_open_to_dotl_flags(flags);
> const unsigned char *name = NULL;
> struct p9_qid qid;
> struct inode *inode;
> struct p9_fid *fid = NULL;
> - struct v9fs_inode *v9inode;
> - struct p9_fid *dfid = NULL, *ofid = NULL, *inode_fid = NULL;
> + struct p9_fid *dfid = NULL, *ofid = NULL;
> struct v9fs_session_info *v9ses;
> struct posix_acl *pacl = NULL, *dacl = NULL;
> struct dentry *res = NULL;
> @@ -282,14 +282,19 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
> /* Update mode based on ACL value */
> err = v9fs_acl_mode(dir, &mode, &dacl, &pacl);
> if (err) {
> - p9_debug(P9_DEBUG_VFS, "Failed to get acl values in creat %d\n",
> + p9_debug(P9_DEBUG_VFS, "Failed to get acl values in create %d\n",
> err);
> goto out;
> }
> - err = p9_client_create_dotl(ofid, name, v9fs_open_to_dotl_flags(flags),
> - mode, gid, &qid);
> +
> + if ((v9ses->cache >= CACHE_WRITEBACK) && (p9_omode & P9_OWRITE)) {
> + p9_omode = (p9_omode & !P9_OWRITE) | P9_ORDWR;

Same here.

CJ

> + p9_debug(P9_DEBUG_CACHE,
> + "write-only file with writeback enabled, creating w/ O_RDWR\n");
> + }
> + err = p9_client_create_dotl(ofid, name, p9_omode, mode, gid, &qid);
> if (err < 0) {
> - p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in creat %d\n",
> + p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in create %d\n",
> err);
> goto out;
> }