Re: [PATCH v4 10/11] fs/9p: writeback mode fixes

From: Eric Van Hensbergen <ericvh@gmail.com>
To: Christian Schoenebeck <linux_oss@crudebyte.com>
Cc: Eric Van Hensbergen <ericvh@kernel.org>,
	asmadeus@codewreck.org, v9fs-developer@lists.sourceforge.net,
	rminnich@gmail.com, lucho@ionkov.net,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v4 10/11] fs/9p: writeback mode fixes
Date: Sat, 18 Feb 2023 16:24:08 -0600	[thread overview]
Message-ID: <CAFkjPT=xhEEedeYcyn1FFcngqOJf_+8ynz4zeLbsXPOGoY6aqw@mail.gmail.com> (raw)
In-Reply-To: <1983433.kCcYWV5373@silver>

Yeah, I guess it depends on what options we want to separate,
writeback == mmap so we can eliminate one option and just use mmap I
suppose.  I feel like readahead has value as it maintains the most
consistency on the host file system since it shouldn't be doing any
writeback buffering.  readahead and mmap are different than loose in
that they don't do any do any dir cache.  To your earlier comments (in
a different thread) it very well may be that eventually we separate
these into file_cache=[ readahead | mmap | loose ] and dir_cache = [
tight | temporal | loose ] and fscache is its own beast.  It struck me
as well with xattr enabled we may want to have separate caches for
xattr caching since it generates a load of traffic with security on.

On Sat, Feb 18, 2023 at 1:58 PM Christian Schoenebeck
<linux_oss@crudebyte.com> wrote:
>
> On Saturday, February 18, 2023 11:01:22 AM CET asmadeus@codewreck.org wrote:
> > 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@kernel.org>
> >
> > 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...)
>
> +1 on docs
>
> The question was also whether to make these true separate options before being
> merged.
>
> I give these patches a spin tomorrow.
>
>
>