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

[Chi Zhiling]

On 2025/1/10 07:28, Dave Chinner wrote:
> On Wed, Jan 08, 2025 at 09:35:47AM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 08, 2025 at 03:43:04PM +0800, Chi Zhiling wrote:
> > > On 2025/1/7 20:13, Amir Goldstein wrote:
> > > > Dave's answer to this question was that there are some legacy applications
> > > > (database applications IIRC) on production systems that do rely on the fact
> > > > that xfs provides this semantics and on the prerequisite that they run on xfs.
> > > >
> > > > However, it was noted that:
> > > > 1. Those application do not require atomicity for any size of IO, they
> > > >       typically work in I/O size that is larger than block size (e.g. 16K or 64K)
> > > >       and they only require no torn writes for this I/O size
> > > > 2. Large folios and iomap can usually provide this semantics via folio lock,
> > > >       but application has currently no way of knowing if the semantics are
> > > >       provided or not
> > >
> > > To be honest, it would be best if the folio lock could provide such
> > > semantics, as it would not cause any potential problems for the
> > > application, and we have hope to achieve concurrent writes.
> > >
> > > However, I am not sure if this is easy to implement and will not cause
> > > other problems.
> >
> > Assuming we're not abandoning POSIX "Thread Interactions with Regular
> > File Operations", you can't use the folio lock for coordination, for
> > several reasons:
> >
> > a) Apps can't directly control the size of the folio in the page cache
> >
> > b) The folio size can (theoretically) change underneath the program at
> > any time (reclaim can take your large folio and the next read gets a
> > smaller folio)
> >
> > c) If your write crosses folios, you've just crossed a synchronization
> > boundary and all bets are off, though all the other filesystems behave
> > this way and there seem not to be complaints
> >
> > d) If you try to "guarantee" folio granularity by messing with min/max
> > folio size, you run the risk of ENOMEM if the base pages get fragmented
> >
> > I think that's why Dave suggested range locks as the correct solution to
> > this; though it is a pity that so far nobody has come up with a
> > performant implementation.
>
> Yes, that's a fair summary of the situation.
>
> That said, I just had a left-field idea for a quasi-range lock
> that may allow random writes to run concurrently and atomically
> with reads.
>
> Essentially, we add an unsigned long to the inode, and use it as a
> lock bitmap. That gives up to 64 "lock segments" for the buffered
> write. We may also need a "segment size" variable....
>
> The existing i_rwsem gets taken shared unless it is an extending
> write.
>
> For a non-extending write, we then do an offset->segment translation
> and lock that bit in the bit mask. If it's already locked, we wait
> on the lock bit. i.e. shared IOLOCK, exclusive write bit lock.
>
> The segments are evenly sized - say a minimum of 64kB each, but when
> EOF is extended or truncated (which is done with the i_rwsem held
> exclusive) the segment size is rescaled. As nothing can hold bit
> locks while the i_rwsem is held exclusive, this will not race with
> anything.
>
> If we are doing an extending write, we take the i_rwsem shared
> first, then check if the extension will rescale the locks. If lock
> rescaling is needed, we have to take the i_rwsem exclusive to do the
> EOF extension. Otherwise, the bit lock that covers EOF will
> serialise file extensions so it can be done under a shared i_rwsem
> safely.
>
> This will allow buffered writes to remain atomic w.r.t. each other,
> and potentially allow buffered reads to wait on writes to the same
> segment and so potentially provide buffered read vs buffered write
> atomicity as well.
>
> If we need more concurrency than an unsigned long worth of bits for
> buffered writes, then maybe we can enlarge the bitmap further.
>
> I suspect this can be extended to direct IO in a similar way to
> buffered reads, and that then opens up the possibility of truncate
> and fallocate() being able to use the bitmap for range exclusion,
> too.
>
> The overhead is likely minimal - setting and clearing bits in a
> bitmap, as opposed to tracking ranges in a tree structure....
>
> Thoughts?

I think it's fine. Additionally, even if multiple writes occur
in the same segment, if the write operations are within a single
page, we can still acquire the i_rwsem lock in shared mode,
right?