From: Eric Van Hensbergen <ericvh@kernel.org>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: v9fs-developer@lists.sourceforge.net, asmadeus@codewreck.org,
linux_oss@crudebyte.com, v9fs@lists.linux.dev
Subject: Re: [PATCH v5] fs/9p: remove writeback fid and fix per-file modes
Date: Wed, 26 Apr 2023 09:45:17 -0700 [thread overview]
Message-ID: <CAFkjPTmzvxV1O0kwg-wVg9z_WPZnuOL_qEmeja3+SH2yFb+25w@mail.gmail.com> (raw)
In-Reply-To: <CAFkjPT=-EvCf1HKT2-k73G4SVBwRDp=YtvfwhNAcjv6BzS4f9Q@mail.gmail.com>
going back over the patch series I realized that this same bitops
problem was in vfs_file and that's the fix I remember doing, but it
didn't propagate to the atomic open variants. Thanks again for
finding this Christophe.
-eric
On Tue, Apr 25, 2023 at 5:01 PM Eric Van Hensbergen <ericvh@kernel.org> wrote:
>
> I swear I fixed that, must have been one of my fixes got dropped in
> the process of churning over this patch. I'm quite concerned that
> this is coming up during the merge window because I'd really rather
> not punt this patch series another two months. I'm going to apply the
> fix as an additional patch which hopefully Linus will accept with the
> rest of the series.
>
> On Tue, Apr 25, 2023 at 12:11 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > 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;
> > > }
> >
parent reply other threads:[~2023-04-26 16:45 UTC|newest]
Thread overview: expand[flat|nested] mbox.gz Atom feed
[parent not found: <CAFkjPT=-EvCf1HKT2-k73G4SVBwRDp=YtvfwhNAcjv6BzS4f9Q@mail.gmail.com>]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFkjPTmzvxV1O0kwg-wVg9z_WPZnuOL_qEmeja3+SH2yFb+25w@mail.gmail.com \
--to=ericvh@kernel.org \
--cc=asmadeus@codewreck.org \
--cc=christophe.jaillet@wanadoo.fr \
--cc=linux_oss@crudebyte.com \
--cc=v9fs-developer@lists.sourceforge.net \
--cc=v9fs@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).