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

From: asmadeus@codewreck.org
To: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Christian Schoenebeck <linux_oss@crudebyte.com>,
	Eric Van Hensbergen <ericvh@kernel.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: Sun, 19 Feb 2023 08:40:53 +0900	[thread overview]
Message-ID: <Y/FiBbMQcEblQ/XR@codewreck.org> (raw)
In-Reply-To: <CAFkjPT=xhEEedeYcyn1FFcngqOJf_+8ynz4zeLbsXPOGoY6aqw@mail.gmail.com>

Eric Van Hensbergen wrote on Sat, Feb 18, 2023 at 04:24:08PM -0600:
> 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.

For history (since I implemented it for CEA back then), mmap was added
because we had applications relying on mmap (so wanted that to somehow
work) without turning to cache=loose as that doesn't behave well with
nfs-ganesha (in particular, fids not closed until memory pressure comes
reclaiming them, which can be very late or never come, doesn't work well
with ganesha that used to (and probably still does, didn't check) cap
the maximum number of fid active per client.

I think writeback would be acceptable for this usecase, especially since
with your patches we now flush on close.

For clarity though I'd use writeback in the documentation, and keep mmap
as a legacy mapping just for mount opts parsing; the behaviour is
slightly different than it used to be (normal read/writes were sync) so
it's good to be clear about that.

> 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.

Separating the two makes sense implementation-wise as well, I like this
idea.
What would the difference be between file_cache=writeback and loose?
Do you plan some form of revalidation with writeback, e.g. using qid
version that loose wouldn't do? (sorry if it's already done, I don't
recall seeing that)

fscache is currently a cache option but it's pretty much unrelated, we
can have it as a separate option and alias cache=fscache to
`file_cache=writeback(loose),dir_cache=loose,fscache=on`
but on its own it ought to work with any level of file_cache and no
dir_cache...
The test matrix will be fun, though :|

> 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.

xattr caching currently isn't done at all afaik, and it'd definitely
make sense with any kind of dir_cache... That'd likely halve the
requests for some find-like workloads.
Probably another new option as well..

-- 
Dominique