Message-ID: <aBK2HAnoRacuO0CO@dread.disaster.area>
Date: Thu, 1 May 2025 09:45:32 +1000
From: Dave Chinner <david@...morbit.com>
To: Chi Zhiling <chizhiling@....com>
Cc: cem@...nel.org, linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	Chi Zhiling <chizhiling@...inos.cn>
Subject: Re: [RFC PATCH 0/2] Implement concurrent buffered write with folio
 lock

On Wed, Apr 30, 2025 at 05:03:51PM +0800, Chi Zhiling wrote:
> On 2025/4/30 10:05, Dave Chinner wrote:
> > On Fri, Apr 25, 2025 at 06:38:39PM +0800, Chi Zhiling wrote:
> > > From: Chi Zhiling <chizhiling@...inos.cn>
> > > 
> > > This is a patch attempting to implement concurrent buffered writes.
> > > The main idea is to use the folio lock to ensure the atomicity of the
> > > write when writing to a single folio, instead of using the i_rwsem.
> > > 
> > > I tried the "folio batch" solution, which is a great idea, but during
> > > testing, I encountered an OOM issue because the locked folios couldn't
> > > be reclaimed.
> > > 
> > > So for now, I can only allow concurrent writes within a single block.
> > > The good news is that since we already support BS > PS, we can use a
> > > larger block size to enable higher granularity concurrency.
> > 
> > I'm not going to say no to this, but I think it's a short term and
> > niche solution to the general problem of enabling shared buffered
> > writes. i.e. I expect that it will not exist for long, whilst
> 
> Hi, Dave,
> 
> Yes, it's a short-term solution, but it's enough for some scenarios.
> I would also like to see better idea.
> 
> > experience tells me that adding special cases to the IO path locking
> > has a fairly high risk of unexpected regressions and/or data
> > corruption....
> 
> I can't say there is definitely no data corruption, but I haven't seen
> any new errors in xfstests.

Yeah, that's why they are "unexpected regressions" - testing looks
fine, but once it gets out into complex production workloads....

> We might need to add some assertions in the code to check for the risk
> of data corruption, not specifically for this patch, but for the current
> XFS system in general. This would help developers avoid introducing new
> bugs, similar to the lockdep tool.

I'm not sure what you invisage here or what problems you think we
might be able to catch - can you describe what you are thinking
about here?

> > > These ideas come from previous discussions:
> > > https://lore.kernel.org/all/953b0499-5832-49dc-8580-436cf625db8c@163.com/
> > 
> > In my spare time I've been looking at using the two state lock from
> > bcachefs for this because it looks to provide a general solution to
> > the issue of concurrent buffered writes.
> 
> In fact, I have tried the two state lock, and it does work quite well.
> However, I noticed some performance degradation in single-threaded
> scenarios in UnixBench (I'm not sure if it's caused by the memory
> barrier).

Please share the patch - I'd like to see how you implemented it and
how you solved the various lock ordering and wider IO serialisation
issues. It may be that I've overlooked something and your
implementation makes me aware of it. OTOH, I might see something in
your patch that could be improved that mitigates the regression.

> Since single-threaded bufferedio is still the primary read-write mode,
> I don't want to introduce too much impact in single-threaded scenarios.

I mostly don't care that much about small single threaded
performance regressions anywhere in XFS if there is some upside for
scalability or performance. We've always traded off single threaded
performance for better concurrency and/or scalability in XFS (right
from the initial design choices way back in the early 1990s), so I
don't see why we'd treat a significant improvement in buffered IO
concurrency any differently.

-Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists