Re: [PATCH] xfs: Remove i_rwsem lock in buffered read

[Christoph Hellwig]

On Thu, Jan 16, 2025 at 08:41:21AM +1100, Dave Chinner wrote:
> > 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.

Of course we never want it.  But if we do have invalidation failures
and thus still folios in the page cache it might the least bad of
all the bad options.

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

Or any other exclusive lock taken by all submitters, yes.

> I think the model and method that bcachefs uses is probably the best
> way to move forward - the "two-state exclusive shared" lock which it
> uses to do buffered vs direct exclusion is a simple, easy way to
> handle this problem. The same-state shared locking fast path is a
> single atomic cmpxchg operation, so it has neglible extra overhead
> compared to using a rwsem in the shared DIO fast path.

NFS and ocfs2 have been doing this for about two decades as well.

> This only leaves DIO coherency issues with mmap() based IO as an
> issue, but that's a problem for a different day...

I think it's a generally unsolveable problem that we can just whack
enough to make it good enough in practice for the few workloads that
matter.

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

This does sound fine to me, but it's hard to judge without seeing
a prototype and results based on it.