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

From: Eric Van Hensbergen <ericvh@kernel.org>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: v9fs-developer@lists.sourceforge.net, asmadeus@codewreck.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux_oss@crudebyte.com
Subject: Re: [PATCH v5] fs/9p: remove writeback fid and fix per-file modes
Date: Tue, 25 Apr 2023 17:01:03 -0700	[thread overview]
Message-ID: <CAFkjPT=-EvCf1HKT2-k73G4SVBwRDp=YtvfwhNAcjv6BzS4f9Q@mail.gmail.com> (raw)
In-Reply-To: <7686c810-4ed6-9e3a-3714-8b803e2d3c46@wanadoo.fr>

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;
> >       }
>