Messages in this thread Patch in this message | | | Date | Tue, 25 Apr 2023 09:11:01 +0200 | Subject | Re: [PATCH v5] fs/9p: remove writeback fid and fix per-file modes | From | Christophe JAILLET <> |
| |
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; > }
| |