Message-ID: <Z4rXY2-fx_59nywH@dread.disaster.area>
Date: Sat, 18 Jan 2025 09:19:15 +1100
From: Dave Chinner <david@...morbit.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Christoph Hellwig <hch@...radead.org>,
	Brian Foster <bfoster@...hat.com>,
	"Darrick J. Wong" <djwong@...nel.org>,
	Chi Zhiling <chizhiling@....com>, cem@...nel.org,
	linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	Chi Zhiling <chizhiling@...inos.cn>,
	John Garry <john.g.garry@...cle.com>
Subject: Re: [PATCH] xfs: Remove i_rwsem lock in buffered read

On Fri, Jan 17, 2025 at 02:27:46PM +0100, Amir Goldstein wrote:
> On Wed, Jan 15, 2025 at 10:41 PM Dave Chinner <david@...morbit.com> wrote:
> >
> > On Tue, Jan 14, 2025 at 09:55:21PM -0800, Christoph Hellwig wrote:
> > > On Mon, Jan 13, 2025 at 08:40:51AM -0500, Brian Foster wrote:
> > > > Sorry if this is out of left field as I haven't followed the discussion
> > > > closely, but I presumed one of the reasons Darrick and Christoph raised
> > > > the idea of using the folio batch thing I'm playing around with on zero
> > > > range for buffered writes would be to acquire and lock all targeted
> > > > folios up front. If so, would that help with what you're trying to
> > > > achieve here? (If not, nothing to see here, move along.. ;).
> > >
> > > I mostly thought about acquiring, as locking doesn't really have much
> > > batching effects.  That being said, no that you got the idea in my mind
> > > here's my early morning brainfart on it:
> > >
> > > Let's ignore DIRECT I/O for the first step.  In that case lookup /
> > > allocation and locking all folios for write before copying data will
> > > remove the need for i_rwsem in the read and write path.  In a way that
> > > sounds perfect, and given that btrfs already does that (although in a
> > > very convoluted way) we know it's possible.
> >
> > Yes, this seems like a sane, general approach to allowing concurrent
> > buffered writes (and reads).
> >
> > > But direct I/O throws a big monkey wrench here as already mentioned by
> > > others.  Now one interesting thing some file systems have done is
> > > to serialize buffered against direct I/O, either by waiting for one
> > > to finish, or by simply forcing buffered I/O when direct I/O would
> > > conflict.
> >
> > Right. We really don't want to downgrade to buffered IO if we can
> > help it, though.
> >
> > > It's easy to detect outstanding direct I/O using i_dio_count
> > > so buffered I/O could wait for that, and downgrading to buffered I/O
> > > (potentially using the new uncached mode from Jens) if there are any
> > > pages on the mapping after the invalidation also sounds pretty doable.
> >
> > It's much harder to sanely serialise DIO against buffered writes
> > this way, because i_dio_count only forms a submission barrier in
> > conjunction with the i_rwsem being held exclusively. e.g. ongoing
> > DIO would result in the buffered write being indefinitely delayed.
> 
> Isn't this already the case today with EX vs. SH iolock?

No. We do not hold the i_rwsem across async DIO read or write, so
we can have DIO in flight whilst a buffered write grabs and holds
the i_rwsem exclusive. This is the problem that i_dio_count solves
for truncate, etc. i.e. the only way to actually ensure there is no
DIO in flight is to grab the i_rwsem exclusive and then call
inode_dio_wait() to ensure all async DIO in flight completes before
continuing.

> I guess the answer depends whether or not i_rwsem
> starves existing writers in the face of ongoing new readers.

It does not. If the lock is held for read and there is a pending
write, new readers are queued behind the pending write waiters
(see rwsem_down_read_slowpath(), which is triggered if there are
pending waiters on an attempt to read lock).

> > > I don't really have time to turn this hand waving into, but maybe we
> > > should think if it's worthwhile or if I'm missing something important.
> >
> > If people are OK with XFS moving to exclusive buffered or DIO
> > submission model, then I can find some time to work on the
> > converting the IO path locking to use a two-state shared lock in
> > preparation for the batched folio stuff that will allow concurrent
> > buffered writes...
> 
> I won't object to getting the best of all worlds, but I have to say,
> upfront, this sounds a bit like premature optimization and for
> a workload (mixed buffered/dio write)

Say what? The dio/buffered exclusion change provides a different
data coherency and correctness model that is required to then
optimising the workload you care about - mixed buffered read/write.

This change doesn't optimise either DIO or buffered IO, nor is a
shared-exclusive BIO/DIO lock isn't going to improve performance of
such mixed IO workloads in any way.  IOWs, it's pretty hard to call
a locking model change like this "optimisation", let alone call it
"premature".

> that I don't think anybody
> does in practice and nobody should care how it performs.
> Am I wrong?

Yes and no. mixed buffered/direct IO worklaods are more common than
you think. e.g. many backup programs use direct IO and so inherently
mix DIO with buffered IO for the majority of files the backup
program touches. However, all we care about in this case is data
coherency, not performance, and this change should improve data
coherency between DIO and buffered IO...

> For all practical purposes, we could maintain a counter in inode
> not for submitted DIO, but for files opened O_DIRECT.
> 
> The first open O_DIRECT could serialize with in-flight
> buffered writes holding a shared iolock and then buffered writes
> could take SH vs. EX iolock depending on folio state and on
> i_dio_open_count.

I don't see how this can be made to work w.r.t. sane data coherency
behaviour. e.g. how does this model serialise a new DIO write that
is submitted after a share-locked buffered write has just started
and passed all the "i_dio_count == 0" checks that enable it to use
shared locking? i.e. we now have potentially overlapping buffered
writes and DIO writes being done concurrently because the buffered
write may not have instantiated folios in the page cache yet....

> I would personally prefer a simple solution that is good enough
> and has a higher likelihood for allocating the development, review
> and testing resources that are needed to bring it to the finish line.

Have you looked at how simple the bcachefs buffered/dio exclusion
implementation is? The lock mechanism itself is about 50 lines of
code, and there are only 4 or 5 places where the lock is actually
needed. It doesn't get much simpler than that.

And, quite frankly, the fact the bcachefs solution also covers AIO
DIO in flight (which i_rwsem based locking does not!) means it is a
more robust solution than trying to rely on racy i_dio_count hacks
and folio residency in the page cache...

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

Powered by blists - more mailing lists