From:   Christian Schoenebeck <linux_oss@crudebyte.com>
To:     Eric Van Hensbergen <ericvh@kernel.org>, asmadeus@codewreck.org
Cc:     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 20:58:08 +0100
Message-ID: <1983433.kCcYWV5373@silver>
In-Reply-To: <Y/Ch8o/6HVS8Iyeh@codewreck.org>
References: <20230124023834.106339-1-ericvh@kernel.org>
 <20230218003323.2322580-11-ericvh@kernel.org>
 <Y/Ch8o/6HVS8Iyeh@codewreck.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.