[PATCH] xfs: Remove i_rwsem lock in buffered read

[PATCH] xfs: Remove i_rwsem lock in buffered read

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

Using an rwsem to protect file data ensures that we can always obtain a
completed modification. But due to the lock, we need to wait for the
write process to release the rwsem before we can read it, even if we are
reading a different region of the file. This could take a lot of time
when many processes need to write and read this file.

On the other hand, The ext4 filesystem and others do not hold the lock
during buffered reading, which make the ext4 have better performance in
that case. Therefore, I think it will be fine if we remove the lock in
xfs, as most applications can handle this situation.

Without this lock, we achieve a great improvement when multi-threaded
reading and writing. Use the following command to test:
fio -ioengine=libaio -filename=testfile -bs=4k -rw=randrw -numjobs=16 -name="randrw"

Before this patch:
   READ: bw=351MiB/s (368MB/s), 21.8MiB/s-22.0MiB/s (22.9MB/s-23.1MB/s), io=8185MiB (8582MB), run=23206-23347msec
  WRITE: bw=351MiB/s (368MB/s), 21.9MiB/s-22.1MiB/s (23.0MB/s-23.2MB/s), io=8199MiB (8597MB), run=23206-23347msec

After this patch:
   READ: bw=1961MiB/s (2056MB/s), 122MiB/s-125MiB/s (128MB/s-131MB/s), io=8185MiB (8582MB), run=4097-4174msec
  WRITE: bw=1964MiB/s (2060MB/s), 123MiB/s-125MiB/s (129MB/s-131MB/s), io=8199MiB (8597MB), run=4097-4174msec

Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
---
 fs/xfs/xfs_file.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9a435b1ff264..7d039cc3ae9e 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -279,18 +279,9 @@ xfs_file_buffered_read(
 	struct kiocb		*iocb,
 	struct iov_iter		*to)
 {
-	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
-	ssize_t			ret;
-
 	trace_xfs_file_buffered_read(iocb, to);
 
-	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
-	if (ret)
-		return ret;
-	ret = generic_file_read_iter(iocb, to);
-	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
-
-	return ret;
+	return generic_file_read_iter(iocb, to);
 }
 
 STATIC ssize_t
-- 
2.43.0

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

[Dave Chinner]

On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> From: Chi Zhiling <chizhiling@kylinos.cn>
> 
> Using an rwsem to protect file data ensures that we can always obtain a
> completed modification. But due to the lock, we need to wait for the
> write process to release the rwsem before we can read it, even if we are
> reading a different region of the file. This could take a lot of time
> when many processes need to write and read this file.
> 
> On the other hand, The ext4 filesystem and others do not hold the lock
> during buffered reading, which make the ext4 have better performance in
> that case. Therefore, I think it will be fine if we remove the lock in
> xfs, as most applications can handle this situation.

Nope.

This means that XFS loses high level serialisation of incoming IO
against operations like truncate, fallocate, pnfs operations, etc.

We've been through this multiple times before; the solution lies in
doing the work to make buffered writes use shared locking, not
removing shared locking from buffered reads.

A couple of old discussions from the list:

https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
https://lore.kernel.org/linux-xfs/20190404165737.30889-1-amir73il@gmail.com/

There are likely others - you can search for them yourself to get
more background information.

Fundamentally, though, removing locking from the read side is not
the answer to this buffered write IO exclusion problem....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Chi Zhiling]

On 2024/12/27 05:50, Dave Chinner wrote:
> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>
>> Using an rwsem to protect file data ensures that we can always obtain a
>> completed modification. But due to the lock, we need to wait for the
>> write process to release the rwsem before we can read it, even if we are
>> reading a different region of the file. This could take a lot of time
>> when many processes need to write and read this file.
>>
>> On the other hand, The ext4 filesystem and others do not hold the lock
>> during buffered reading, which make the ext4 have better performance in
>> that case. Therefore, I think it will be fine if we remove the lock in
>> xfs, as most applications can handle this situation.
> 
> Nope.
> 
> This means that XFS loses high level serialisation of incoming IO
> against operations like truncate, fallocate, pnfs operations, etc.
> 
> We've been through this multiple times before; the solution lies in
> doing the work to make buffered writes use shared locking, not
> removing shared locking from buffered reads.

You mean using shared locking for buffered reads and writes, right?

I think it's a great idea. In theory, write operations can be performed
simultaneously if they write to different ranges.

So we should track all the ranges we are reading or writing,
and check whether the new read or write operations can be performed
concurrently with the current operations.

Do we have any plans to use shared locking for buffered writes?


> 
> A couple of old discussions from the list:
> 
> https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> https://lore.kernel.org/linux-xfs/20190404165737.30889-1-amir73il@gmail.com/
> 
> There are likely others - you can search for them yourself to get
> more background information.

Sorry, I didn't find those discussions earlier.

> 
> Fundamentally, though, removing locking from the read side is not
> the answer to this buffered write IO exclusion problem....
> 
> -Dave.

Best regards,
Chi Zhiling

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

[Dave Chinner]

On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> 
> 
> On 2024/12/27 05:50, Dave Chinner wrote:
> > On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> > > From: Chi Zhiling <chizhiling@kylinos.cn>
> > > 
> > > Using an rwsem to protect file data ensures that we can always obtain a
> > > completed modification. But due to the lock, we need to wait for the
> > > write process to release the rwsem before we can read it, even if we are
> > > reading a different region of the file. This could take a lot of time
> > > when many processes need to write and read this file.
> > > 
> > > On the other hand, The ext4 filesystem and others do not hold the lock
> > > during buffered reading, which make the ext4 have better performance in
> > > that case. Therefore, I think it will be fine if we remove the lock in
> > > xfs, as most applications can handle this situation.
> > 
> > Nope.
> > 
> > This means that XFS loses high level serialisation of incoming IO
> > against operations like truncate, fallocate, pnfs operations, etc.
> > 
> > We've been through this multiple times before; the solution lies in
> > doing the work to make buffered writes use shared locking, not
> > removing shared locking from buffered reads.
> 
> You mean using shared locking for buffered reads and writes, right?
> 
> I think it's a great idea. In theory, write operations can be performed
> simultaneously if they write to different ranges.

Even if they overlap, the folio locks will prevent concurrent writes
to the same range.

Now that we have atomic write support as native functionality (i.e.
RWF_ATOMIC), we really should not have to care that much about
normal buffered IO being atomic. i.e. if the application wants
atomic writes, it can now specify that it wants atomic writes and so
we can relax the constraints we have on existing IO...

> So we should track all the ranges we are reading or writing,
> and check whether the new read or write operations can be performed
> concurrently with the current operations.

That is all discussed in detail in the discussions I linked.

> Do we have any plans to use shared locking for buffered writes?

All we are waiting for is someone to put the resources into making
the changes and testing it properly...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Chi Zhiling]

On 2024/12/29 06:17, Dave Chinner wrote:
> On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
>>
>>
>> On 2024/12/27 05:50, Dave Chinner wrote:
>>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
>>>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>>>
>>>> Using an rwsem to protect file data ensures that we can always obtain a
>>>> completed modification. But due to the lock, we need to wait for the
>>>> write process to release the rwsem before we can read it, even if we are
>>>> reading a different region of the file. This could take a lot of time
>>>> when many processes need to write and read this file.
>>>>
>>>> On the other hand, The ext4 filesystem and others do not hold the lock
>>>> during buffered reading, which make the ext4 have better performance in
>>>> that case. Therefore, I think it will be fine if we remove the lock in
>>>> xfs, as most applications can handle this situation.
>>>
>>> Nope.
>>>
>>> This means that XFS loses high level serialisation of incoming IO
>>> against operations like truncate, fallocate, pnfs operations, etc.
>>>
>>> We've been through this multiple times before; the solution lies in
>>> doing the work to make buffered writes use shared locking, not
>>> removing shared locking from buffered reads.
>>
>> You mean using shared locking for buffered reads and writes, right?
>>
>> I think it's a great idea. In theory, write operations can be performed
>> simultaneously if they write to different ranges.
> 
> Even if they overlap, the folio locks will prevent concurrent writes
> to the same range.
> 
> Now that we have atomic write support as native functionality (i.e.
> RWF_ATOMIC), we really should not have to care that much about
> normal buffered IO being atomic. i.e. if the application wants
> atomic writes, it can now specify that it wants atomic writes and so
> we can relax the constraints we have on existing IO...

Yes, I'm not particularly concerned about whether buffered I/O is 
atomic. I'm more concerned about the multithreading performance of 
buffered I/O.

Last week, it was mentioned that removing i_rwsem would have some 
impacts on truncate, fallocate, and PNFS operations.

(I'm not familiar with pNFS, so please correct me if I'm wrong.)

My understanding is that the current i_rwsem is used to protect both
the file's data and its size. Operations like truncate, fallocate,
and PNFS use i_rwsem because they modify both the file's data and its 
size. So, I'm thinking whether it's possible to use i_rwsem to protect 
only the file's size, without protecting the file's data.

So operations that modify the file's size need to be executed 
sequentially. For example, buffered writes to the EOF, fallocate 
operations without the "keep size" requirement, and truncate operations, 
etc, all need to hold an exclusive lock.

Other operations require a shared lock because they only need to access
the file's size without modifying it.

> 
>> So we should track all the ranges we are reading or writing,
>> and check whether the new read or write operations can be performed
>> concurrently with the current operations.
> 
> That is all discussed in detail in the discussions I linked.

Sorry, I overlooked some details from old discussion last time.
It seems that you are not satisfied with the effectiveness of
range locks.


Best regards,
Chi Zhiling

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

[Amir Goldstein]

On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
>
>
>
> On 2024/12/29 06:17, Dave Chinner wrote:
> > On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> >>
> >>
> >> On 2024/12/27 05:50, Dave Chinner wrote:
> >>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> >>>> From: Chi Zhiling <chizhiling@kylinos.cn>
> >>>>
> >>>> Using an rwsem to protect file data ensures that we can always obtain a
> >>>> completed modification. But due to the lock, we need to wait for the
> >>>> write process to release the rwsem before we can read it, even if we are
> >>>> reading a different region of the file. This could take a lot of time
> >>>> when many processes need to write and read this file.
> >>>>
> >>>> On the other hand, The ext4 filesystem and others do not hold the lock
> >>>> during buffered reading, which make the ext4 have better performance in
> >>>> that case. Therefore, I think it will be fine if we remove the lock in
> >>>> xfs, as most applications can handle this situation.
> >>>
> >>> Nope.
> >>>
> >>> This means that XFS loses high level serialisation of incoming IO
> >>> against operations like truncate, fallocate, pnfs operations, etc.
> >>>
> >>> We've been through this multiple times before; the solution lies in
> >>> doing the work to make buffered writes use shared locking, not
> >>> removing shared locking from buffered reads.
> >>
> >> You mean using shared locking for buffered reads and writes, right?
> >>
> >> I think it's a great idea. In theory, write operations can be performed
> >> simultaneously if they write to different ranges.
> >
> > Even if they overlap, the folio locks will prevent concurrent writes
> > to the same range.
> >
> > Now that we have atomic write support as native functionality (i.e.
> > RWF_ATOMIC), we really should not have to care that much about
> > normal buffered IO being atomic. i.e. if the application wants
> > atomic writes, it can now specify that it wants atomic writes and so
> > we can relax the constraints we have on existing IO...
>
> Yes, I'm not particularly concerned about whether buffered I/O is
> atomic. I'm more concerned about the multithreading performance of
> buffered I/O.

Hi Chi,

Sorry for joining late, I was on vacation.
I am very happy that you have taken on this task and your timing is good,
because  John Garry just posted his patches for large atomic writes [1].

I need to explain the relation to atomic buffered I/O, because it is not
easy to follow it from the old discussions and also some of the discussions
about the solution were held in-person during LSFMM2024.

Naturally, your interest is improving multithreading performance of
buffered I/O, so was mine when I first posted this question [2].

The issue with atomicity of buffered I/O is the xfs has traditionally
provided atomicity of write vs. read (a.k.a no torn writes), which is
not required by POSIX standard (because POSIX was not written with
threads in mind) and is not respected by any other in-tree filesystem.

It is obvious that the exclusive rw lock for buffered write hurts performance
in the case of mixed read/write on xfs, so the question was - if xfs provides
atomic semantics that portable applications cannot rely on, why bother
providing these atomic semantics at all?

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
3. The upcoming ability of application to opt-in for atomic writes larger
    than fs block size [1] can be used to facilitate the applications that
    want to legacy xfs semantics and avoid the need to enforce the legacy
    semantics all the time for no good reason

Disclaimer: this is not a standard way to deal with potentially breaking
legacy semantics, because old applications will not usually be rebuilt
to opt-in for the old semantics, but the use case in hand is probably
so specific, of a specific application that relies on xfs specific semantics
that there are currently no objections for trying this solution.

[1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
[2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/

>
> Last week, it was mentioned that removing i_rwsem would have some
> impacts on truncate, fallocate, and PNFS operations.
>
> (I'm not familiar with pNFS, so please correct me if I'm wrong.)

You are not wrong. pNFS uses a "layout lease", which requires
that the blockmap of the file will not be modified while the lease is held.
but I think that block that are not mapped (i.e. holes) can be mapped
while the lease is held.

>
> My understanding is that the current i_rwsem is used to protect both
> the file's data and its size. Operations like truncate, fallocate,
> and PNFS use i_rwsem because they modify both the file's data and its
> size. So, I'm thinking whether it's possible to use i_rwsem to protect
> only the file's size, without protecting the file's data.
>

It also protects the file's blockmap, for example, punch hole
does not change the size, but it unmaps blocks from the blockmap,
leading to races that could end up reading stale data from disk
if the lock wasn't taken.

> So operations that modify the file's size need to be executed
> sequentially. For example, buffered writes to the EOF, fallocate
> operations without the "keep size" requirement, and truncate operations,
> etc, all need to hold an exclusive lock.
>
> Other operations require a shared lock because they only need to access
> the file's size without modifying it.
>

As far as I understand, exclusive lock is not needed for non-extending
writes, because it is ok to map new blocks.
I guess the need for exclusive lock for extending writes is related to
update of file size, but not 100% sure.
Anyway, exclusive lock on extending write is widely common in other fs,
while exclusive lock for non-extending write is unique to xfs.

> >
> >> So we should track all the ranges we are reading or writing,
> >> and check whether the new read or write operations can be performed
> >> concurrently with the current operations.
> >
> > That is all discussed in detail in the discussions I linked.
>
> Sorry, I overlooked some details from old discussion last time.
> It seems that you are not satisfied with the effectiveness of
> range locks.
>

Correct. This solution was taken off the table.

I hope my explanation was correct and clear.
If anything else is not clear, please feel free to ask.

Thanks,
Amir.

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

[Christoph Hellwig]

On Tue, Jan 07, 2025 at 01:13:17PM +0100, Amir Goldstein wrote:
> The issue with atomicity of buffered I/O is the xfs has traditionally
> provided atomicity of write vs. read (a.k.a no torn writes), which is
> not required by POSIX standard (because POSIX was not written with
> threads in mind) and is not respected by any other in-tree filesystem.

That is true for original Posix, but once Posix Threads joined the game
the behavior was and still is required.  See "2.9.7 Thread Interactions
with Regular File Operations" here:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html

Now most Linux filesystems ignored that and got away with ignoring
the requirement, but it still exists.

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

[Chi Zhiling]

On 2025/1/7 20:13, Amir Goldstein wrote:
> On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
>> On 2024/12/29 06:17, Dave Chinner wrote:
>>> On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
>>>> On 2024/12/27 05:50, Dave Chinner wrote:
>>>>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
>>>>>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>>>>>
>>>>>> Using an rwsem to protect file data ensures that we can always obtain a
>>>>>> completed modification. But due to the lock, we need to wait for the
>>>>>> write process to release the rwsem before we can read it, even if we are
>>>>>> reading a different region of the file. This could take a lot of time
>>>>>> when many processes need to write and read this file.
>>>>>>
>>>>>> On the other hand, The ext4 filesystem and others do not hold the lock
>>>>>> during buffered reading, which make the ext4 have better performance in
>>>>>> that case. Therefore, I think it will be fine if we remove the lock in
>>>>>> xfs, as most applications can handle this situation.
>>>>>
>>>>> Nope.
>>>>>
>>>>> This means that XFS loses high level serialisation of incoming IO
>>>>> against operations like truncate, fallocate, pnfs operations, etc.
>>>>>
>>>>> We've been through this multiple times before; the solution lies in
>>>>> doing the work to make buffered writes use shared locking, not
>>>>> removing shared locking from buffered reads.
>>>>
>>>> You mean using shared locking for buffered reads and writes, right?
>>>>
>>>> I think it's a great idea. In theory, write operations can be performed
>>>> simultaneously if they write to different ranges.
>>>
>>> Even if they overlap, the folio locks will prevent concurrent writes
>>> to the same range.
>>>
>>> Now that we have atomic write support as native functionality (i.e.
>>> RWF_ATOMIC), we really should not have to care that much about
>>> normal buffered IO being atomic. i.e. if the application wants
>>> atomic writes, it can now specify that it wants atomic writes and so
>>> we can relax the constraints we have on existing IO...
>>
>> Yes, I'm not particularly concerned about whether buffered I/O is
>> atomic. I'm more concerned about the multithreading performance of
>> buffered I/O.
> 
> Hi Chi,
> 
> Sorry for joining late, I was on vacation.
> I am very happy that you have taken on this task and your timing is good,
> because  John Garry just posted his patches for large atomic writes [1].

I'm glad to have you on board. :)

I'm not sure if my understanding is correct, but it seems that our
discussion is about multithreading safety, while John's patch is about
providing power-failure safety, even though both mention atomicity.

> 
> I need to explain the relation to atomic buffered I/O, because it is not
> easy to follow it from the old discussions and also some of the discussions
> about the solution were held in-person during LSFMM2024.
> 
> Naturally, your interest is improving multithreading performance of
> buffered I/O, so was mine when I first posted this question [2].
> 
> The issue with atomicity of buffered I/O is the xfs has traditionally
> provided atomicity of write vs. read (a.k.a no torn writes), which is
> not required by POSIX standard (because POSIX was not written with
> threads in mind) and is not respected by any other in-tree filesystem.
> 
> It is obvious that the exclusive rw lock for buffered write hurts performance
> in the case of mixed read/write on xfs, so the question was - if xfs provides
> atomic semantics that portable applications cannot rely on, why bother
> providing these atomic semantics at all?

Perhaps we can add an option that allows distributions or users to
decide whether they need this semantics. I would not hesitate to
turn off this semantics on my system when the time comes.

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

> 3. The upcoming ability of application to opt-in for atomic writes larger
>      than fs block size [1] can be used to facilitate the applications that
>      want to legacy xfs semantics and avoid the need to enforce the legacy
>      semantics all the time for no good reason
> 
> Disclaimer: this is not a standard way to deal with potentially breaking
> legacy semantics, because old applications will not usually be rebuilt
> to opt-in for the old semantics, but the use case in hand is probably
> so specific, of a specific application that relies on xfs specific semantics
> that there are currently no objections for trying this solution.
> 
> [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
> [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> 
>>
>> Last week, it was mentioned that removing i_rwsem would have some
>> impacts on truncate, fallocate, and PNFS operations.
>>
>> (I'm not familiar with pNFS, so please correct me if I'm wrong.)
> 
> You are not wrong. pNFS uses a "layout lease", which requires
> that the blockmap of the file will not be modified while the lease is held.
> but I think that block that are not mapped (i.e. holes) can be mapped
> while the lease is held.
> 
>>
>> My understanding is that the current i_rwsem is used to protect both
>> the file's data and its size. Operations like truncate, fallocate,
>> and PNFS use i_rwsem because they modify both the file's data and its
>> size. So, I'm thinking whether it's possible to use i_rwsem to protect
>> only the file's size, without protecting the file's data.
>>
> 
> It also protects the file's blockmap, for example, punch hole
> does not change the size, but it unmaps blocks from the blockmap,
> leading to races that could end up reading stale data from disk
> if the lock wasn't taken.
> 
>> So operations that modify the file's size need to be executed
>> sequentially. For example, buffered writes to the EOF, fallocate
>> operations without the "keep size" requirement, and truncate operations,
>> etc, all need to hold an exclusive lock.
>>
>> Other operations require a shared lock because they only need to access
>> the file's size without modifying it.
>>
> 
> As far as I understand, exclusive lock is not needed for non-extending
> writes, because it is ok to map new blocks.
> I guess the need for exclusive lock for extending writes is related to
> update of file size, but not 100% sure.
> Anyway, exclusive lock on extending write is widely common in other fs,
> while exclusive lock for non-extending write is unique to xfs.
> 
>>>
>>>> So we should track all the ranges we are reading or writing,
>>>> and check whether the new read or write operations can be performed
>>>> concurrently with the current operations.
>>>
>>> That is all discussed in detail in the discussions I linked.
>>
>> Sorry, I overlooked some details from old discussion last time.
>> It seems that you are not satisfied with the effectiveness of
>> range locks.
>>
> 
> Correct. This solution was taken off the table.
> 
> I hope my explanation was correct and clear.
> If anything else is not clear, please feel free to ask.
> 

I think your explanation is very clear.

Thanks,
Chi Zhiling

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

[Amir Goldstein]

On Wed, Jan 8, 2025 at 8:43 AM Chi Zhiling <chizhiling@163.com> wrote:
>
> On 2025/1/7 20:13, Amir Goldstein wrote:
> > On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
> >> On 2024/12/29 06:17, Dave Chinner wrote:
> >>> On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> >>>> On 2024/12/27 05:50, Dave Chinner wrote:
> >>>>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> >>>>>> From: Chi Zhiling <chizhiling@kylinos.cn>
> >>>>>>
> >>>>>> Using an rwsem to protect file data ensures that we can always obtain a
> >>>>>> completed modification. But due to the lock, we need to wait for the
> >>>>>> write process to release the rwsem before we can read it, even if we are
> >>>>>> reading a different region of the file. This could take a lot of time
> >>>>>> when many processes need to write and read this file.
> >>>>>>
> >>>>>> On the other hand, The ext4 filesystem and others do not hold the lock
> >>>>>> during buffered reading, which make the ext4 have better performance in
> >>>>>> that case. Therefore, I think it will be fine if we remove the lock in
> >>>>>> xfs, as most applications can handle this situation.
> >>>>>
> >>>>> Nope.
> >>>>>
> >>>>> This means that XFS loses high level serialisation of incoming IO
> >>>>> against operations like truncate, fallocate, pnfs operations, etc.
> >>>>>
> >>>>> We've been through this multiple times before; the solution lies in
> >>>>> doing the work to make buffered writes use shared locking, not
> >>>>> removing shared locking from buffered reads.
> >>>>
> >>>> You mean using shared locking for buffered reads and writes, right?
> >>>>
> >>>> I think it's a great idea. In theory, write operations can be performed
> >>>> simultaneously if they write to different ranges.
> >>>
> >>> Even if they overlap, the folio locks will prevent concurrent writes
> >>> to the same range.
> >>>
> >>> Now that we have atomic write support as native functionality (i.e.
> >>> RWF_ATOMIC), we really should not have to care that much about
> >>> normal buffered IO being atomic. i.e. if the application wants
> >>> atomic writes, it can now specify that it wants atomic writes and so
> >>> we can relax the constraints we have on existing IO...
> >>
> >> Yes, I'm not particularly concerned about whether buffered I/O is
> >> atomic. I'm more concerned about the multithreading performance of
> >> buffered I/O.
> >
> > Hi Chi,
> >
> > Sorry for joining late, I was on vacation.
> > I am very happy that you have taken on this task and your timing is good,
> > because  John Garry just posted his patches for large atomic writes [1].
>
> I'm glad to have you on board. :)
>
> I'm not sure if my understanding is correct, but it seems that our
> discussion is about multithreading safety, while John's patch is about
> providing power-failure safety, even though both mention atomicity.
>

You are right about the *motivation* of John's work.
But as a by-product of his work, we get an API to opt-in for
atomic writes and we can piggy back this opt-in API to say
that whoever wants the legacy behavior can use the new API
to get both power failure safety and multithreading safety.

> >
> > I need to explain the relation to atomic buffered I/O, because it is not
> > easy to follow it from the old discussions and also some of the discussions
> > about the solution were held in-person during LSFMM2024.
> >
> > Naturally, your interest is improving multithreading performance of
> > buffered I/O, so was mine when I first posted this question [2].
> >
> > The issue with atomicity of buffered I/O is the xfs has traditionally
> > provided atomicity of write vs. read (a.k.a no torn writes), which is
> > not required by POSIX standard (because POSIX was not written with
> > threads in mind) and is not respected by any other in-tree filesystem.
> >
> > It is obvious that the exclusive rw lock for buffered write hurts performance
> > in the case of mixed read/write on xfs, so the question was - if xfs provides
> > atomic semantics that portable applications cannot rely on, why bother
> > providing these atomic semantics at all?
>
> Perhaps we can add an option that allows distributions or users to
> decide whether they need this semantics. I would not hesitate to
> turn off this semantics on my system when the time comes.
>

Yes, we can, but we do not want to do it unless we have to.
99% of the users do not want the legacy semantics, so it would
be better if they get the best performance without having to opt-in to get it.

> >
> > 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.
>
> > 3. The upcoming ability of application to opt-in for atomic writes larger
> >      than fs block size [1] can be used to facilitate the applications that
> >      want to legacy xfs semantics and avoid the need to enforce the legacy
> >      semantics all the time for no good reason
> >
> > Disclaimer: this is not a standard way to deal with potentially breaking
> > legacy semantics, because old applications will not usually be rebuilt
> > to opt-in for the old semantics, but the use case in hand is probably
> > so specific, of a specific application that relies on xfs specific semantics
> > that there are currently no objections for trying this solution.
> >
> > [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
> > [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> >
> >>
> >> Last week, it was mentioned that removing i_rwsem would have some
> >> impacts on truncate, fallocate, and PNFS operations.
> >>
> >> (I'm not familiar with pNFS, so please correct me if I'm wrong.)
> >
> > You are not wrong. pNFS uses a "layout lease", which requires
> > that the blockmap of the file will not be modified while the lease is held.
> > but I think that block that are not mapped (i.e. holes) can be mapped
> > while the lease is held.
> >
> >>
> >> My understanding is that the current i_rwsem is used to protect both
> >> the file's data and its size. Operations like truncate, fallocate,
> >> and PNFS use i_rwsem because they modify both the file's data and its
> >> size. So, I'm thinking whether it's possible to use i_rwsem to protect
> >> only the file's size, without protecting the file's data.
> >>
> >
> > It also protects the file's blockmap, for example, punch hole
> > does not change the size, but it unmaps blocks from the blockmap,
> > leading to races that could end up reading stale data from disk
> > if the lock wasn't taken.
> >
> >> So operations that modify the file's size need to be executed
> >> sequentially. For example, buffered writes to the EOF, fallocate
> >> operations without the "keep size" requirement, and truncate operations,
> >> etc, all need to hold an exclusive lock.
> >>
> >> Other operations require a shared lock because they only need to access
> >> the file's size without modifying it.
> >>
> >
> > As far as I understand, exclusive lock is not needed for non-extending
> > writes, because it is ok to map new blocks.
> > I guess the need for exclusive lock for extending writes is related to
> > update of file size, but not 100% sure.
> > Anyway, exclusive lock on extending write is widely common in other fs,
> > while exclusive lock for non-extending write is unique to xfs.
> >
> >>>
> >>>> So we should track all the ranges we are reading or writing,
> >>>> and check whether the new read or write operations can be performed
> >>>> concurrently with the current operations.
> >>>
> >>> That is all discussed in detail in the discussions I linked.
> >>
> >> Sorry, I overlooked some details from old discussion last time.
> >> It seems that you are not satisfied with the effectiveness of
> >> range locks.
> >>
> >
> > Correct. This solution was taken off the table.
> >
> > I hope my explanation was correct and clear.
> > If anything else is not clear, please feel free to ask.
> >
>
> I think your explanation is very clear.

One more thing I should mention.
You do not need to wait for atomic large writes patches to land.
There is nothing stopping you from implementing the suggested
solution based on the xfs code already in master (v6.13-rc1),
which has support for the RWF_ATOMIC flag for writes.

It just means that the API will not be usable for applications that
want to do IO larger than block size, but concurrent read/write
performance of 4K IO could be improved already.

It's possible that all you need to do is:

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c488ae26b23d0..2542f15496488 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -777,9 +777,10 @@ xfs_file_buffered_write(
        ssize_t                 ret;
        bool                    cleared_space = false;
        unsigned int            iolock;
+       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;

 write_retry:
-       iolock = XFS_IOLOCK_EXCL;
+       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;
        ret = xfs_ilock_iocb(iocb, iolock);
--

xfs_file_write_checks() afterwards already takes care of promoting
XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.

It is possible that XFS_IOLOCK_EXCL could be immediately demoted
back to XFS_IOLOCK_SHARED for atomic_writes as done in
xfs_file_dio_write_aligned().

TBH, I am not sure which blockdevs support 4K atomic writes that could
be used to test this.

John, can you share your test setup instructions for atomic writes?

Thanks,
Amir.

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

[Amir Goldstein]

On Wed, Jan 8, 2025 at 12:33 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jan 8, 2025 at 8:43 AM Chi Zhiling <chizhiling@163.com> wrote:
> >
> > On 2025/1/7 20:13, Amir Goldstein wrote:
> > > On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
> > >> On 2024/12/29 06:17, Dave Chinner wrote:
> > >>> On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> > >>>> On 2024/12/27 05:50, Dave Chinner wrote:
> > >>>>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> > >>>>>> From: Chi Zhiling <chizhiling@kylinos.cn>
> > >>>>>>
> > >>>>>> Using an rwsem to protect file data ensures that we can always obtain a
> > >>>>>> completed modification. But due to the lock, we need to wait for the
> > >>>>>> write process to release the rwsem before we can read it, even if we are
> > >>>>>> reading a different region of the file. This could take a lot of time
> > >>>>>> when many processes need to write and read this file.
> > >>>>>>
> > >>>>>> On the other hand, The ext4 filesystem and others do not hold the lock
> > >>>>>> during buffered reading, which make the ext4 have better performance in
> > >>>>>> that case. Therefore, I think it will be fine if we remove the lock in
> > >>>>>> xfs, as most applications can handle this situation.
> > >>>>>
> > >>>>> Nope.
> > >>>>>
> > >>>>> This means that XFS loses high level serialisation of incoming IO
> > >>>>> against operations like truncate, fallocate, pnfs operations, etc.
> > >>>>>
> > >>>>> We've been through this multiple times before; the solution lies in
> > >>>>> doing the work to make buffered writes use shared locking, not
> > >>>>> removing shared locking from buffered reads.
> > >>>>
> > >>>> You mean using shared locking for buffered reads and writes, right?
> > >>>>
> > >>>> I think it's a great idea. In theory, write operations can be performed
> > >>>> simultaneously if they write to different ranges.
> > >>>
> > >>> Even if they overlap, the folio locks will prevent concurrent writes
> > >>> to the same range.
> > >>>
> > >>> Now that we have atomic write support as native functionality (i.e.
> > >>> RWF_ATOMIC), we really should not have to care that much about
> > >>> normal buffered IO being atomic. i.e. if the application wants
> > >>> atomic writes, it can now specify that it wants atomic writes and so
> > >>> we can relax the constraints we have on existing IO...
> > >>
> > >> Yes, I'm not particularly concerned about whether buffered I/O is
> > >> atomic. I'm more concerned about the multithreading performance of
> > >> buffered I/O.
> > >
> > > Hi Chi,
> > >
> > > Sorry for joining late, I was on vacation.
> > > I am very happy that you have taken on this task and your timing is good,
> > > because  John Garry just posted his patches for large atomic writes [1].
> >
> > I'm glad to have you on board. :)
> >
> > I'm not sure if my understanding is correct, but it seems that our
> > discussion is about multithreading safety, while John's patch is about
> > providing power-failure safety, even though both mention atomicity.
> >
>
> You are right about the *motivation* of John's work.
> But as a by-product of his work, we get an API to opt-in for
> atomic writes and we can piggy back this opt-in API to say
> that whoever wants the legacy behavior can use the new API
> to get both power failure safety and multithreading safety.
>
> > >
> > > I need to explain the relation to atomic buffered I/O, because it is not
> > > easy to follow it from the old discussions and also some of the discussions
> > > about the solution were held in-person during LSFMM2024.
> > >
> > > Naturally, your interest is improving multithreading performance of
> > > buffered I/O, so was mine when I first posted this question [2].
> > >
> > > The issue with atomicity of buffered I/O is the xfs has traditionally
> > > provided atomicity of write vs. read (a.k.a no torn writes), which is
> > > not required by POSIX standard (because POSIX was not written with
> > > threads in mind) and is not respected by any other in-tree filesystem.
> > >
> > > It is obvious that the exclusive rw lock for buffered write hurts performance
> > > in the case of mixed read/write on xfs, so the question was - if xfs provides
> > > atomic semantics that portable applications cannot rely on, why bother
> > > providing these atomic semantics at all?
> >
> > Perhaps we can add an option that allows distributions or users to
> > decide whether they need this semantics. I would not hesitate to
> > turn off this semantics on my system when the time comes.
> >
>
> Yes, we can, but we do not want to do it unless we have to.
> 99% of the users do not want the legacy semantics, so it would
> be better if they get the best performance without having to opt-in to get it.
>
> > >
> > > 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.
> >
> > > 3. The upcoming ability of application to opt-in for atomic writes larger
> > >      than fs block size [1] can be used to facilitate the applications that
> > >      want to legacy xfs semantics and avoid the need to enforce the legacy
> > >      semantics all the time for no good reason
> > >
> > > Disclaimer: this is not a standard way to deal with potentially breaking
> > > legacy semantics, because old applications will not usually be rebuilt
> > > to opt-in for the old semantics, but the use case in hand is probably
> > > so specific, of a specific application that relies on xfs specific semantics
> > > that there are currently no objections for trying this solution.
> > >
> > > [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
> > > [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> > >
> > >>
> > >> Last week, it was mentioned that removing i_rwsem would have some
> > >> impacts on truncate, fallocate, and PNFS operations.
> > >>
> > >> (I'm not familiar with pNFS, so please correct me if I'm wrong.)
> > >
> > > You are not wrong. pNFS uses a "layout lease", which requires
> > > that the blockmap of the file will not be modified while the lease is held.
> > > but I think that block that are not mapped (i.e. holes) can be mapped
> > > while the lease is held.
> > >
> > >>
> > >> My understanding is that the current i_rwsem is used to protect both
> > >> the file's data and its size. Operations like truncate, fallocate,
> > >> and PNFS use i_rwsem because they modify both the file's data and its
> > >> size. So, I'm thinking whether it's possible to use i_rwsem to protect
> > >> only the file's size, without protecting the file's data.
> > >>
> > >
> > > It also protects the file's blockmap, for example, punch hole
> > > does not change the size, but it unmaps blocks from the blockmap,
> > > leading to races that could end up reading stale data from disk
> > > if the lock wasn't taken.
> > >
> > >> So operations that modify the file's size need to be executed
> > >> sequentially. For example, buffered writes to the EOF, fallocate
> > >> operations without the "keep size" requirement, and truncate operations,
> > >> etc, all need to hold an exclusive lock.
> > >>
> > >> Other operations require a shared lock because they only need to access
> > >> the file's size without modifying it.
> > >>
> > >
> > > As far as I understand, exclusive lock is not needed for non-extending
> > > writes, because it is ok to map new blocks.
> > > I guess the need for exclusive lock for extending writes is related to
> > > update of file size, but not 100% sure.
> > > Anyway, exclusive lock on extending write is widely common in other fs,
> > > while exclusive lock for non-extending write is unique to xfs.
> > >
> > >>>
> > >>>> So we should track all the ranges we are reading or writing,
> > >>>> and check whether the new read or write operations can be performed
> > >>>> concurrently with the current operations.
> > >>>
> > >>> That is all discussed in detail in the discussions I linked.
> > >>
> > >> Sorry, I overlooked some details from old discussion last time.
> > >> It seems that you are not satisfied with the effectiveness of
> > >> range locks.
> > >>
> > >
> > > Correct. This solution was taken off the table.
> > >
> > > I hope my explanation was correct and clear.
> > > If anything else is not clear, please feel free to ask.
> > >
> >
> > I think your explanation is very clear.
>
> One more thing I should mention.
> You do not need to wait for atomic large writes patches to land.
> There is nothing stopping you from implementing the suggested
> solution based on the xfs code already in master (v6.13-rc1),
> which has support for the RWF_ATOMIC flag for writes.
>
> It just means that the API will not be usable for applications that
> want to do IO larger than block size, but concurrent read/write
> performance of 4K IO could be improved already.
>
> It's possible that all you need to do is:
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c488ae26b23d0..2542f15496488 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -777,9 +777,10 @@ xfs_file_buffered_write(
>         ssize_t                 ret;
>         bool                    cleared_space = false;
>         unsigned int            iolock;
> +       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;
>
>  write_retry:
> -       iolock = XFS_IOLOCK_EXCL;
> +       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;

It's the other way around of course :)
and did not state the obvious - this mock patch is NOT TESTED!

Thanks,
Amir.

>         ret = xfs_ilock_iocb(iocb, iolock);
> --
>
> xfs_file_write_checks() afterwards already takes care of promoting
> XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.
>
> It is possible that XFS_IOLOCK_EXCL could be immediately demoted
> back to XFS_IOLOCK_SHARED for atomic_writes as done in
> xfs_file_dio_write_aligned().
>
> TBH, I am not sure which blockdevs support 4K atomic writes that could
> be used to test this.
>
> John, can you share your test setup instructions for atomic writes?
>
> Thanks,
> Amir.

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

[John Garry]

> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c488ae26b23d0..2542f15496488 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -777,9 +777,10 @@ xfs_file_buffered_write(
>          ssize_t                 ret;
>          bool                    cleared_space = false;
>          unsigned int            iolock;
> +       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;
> 
>   write_retry:
> -       iolock = XFS_IOLOCK_EXCL;
> +       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;
>          ret = xfs_ilock_iocb(iocb, iolock);
> --
> 
> xfs_file_write_checks() afterwards already takes care of promoting
> XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.
> 
> It is possible that XFS_IOLOCK_EXCL could be immediately demoted
> back to XFS_IOLOCK_SHARED for atomic_writes as done in
> xfs_file_dio_write_aligned().
> 
> TBH, I am not sure which blockdevs support 4K atomic writes that could
> be used to test this.
> 
> John, can you share your test setup instructions for atomic writes?

Please note that IOCB_ATOMIC is not supported for buffered IO, so we 
can't do this - we only support direct IO today.

And supporting buffered IO has its challenges; how to handle overlapping 
atomic writes of differing sizes sitting in the page cache is the main 
issue which comes to mind.

Thanks,
John

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

[Amir Goldstein]

On Wed, Jan 8, 2025 at 1:16 PM John Garry <john.g.garry@oracle.com> wrote:
>
>
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index c488ae26b23d0..2542f15496488 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -777,9 +777,10 @@ xfs_file_buffered_write(
> >          ssize_t                 ret;
> >          bool                    cleared_space = false;
> >          unsigned int            iolock;
> > +       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;
> >
> >   write_retry:
> > -       iolock = XFS_IOLOCK_EXCL;
> > +       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;
> >          ret = xfs_ilock_iocb(iocb, iolock);
> > --
> >
> > xfs_file_write_checks() afterwards already takes care of promoting
> > XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.
> >
> > It is possible that XFS_IOLOCK_EXCL could be immediately demoted
> > back to XFS_IOLOCK_SHARED for atomic_writes as done in
> > xfs_file_dio_write_aligned().
> >
> > TBH, I am not sure which blockdevs support 4K atomic writes that could
> > be used to test this.
> >
> > John, can you share your test setup instructions for atomic writes?
>
> Please note that IOCB_ATOMIC is not supported for buffered IO, so we
> can't do this - we only support direct IO today.

Oops. I see now.

>
> And supporting buffered IO has its challenges; how to handle overlapping
> atomic writes of differing sizes sitting in the page cache is the main
> issue which comes to mind.
>

How about the combination of RWF_ATOMIC | RWF_UNCACHED [1]
Would it be easier/possible to support this considering that the write of folio
is started before the write system call returns?

Note that application that desires mutithreaded atomicity of writes vs. reads
will only need to opt-in for RWF_ATOMIC | RWF_UNCACHED writes, so this
is not expected to actually break its performance by killing the read caching.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20241220154831.1086649-1-axboe@kernel.dk/

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

[John Garry]

On 09/01/2025 10:07, Amir Goldstein wrote:
>> Please note that IOCB_ATOMIC is not supported for buffered IO, so we
>> can't do this - we only support direct IO today.
> Oops. I see now.
> 
>> And supporting buffered IO has its challenges; how to handle overlapping
>> atomic writes of differing sizes sitting in the page cache is the main
>> issue which comes to mind.
>>
> How about the combination of RWF_ATOMIC | RWF_UNCACHED [1]
> Would it be easier/possible to support this considering that the write of folio
> is started before the write system call returns?

I am not sure exactly what you are proposing. Is it that RWF_ATOMIC for 
buffered IO auto-sets RWF_UNCACHED? Or that RWF_ATOMIC requires 
RWF_UNCACHED to be set?

But that is not so important, as I just think that future users of 
RWF_ATOMIC may not want the behavior of RWF_UNCACHED always (for 
buffered IO).

And I don't think that RWF_UNCACHED even properly solves the issues of 
RWF_ATOMIC for buffered IO in terms of handling overlapping atomic 
writes in the page cache.

Thanks,
John

> 
> Note that application that desires mutithreaded atomicity of writes vs. reads
> will only need to opt-in for RWF_ATOMIC | RWF_UNCACHED writes, so this
> is not expected to actually break its performance by killing the read caching.
> 
> Thanks,
> Amir.
> 
> [1]https://urldefense.com/v3/__https://lore.kernel.org/linux- 
> fsdevel/20241220154831.1086649-1-axboe@kernel.dk/__;!!ACWV5N9M2RV99hQ! 
> J7_5N_kSixl5iSy8IX37Cup3uKTHAaC5Oy-RlvsJeTE2kr3iJ2IXNww_rApK7TwI_ocCBSE- 
> G2vZSKSRHqY$ 

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

[Chi Zhiling]

On 2025/1/8 19:33, Amir Goldstein wrote:
> On Wed, Jan 8, 2025 at 8:43 AM Chi Zhiling <chizhiling@163.com> wrote:
>> On 2025/1/7 20:13, Amir Goldstein wrote:
>>> On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
>>>> On 2024/12/29 06:17, Dave Chinner wrote:
>>>>> On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
>>>>>> On 2024/12/27 05:50, Dave Chinner wrote:
>>>>>>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
>>>>>>>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>>>>>>>
>>>>>>>> Using an rwsem to protect file data ensures that we can always obtain a
>>>>>>>> completed modification. But due to the lock, we need to wait for the
>>>>>>>> write process to release the rwsem before we can read it, even if we are
>>>>>>>> reading a different region of the file. This could take a lot of time
>>>>>>>> when many processes need to write and read this file.
>>>>>>>>
>>>>>>>> On the other hand, The ext4 filesystem and others do not hold the lock
>>>>>>>> during buffered reading, which make the ext4 have better performance in
>>>>>>>> that case. Therefore, I think it will be fine if we remove the lock in
>>>>>>>> xfs, as most applications can handle this situation.
>>>>>>>
>>>>>>> Nope.
>>>>>>>
>>>>>>> This means that XFS loses high level serialisation of incoming IO
>>>>>>> against operations like truncate, fallocate, pnfs operations, etc.
>>>>>>>
>>>>>>> We've been through this multiple times before; the solution lies in
>>>>>>> doing the work to make buffered writes use shared locking, not
>>>>>>> removing shared locking from buffered reads.
>>>>>>
>>>>>> You mean using shared locking for buffered reads and writes, right?
>>>>>>
>>>>>> I think it's a great idea. In theory, write operations can be performed
>>>>>> simultaneously if they write to different ranges.
>>>>>
>>>>> Even if they overlap, the folio locks will prevent concurrent writes
>>>>> to the same range.
>>>>>
>>>>> Now that we have atomic write support as native functionality (i.e.
>>>>> RWF_ATOMIC), we really should not have to care that much about
>>>>> normal buffered IO being atomic. i.e. if the application wants
>>>>> atomic writes, it can now specify that it wants atomic writes and so
>>>>> we can relax the constraints we have on existing IO...
>>>>
>>>> Yes, I'm not particularly concerned about whether buffered I/O is
>>>> atomic. I'm more concerned about the multithreading performance of
>>>> buffered I/O.
>>>
>>> Hi Chi,
>>>
>>> Sorry for joining late, I was on vacation.
>>> I am very happy that you have taken on this task and your timing is good,
>>> because  John Garry just posted his patches for large atomic writes [1].
>>
>> I'm glad to have you on board. :)
>>
>> I'm not sure if my understanding is correct, but it seems that our
>> discussion is about multithreading safety, while John's patch is about
>> providing power-failure safety, even though both mention atomicity.
>>
> 
> You are right about the *motivation* of John's work.
> But as a by-product of his work, we get an API to opt-in for
> atomic writes and we can piggy back this opt-in API to say
> that whoever wants the legacy behavior can use the new API
> to get both power failure safety and multithreading safety.

Okay, I understand what you mean.

> 
>>>
>>> I need to explain the relation to atomic buffered I/O, because it is not
>>> easy to follow it from the old discussions and also some of the discussions
>>> about the solution were held in-person during LSFMM2024.
>>>
>>> Naturally, your interest is improving multithreading performance of
>>> buffered I/O, so was mine when I first posted this question [2].
>>>
>>> The issue with atomicity of buffered I/O is the xfs has traditionally
>>> provided atomicity of write vs. read (a.k.a no torn writes), which is
>>> not required by POSIX standard (because POSIX was not written with
>>> threads in mind) and is not respected by any other in-tree filesystem.
>>>
>>> It is obvious that the exclusive rw lock for buffered write hurts performance
>>> in the case of mixed read/write on xfs, so the question was - if xfs provides
>>> atomic semantics that portable applications cannot rely on, why bother
>>> providing these atomic semantics at all?
>>
>> Perhaps we can add an option that allows distributions or users to
>> decide whether they need this semantics. I would not hesitate to
>> turn off this semantics on my system when the time comes.
>>
> 
> Yes, we can, but we do not want to do it unless we have to.
> 99% of the users do not want the legacy semantics, so it would
> be better if they get the best performance without having to opt-in to get it.
> 
>>>
>>> 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.
>>
>>> 3. The upcoming ability of application to opt-in for atomic writes larger
>>>       than fs block size [1] can be used to facilitate the applications that
>>>       want to legacy xfs semantics and avoid the need to enforce the legacy
>>>       semantics all the time for no good reason
>>>
>>> Disclaimer: this is not a standard way to deal with potentially breaking
>>> legacy semantics, because old applications will not usually be rebuilt
>>> to opt-in for the old semantics, but the use case in hand is probably
>>> so specific, of a specific application that relies on xfs specific semantics
>>> that there are currently no objections for trying this solution.
>>>
>>> [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
>>> [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
>>>
>>>>
>>>> Last week, it was mentioned that removing i_rwsem would have some
>>>> impacts on truncate, fallocate, and PNFS operations.
>>>>
>>>> (I'm not familiar with pNFS, so please correct me if I'm wrong.)
>>>
>>> You are not wrong. pNFS uses a "layout lease", which requires
>>> that the blockmap of the file will not be modified while the lease is held.
>>> but I think that block that are not mapped (i.e. holes) can be mapped
>>> while the lease is held.
>>>
>>>>
>>>> My understanding is that the current i_rwsem is used to protect both
>>>> the file's data and its size. Operations like truncate, fallocate,
>>>> and PNFS use i_rwsem because they modify both the file's data and its
>>>> size. So, I'm thinking whether it's possible to use i_rwsem to protect
>>>> only the file's size, without protecting the file's data.
>>>>
>>>
>>> It also protects the file's blockmap, for example, punch hole
>>> does not change the size, but it unmaps blocks from the blockmap,
>>> leading to races that could end up reading stale data from disk
>>> if the lock wasn't taken.
>>>
>>>> So operations that modify the file's size need to be executed
>>>> sequentially. For example, buffered writes to the EOF, fallocate
>>>> operations without the "keep size" requirement, and truncate operations,
>>>> etc, all need to hold an exclusive lock.
>>>>
>>>> Other operations require a shared lock because they only need to access
>>>> the file's size without modifying it.
>>>>
>>>
>>> As far as I understand, exclusive lock is not needed for non-extending
>>> writes, because it is ok to map new blocks.
>>> I guess the need for exclusive lock for extending writes is related to
>>> update of file size, but not 100% sure.
>>> Anyway, exclusive lock on extending write is widely common in other fs,
>>> while exclusive lock for non-extending write is unique to xfs.
>>>
>>>>>
>>>>>> So we should track all the ranges we are reading or writing,
>>>>>> and check whether the new read or write operations can be performed
>>>>>> concurrently with the current operations.
>>>>>
>>>>> That is all discussed in detail in the discussions I linked.
>>>>
>>>> Sorry, I overlooked some details from old discussion last time.
>>>> It seems that you are not satisfied with the effectiveness of
>>>> range locks.
>>>>
>>>
>>> Correct. This solution was taken off the table.
>>>
>>> I hope my explanation was correct and clear.
>>> If anything else is not clear, please feel free to ask.
>>>
>>
>> I think your explanation is very clear.
> 
> One more thing I should mention.
> You do not need to wait for atomic large writes patches to land.
> There is nothing stopping you from implementing the suggested
> solution based on the xfs code already in master (v6.13-rc1),
> which has support for the RWF_ATOMIC flag for writes.
> 
> It just means that the API will not be usable for applications that
> want to do IO larger than block size, but concurrent read/write
                               ^
To be precise, this is the page size, not the block size, right?

> performance of 4K IO could be improved already.

Great, which means that IO operations aligned within a single page
can be executed concurrently, because the folio lock already
provides atomicity guarantees.

If the write does not exceed the boundary of a page, we can
downgrade the iolock to XFS_IOLOCK_SHARED. It seems to be safe
and will not change the current behavior.

--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -454,6 +454,11 @@ xfs_file_write_checks(
         if (error)
                 return error;

+       if ( iocb->ki_pos >> PAGE_SHIFT == (iocb->ki_pos + count) >> 
PAGE_SHIFT) {
+               *iolock = XFS_IOLOCK_SHARED;
+       }
+
         /*
          * For changing security info in file_remove_privs() we need 
i_rwsem
          * exclusively.

> 
> It's possible that all you need to do is:
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c488ae26b23d0..2542f15496488 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -777,9 +777,10 @@ xfs_file_buffered_write(
>          ssize_t                 ret;
>          bool                    cleared_space = false;
>          unsigned int            iolock;
> +       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;
> 
>   write_retry:
> -       iolock = XFS_IOLOCK_EXCL;
> +       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;
>          ret = xfs_ilock_iocb(iocb, iolock);
> --
> 
> xfs_file_write_checks() afterwards already takes care of promoting
> XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.

Yeah, for writes that exceed the PAGE boundary, we can also promote
the lock to XFS_IOLOCK_EXCL. Otherwise, I am concerned that it may
lead to old data being retained in the file.

For example, two processes writing four pages of data to the same area.

process A           process B
--------------------------------
write AA--
<sleep>
                     new write BBBB
write --AA

The final data is BBAA.

> 
> It is possible that XFS_IOLOCK_EXCL could be immediately demoted
> back to XFS_IOLOCK_SHARED for atomic_writes as done in
> xfs_file_dio_write_aligned().
> 



Thanks,
Chi Zhiling

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

[Amir Goldstein]

> > One more thing I should mention.
> > You do not need to wait for atomic large writes patches to land.
> > There is nothing stopping you from implementing the suggested
> > solution based on the xfs code already in master (v6.13-rc1),
> > which has support for the RWF_ATOMIC flag for writes.

Only I missed the fact that there is not yet a plan to support
atomic buffered writes :-/

> >
> > It just means that the API will not be usable for applications that
> > want to do IO larger than block size, but concurrent read/write
>                                ^
> To be precise, this is the page size, not the block size, right?
>

fs block size:

        if (iocb->ki_flags & IOCB_ATOMIC) {
                /*
                 * Currently only atomic writing of a single FS block is
                 * supported. It would be possible to atomic write smaller than
                 * a FS block, but there is no requirement to support this.
                 * Note that iomap also does not support this yet.
                 */
                if (ocount != ip->i_mount->m_sb.sb_blocksize)
                        return -EINVAL;
                ret = generic_atomic_write_valid(iocb, from);
                if (ret)
                        return ret;
        }

> > performance of 4K IO could be improved already.
>
> Great, which means that IO operations aligned within a single page
> can be executed concurrently, because the folio lock already
> provides atomicity guarantees.
>
> If the write does not exceed the boundary of a page, we can
> downgrade the iolock to XFS_IOLOCK_SHARED. It seems to be safe
> and will not change the current behavior.
>
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -454,6 +454,11 @@ xfs_file_write_checks(
>          if (error)
>                  return error;
>
> +       if ( iocb->ki_pos >> PAGE_SHIFT == (iocb->ki_pos + count) >>
> PAGE_SHIFT) {
> +               *iolock = XFS_IOLOCK_SHARED;
> +       }
> +
>          /*
>           * For changing security info in file_remove_privs() we need
> i_rwsem
>           * exclusively.
>

I think that may be possible, but you should do it in the buffered write
code as the patch below.
xfs_file_write_checks() is called from code paths like
xfs_file_dio_write_unaligned() where you should not demote to shared lock.


> >
> > It's possible that all you need to do is:
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index c488ae26b23d0..2542f15496488 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -777,9 +777,10 @@ xfs_file_buffered_write(
> >          ssize_t                 ret;
> >          bool                    cleared_space = false;
> >          unsigned int            iolock;
> > +       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;
> >
> >   write_retry:
> > -       iolock = XFS_IOLOCK_EXCL;
> > +       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;
> >          ret = xfs_ilock_iocb(iocb, iolock);
> > --
> >
> > xfs_file_write_checks() afterwards already takes care of promoting
> > XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.
>
> Yeah, for writes that exceed the PAGE boundary, we can also promote
> the lock to XFS_IOLOCK_EXCL. Otherwise, I am concerned that it may
> lead to old data being retained in the file.
>
> For example, two processes writing four pages of data to the same area.
>
> process A           process B
> --------------------------------
> write AA--
> <sleep>
>                      new write BBBB
> write --AA
>
> The final data is BBAA.
>

What is the use case for which you are trying to fix performance?
Is it a use case with single block IO? if not then it does not help to implement
a partial solution for single block size IO.

Thanks,
Amir.

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

[Chi Zhiling]

On 2025/1/9 18:25, Amir Goldstein wrote:
>>> One more thing I should mention.
>>> You do not need to wait for atomic large writes patches to land.
>>> There is nothing stopping you from implementing the suggested
>>> solution based on the xfs code already in master (v6.13-rc1),
>>> which has support for the RWF_ATOMIC flag for writes.
> 
> Only I missed the fact that there is not yet a plan to support
> atomic buffered writes :-/

I think it's necessary to support atomic buffered writes.

> 
>>>
>>> It just means that the API will not be usable for applications that
>>> want to do IO larger than block size, but concurrent read/write
>>                                 ^
>> To be precise, this is the page size, not the block size, right?
>>
> 
> fs block size:
> 
>          if (iocb->ki_flags & IOCB_ATOMIC) {
>                  /*
>                   * Currently only atomic writing of a single FS block is
>                   * supported. It would be possible to atomic write smaller than
>                   * a FS block, but there is no requirement to support this.
>                   * Note that iomap also does not support this yet.
>                   */
>                  if (ocount != ip->i_mount->m_sb.sb_blocksize)
>                          return -EINVAL;
>                  ret = generic_atomic_write_valid(iocb, from);
>                  if (ret)
>                          return ret;
>          }

Uh, okay, maybe I didn't understand it very accurately. I thought
you were talking about buffered IO. I was quite curious as to why
you mentioned block size in the context of buffered IO.

> 
>>> performance of 4K IO could be improved already.
>>
>> Great, which means that IO operations aligned within a single page
>> can be executed concurrently, because the folio lock already
>> provides atomicity guarantees.
>>
>> If the write does not exceed the boundary of a page, we can
>> downgrade the iolock to XFS_IOLOCK_SHARED. It seems to be safe
>> and will not change the current behavior.
>>
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -454,6 +454,11 @@ xfs_file_write_checks(
>>           if (error)
>>                   return error;
>>
>> +       if ( iocb->ki_pos >> PAGE_SHIFT == (iocb->ki_pos + count) >>
>> PAGE_SHIFT) {
>> +               *iolock = XFS_IOLOCK_SHARED;
>> +       }
>> +
>>           /*
>>            * For changing security info in file_remove_privs() we need
>> i_rwsem
>>            * exclusively.
>>
> 
> I think that may be possible, but you should do it in the buffered write
> code as the patch below.
> xfs_file_write_checks() is called from code paths like
> xfs_file_dio_write_unaligned() where you should not demote to shared lock.

Wow, thank you for the reminder. This is the prototype of the patch.
I might need to consider more scenarios and conduct testing before
sending the patch.


> 
> 
>>>
>>> It's possible that all you need to do is:
>>>
>>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>>> index c488ae26b23d0..2542f15496488 100644
>>> --- a/fs/xfs/xfs_file.c
>>> +++ b/fs/xfs/xfs_file.c
>>> @@ -777,9 +777,10 @@ xfs_file_buffered_write(
>>>           ssize_t                 ret;
>>>           bool                    cleared_space = false;
>>>           unsigned int            iolock;
>>> +       bool                    atomic_write = iocb->ki_flags & IOCB_ATOMIC;
>>>
>>>    write_retry:
>>> -       iolock = XFS_IOLOCK_EXCL;
>>> +       iolock = atomic_write ? XFS_IOLOCK_SHARED : XFS_IOLOCK_EXCL;
>>>           ret = xfs_ilock_iocb(iocb, iolock);
>>> --
>>>
>>> xfs_file_write_checks() afterwards already takes care of promoting
>>> XFS_IOLOCK_SHARED to XFS_IOLOCK_EXCL for extending writes.
>>
>> Yeah, for writes that exceed the PAGE boundary, we can also promote
>> the lock to XFS_IOLOCK_EXCL. Otherwise, I am concerned that it may
>> lead to old data being retained in the file.
>>
>> For example, two processes writing four pages of data to the same area.
>>
>> process A           process B
>> --------------------------------
>> write AA--
>> <sleep>
>>                       new write BBBB
>> write --AA
>>
>> The final data is BBAA.
>>
> 
> What is the use case for which you are trying to fix performance?
> Is it a use case with single block IO? if not then it does not help to implement
> a partial solution for single block size IO.

I want to improve the UnixBench score for XFS. UnixBench uses buffered
IO for testing, including both single-threaded and multi-threaded tests.
Additionally, the IO size tested in UnixBench is below 4K, so this will
be very helpful.


Thanks,
Chi Zhiling

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

[John Garry]

On 09/01/2025 12:10, Chi Zhiling wrote:
> On 2025/1/9 18:25, Amir Goldstein wrote:
>>>> One more thing I should mention.
>>>> You do not need to wait for atomic large writes patches to land.
>>>> There is nothing stopping you from implementing the suggested
>>>> solution based on the xfs code already in master (v6.13-rc1),
>>>> which has support for the RWF_ATOMIC flag for writes.
>>
>> Only I missed the fact that there is not yet a plan to support
>> atomic buffered writes :-/
> 
> I think it's necessary to support atomic buffered writes.

Please check this thread then:
https://lore.kernel.org/linux-fsdevel/20240228061257.GA106651@mit.edu/

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

[Darrick J. Wong]

On Wed, Jan 08, 2025 at 03:43:04PM +0800, Chi Zhiling wrote:
> On 2025/1/7 20:13, Amir Goldstein wrote:
> > On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
> > > On 2024/12/29 06:17, Dave Chinner wrote:
> > > > On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> > > > > On 2024/12/27 05:50, Dave Chinner wrote:
> > > > > > On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> > > > > > > From: Chi Zhiling <chizhiling@kylinos.cn>
> > > > > > > 
> > > > > > > Using an rwsem to protect file data ensures that we can always obtain a
> > > > > > > completed modification. But due to the lock, we need to wait for the
> > > > > > > write process to release the rwsem before we can read it, even if we are
> > > > > > > reading a different region of the file. This could take a lot of time
> > > > > > > when many processes need to write and read this file.
> > > > > > > 
> > > > > > > On the other hand, The ext4 filesystem and others do not hold the lock
> > > > > > > during buffered reading, which make the ext4 have better performance in
> > > > > > > that case. Therefore, I think it will be fine if we remove the lock in
> > > > > > > xfs, as most applications can handle this situation.
> > > > > > 
> > > > > > Nope.
> > > > > > 
> > > > > > This means that XFS loses high level serialisation of incoming IO
> > > > > > against operations like truncate, fallocate, pnfs operations, etc.
> > > > > > 
> > > > > > We've been through this multiple times before; the solution lies in
> > > > > > doing the work to make buffered writes use shared locking, not
> > > > > > removing shared locking from buffered reads.
> > > > > 
> > > > > You mean using shared locking for buffered reads and writes, right?
> > > > > 
> > > > > I think it's a great idea. In theory, write operations can be performed
> > > > > simultaneously if they write to different ranges.
> > > > 
> > > > Even if they overlap, the folio locks will prevent concurrent writes
> > > > to the same range.
> > > > 
> > > > Now that we have atomic write support as native functionality (i.e.
> > > > RWF_ATOMIC), we really should not have to care that much about
> > > > normal buffered IO being atomic. i.e. if the application wants
> > > > atomic writes, it can now specify that it wants atomic writes and so
> > > > we can relax the constraints we have on existing IO...
> > > 
> > > Yes, I'm not particularly concerned about whether buffered I/O is
> > > atomic. I'm more concerned about the multithreading performance of
> > > buffered I/O.
> > 
> > Hi Chi,
> > 
> > Sorry for joining late, I was on vacation.
> > I am very happy that you have taken on this task and your timing is good,
> > because  John Garry just posted his patches for large atomic writes [1].
> 
> I'm glad to have you on board. :)
> 
> I'm not sure if my understanding is correct, but it seems that our
> discussion is about multithreading safety, while John's patch is about
> providing power-failure safety, even though both mention atomicity.
> 
> > 
> > I need to explain the relation to atomic buffered I/O, because it is not
> > easy to follow it from the old discussions and also some of the discussions
> > about the solution were held in-person during LSFMM2024.
> > 
> > Naturally, your interest is improving multithreading performance of
> > buffered I/O, so was mine when I first posted this question [2].
> > 
> > The issue with atomicity of buffered I/O is the xfs has traditionally
> > provided atomicity of write vs. read (a.k.a no torn writes), which is
> > not required by POSIX standard (because POSIX was not written with
> > threads in mind) and is not respected by any other in-tree filesystem.
> > 
> > It is obvious that the exclusive rw lock for buffered write hurts performance
> > in the case of mixed read/write on xfs, so the question was - if xfs provides
> > atomic semantics that portable applications cannot rely on, why bother
> > providing these atomic semantics at all?
> 
> Perhaps we can add an option that allows distributions or users to
> decide whether they need this semantics. I would not hesitate to
> turn off this semantics on my system when the time comes.
> 
> > 
> > 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.

--D

> > 3. The upcoming ability of application to opt-in for atomic writes larger
> >      than fs block size [1] can be used to facilitate the applications that
> >      want to legacy xfs semantics and avoid the need to enforce the legacy
> >      semantics all the time for no good reason
> > 
> > Disclaimer: this is not a standard way to deal with potentially breaking
> > legacy semantics, because old applications will not usually be rebuilt
> > to opt-in for the old semantics, but the use case in hand is probably
> > so specific, of a specific application that relies on xfs specific semantics
> > that there are currently no objections for trying this solution.
> > 
> > [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
> > [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> > 
> > > 
> > > Last week, it was mentioned that removing i_rwsem would have some
> > > impacts on truncate, fallocate, and PNFS operations.
> > > 
> > > (I'm not familiar with pNFS, so please correct me if I'm wrong.)
> > 
> > You are not wrong. pNFS uses a "layout lease", which requires
> > that the blockmap of the file will not be modified while the lease is held.
> > but I think that block that are not mapped (i.e. holes) can be mapped
> > while the lease is held.
> > 
> > > 
> > > My understanding is that the current i_rwsem is used to protect both
> > > the file's data and its size. Operations like truncate, fallocate,
> > > and PNFS use i_rwsem because they modify both the file's data and its
> > > size. So, I'm thinking whether it's possible to use i_rwsem to protect
> > > only the file's size, without protecting the file's data.
> > > 
> > 
> > It also protects the file's blockmap, for example, punch hole
> > does not change the size, but it unmaps blocks from the blockmap,
> > leading to races that could end up reading stale data from disk
> > if the lock wasn't taken.
> > 
> > > So operations that modify the file's size need to be executed
> > > sequentially. For example, buffered writes to the EOF, fallocate
> > > operations without the "keep size" requirement, and truncate operations,
> > > etc, all need to hold an exclusive lock.
> > > 
> > > Other operations require a shared lock because they only need to access
> > > the file's size without modifying it.
> > > 
> > 
> > As far as I understand, exclusive lock is not needed for non-extending
> > writes, because it is ok to map new blocks.
> > I guess the need for exclusive lock for extending writes is related to
> > update of file size, but not 100% sure.
> > Anyway, exclusive lock on extending write is widely common in other fs,
> > while exclusive lock for non-extending write is unique to xfs.
> > 
> > > > 
> > > > > So we should track all the ranges we are reading or writing,
> > > > > and check whether the new read or write operations can be performed
> > > > > concurrently with the current operations.
> > > > 
> > > > That is all discussed in detail in the discussions I linked.
> > > 
> > > Sorry, I overlooked some details from old discussion last time.
> > > It seems that you are not satisfied with the effectiveness of
> > > range locks.
> > > 
> > 
> > Correct. This solution was taken off the table.
> > 
> > I hope my explanation was correct and clear.
> > If anything else is not clear, please feel free to ask.
> > 
> 
> I think your explanation is very clear.
> 
> Thanks,
> Chi Zhiling
> 
> 

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

[Dave Chinner]

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?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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?

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

[Amir Goldstein]

On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> 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 that's a very neat idea, but it will not address the reference
benchmark.
The reference benchmark I started the original report with which is similar
to my understanding to the benchmark that Chi is running simulates the
workload of a database writing with buffered IO.

That means a very large file and small IO size ~64K.
Leaving the probability of intersecting writes in the same segment quite high.

Can we do this opportunistically based on available large folios?
If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
if it is not, use IOLOCK_EXCL?

for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
naturally align to IO size and above?

Thanks,
Amir.

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

[Chi Zhiling]

On 2025/1/11 01:07, Amir Goldstein wrote:
> On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> 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 that's a very neat idea, but it will not address the reference
> benchmark.
> The reference benchmark I started the original report with which is similar
> to my understanding to the benchmark that Chi is running simulates the
> workload of a database writing with buffered IO.
> 
> That means a very large file and small IO size ~64K.
> Leaving the probability of intersecting writes in the same segment quite high.
> 
> Can we do this opportunistically based on available large folios?
> If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
> if it is not, use IOLOCK_EXCL?
> 
> for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
> naturally align to IO size and above?
> 

Great, I think we're getting close to aligning our thoughts.

IMO, we shouldn't use a shared lock for write operations that are
larger than page size.

I believe the current issue is that when acquiring the i_rwsem lock,
we have no way of knowing the size of a large folio [1] (as Darrick
mentioned earlier), so we can't determine if only one large folio will
be written.

There's only one certainty: if the IO size fits within one page size,
it will definitely fit within one large folio.

So for now, we can only use IOLOCK_SHARED if we verify that the IO fits
within page size.

[1]: Maybe we can find a way to obtain the size of a folio from the page
cache, but it might come with some performance costs.


Thanks,
Chi Zhiling

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

[Darrick J. Wong]

On Sun, Jan 12, 2025 at 06:05:37PM +0800, Chi Zhiling wrote:
> On 2025/1/11 01:07, Amir Goldstein wrote:
> > On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> 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 that's a very neat idea, but it will not address the reference
> > benchmark.
> > The reference benchmark I started the original report with which is similar
> > to my understanding to the benchmark that Chi is running simulates the
> > workload of a database writing with buffered IO.
> > 
> > That means a very large file and small IO size ~64K.
> > Leaving the probability of intersecting writes in the same segment quite high.
> > 
> > Can we do this opportunistically based on available large folios?
> > If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
> > if it is not, use IOLOCK_EXCL?
> > 
> > for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
> > naturally align to IO size and above?
> > 
> 
> Great, I think we're getting close to aligning our thoughts.
> 
> IMO, we shouldn't use a shared lock for write operations that are
> larger than page size.
> 
> I believe the current issue is that when acquiring the i_rwsem lock,
> we have no way of knowing the size of a large folio [1] (as Darrick
> mentioned earlier), so we can't determine if only one large folio will
> be written.
> 
> There's only one certainty: if the IO size fits within one page size,
> it will definitely fit within one large folio.
> 
> So for now, we can only use IOLOCK_SHARED if we verify that the IO fits
> within page size.

For filesystems that /do/ support large folios (xfs), I suppose you
could have it tell iomap that it only took i_rwsem in shared mode; and
then the iomap buffered write implementation could proceed if it got a
folio covering the entire write range, or return some magic code that
means "take i_rwsem in exclusive mode and try again".

Though you're correct that we should always take IOLOCK_EXCL if the
write size is larger than whatever the max folio size is for that file.

--D

> [1]: Maybe we can find a way to obtain the size of a folio from the page
> cache, but it might come with some performance costs.
> 
> 
> Thanks,
> Chi Zhiling
> 
> 

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

[Chi Zhiling]

On 2025/1/13 10:44, Darrick J. Wong wrote:
> On Sun, Jan 12, 2025 at 06:05:37PM +0800, Chi Zhiling wrote:
>> On 2025/1/11 01:07, Amir Goldstein wrote:
>>> On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> 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 that's a very neat idea, but it will not address the reference
>>> benchmark.
>>> The reference benchmark I started the original report with which is similar
>>> to my understanding to the benchmark that Chi is running simulates the
>>> workload of a database writing with buffered IO.
>>>
>>> That means a very large file and small IO size ~64K.
>>> Leaving the probability of intersecting writes in the same segment quite high.
>>>
>>> Can we do this opportunistically based on available large folios?
>>> If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
>>> if it is not, use IOLOCK_EXCL?
>>>
>>> for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
>>> naturally align to IO size and above?
>>>
>>
>> Great, I think we're getting close to aligning our thoughts.
>>
>> IMO, we shouldn't use a shared lock for write operations that are
>> larger than page size.
>>
>> I believe the current issue is that when acquiring the i_rwsem lock,
>> we have no way of knowing the size of a large folio [1] (as Darrick
>> mentioned earlier), so we can't determine if only one large folio will
>> be written.
>>
>> There's only one certainty: if the IO size fits within one page size,
>> it will definitely fit within one large folio.
>>
>> So for now, we can only use IOLOCK_SHARED if we verify that the IO fits
>> within page size.
> 
> For filesystems that /do/ support large folios (xfs), I suppose you
> could have it tell iomap that it only took i_rwsem in shared mode; and
> then the iomap buffered write implementation could proceed if it got a
> folio covering the entire write range, or return some magic code that
> means "take i_rwsem in exclusive mode and try again".
> 
> Though you're correct that we should always take IOLOCK_EXCL if the
> write size is larger than whatever the max folio size is for that file.
> 

I think this is a pretty good idea.

I'll implement the basic functionality as soon as possible, and then we
can discuss some details for optimization.


Thanks,
Chi Zhiling

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

[Brian Foster]

On Sun, Jan 12, 2025 at 06:44:01PM -0800, Darrick J. Wong wrote:
> On Sun, Jan 12, 2025 at 06:05:37PM +0800, Chi Zhiling wrote:
> > On 2025/1/11 01:07, Amir Goldstein wrote:
> > > On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> 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 that's a very neat idea, but it will not address the reference
> > > benchmark.
> > > The reference benchmark I started the original report with which is similar
> > > to my understanding to the benchmark that Chi is running simulates the
> > > workload of a database writing with buffered IO.
> > > 
> > > That means a very large file and small IO size ~64K.
> > > Leaving the probability of intersecting writes in the same segment quite high.
> > > 
> > > Can we do this opportunistically based on available large folios?
> > > If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
> > > if it is not, use IOLOCK_EXCL?
> > > 
> > > for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
> > > naturally align to IO size and above?
> > > 
> > 
> > Great, I think we're getting close to aligning our thoughts.
> > 
> > IMO, we shouldn't use a shared lock for write operations that are
> > larger than page size.
> > 
> > I believe the current issue is that when acquiring the i_rwsem lock,
> > we have no way of knowing the size of a large folio [1] (as Darrick
> > mentioned earlier), so we can't determine if only one large folio will
> > be written.
> > 
> > There's only one certainty: if the IO size fits within one page size,
> > it will definitely fit within one large folio.
> > 
> > So for now, we can only use IOLOCK_SHARED if we verify that the IO fits
> > within page size.
> 
> For filesystems that /do/ support large folios (xfs), I suppose you
> could have it tell iomap that it only took i_rwsem in shared mode; and
> then the iomap buffered write implementation could proceed if it got a
> folio covering the entire write range, or return some magic code that
> means "take i_rwsem in exclusive mode and try again".
> 

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

Brian

> Though you're correct that we should always take IOLOCK_EXCL if the
> write size is larger than whatever the max folio size is for that file.
> 
> --D
> 
> > [1]: Maybe we can find a way to obtain the size of a folio from the page
> > cache, but it might come with some performance costs.
> > 
> > 
> > Thanks,
> > Chi Zhiling
> > 
> > 
> 

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

[Darrick J. Wong]

On Mon, Jan 13, 2025 at 08:40:51AM -0500, Brian Foster wrote:
> On Sun, Jan 12, 2025 at 06:44:01PM -0800, Darrick J. Wong wrote:
> > On Sun, Jan 12, 2025 at 06:05:37PM +0800, Chi Zhiling wrote:
> > > On 2025/1/11 01:07, Amir Goldstein wrote:
> > > > On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> 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 that's a very neat idea, but it will not address the reference
> > > > benchmark.
> > > > The reference benchmark I started the original report with which is similar
> > > > to my understanding to the benchmark that Chi is running simulates the
> > > > workload of a database writing with buffered IO.
> > > > 
> > > > That means a very large file and small IO size ~64K.
> > > > Leaving the probability of intersecting writes in the same segment quite high.
> > > > 
> > > > Can we do this opportunistically based on available large folios?
> > > > If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
> > > > if it is not, use IOLOCK_EXCL?
> > > > 
> > > > for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
> > > > naturally align to IO size and above?
> > > > 
> > > 
> > > Great, I think we're getting close to aligning our thoughts.
> > > 
> > > IMO, we shouldn't use a shared lock for write operations that are
> > > larger than page size.
> > > 
> > > I believe the current issue is that when acquiring the i_rwsem lock,
> > > we have no way of knowing the size of a large folio [1] (as Darrick
> > > mentioned earlier), so we can't determine if only one large folio will
> > > be written.
> > > 
> > > There's only one certainty: if the IO size fits within one page size,
> > > it will definitely fit within one large folio.
> > > 
> > > So for now, we can only use IOLOCK_SHARED if we verify that the IO fits
> > > within page size.
> > 
> > For filesystems that /do/ support large folios (xfs), I suppose you
> > could have it tell iomap that it only took i_rwsem in shared mode; and
> > then the iomap buffered write implementation could proceed if it got a
> > folio covering the entire write range, or return some magic code that
> > means "take i_rwsem in exclusive mode and try again".
> > 
> 
> 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 think the folio batch thing would help here, since you then could just
lock all the folios you need for a write, which provides (in effect) a
range lock implementation.

--D

> Brian
> 
> > Though you're correct that we should always take IOLOCK_EXCL if the
> > write size is larger than whatever the max folio size is for that file.
> > 
> > --D
> > 
> > > [1]: Maybe we can find a way to obtain the size of a folio from the page
> > > cache, but it might come with some performance costs.
> > > 
> > > 
> > > Thanks,
> > > Chi Zhiling
> > > 
> > > 
> > 
> 
> 

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

[Christoph Hellwig]

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.

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

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

[Dave Chinner]

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.

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.

The lock also has non-owner semantics, so DIO can take it during
submission and then drop it during IO completion. This solves the
problem we currently use the i_rwsem and
inode_dio_{start,end/wait}() to solve (i.e. create a DIO submission
barrier and waiting for all existing DIO to drain).

IOWs, a two-state shared lock provides the mechanism to allow DIO
to be done without holding the i_rwsem at all, as well as being able
to elide two atomic operations per DIO to track in-flight DIOs.

We'd get this whilst maintaining buffered/DIO coherency without
adding any new overhead to the DIO path, and allow concurrent
buffered reads and writes that have their atomicity defined by the
batched folio locking strategy that Brian is working on...

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

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

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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.

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

[Dave Chinner]

On Wed, Jan 15, 2025 at 08:36:48PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 16, 2025 at 08:41:21AM +1100, Dave Chinner wrote:
> > > 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.

OK, the consensus seems to be to run with this for the moment and
see how clean it ends up being. I'll look into it over the next
couple of weeks then.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Brian Foster]

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

Ack to this, FWIW. I think this is a natural/logical approach,
prototyping and whatnot notwithstanding.

Brian

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

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

[Amir Goldstein]

On Wed, Jan 15, 2025 at 10:41 PM Dave Chinner <david@fromorbit.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?
I guess the answer depends whether or not i_rwsem
starves existing writers in the face of ongoing new readers.
If my memory serves me right, this exact behavior of i_rwsem
is what sparked the original regression report when xfs
moved from i_iolock to i_rwsem [1].

[1] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/

>
> 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.
>
> The lock also has non-owner semantics, so DIO can take it during
> submission and then drop it during IO completion. This solves the
> problem we currently use the i_rwsem and
> inode_dio_{start,end/wait}() to solve (i.e. create a DIO submission
> barrier and waiting for all existing DIO to drain).
>
> IOWs, a two-state shared lock provides the mechanism to allow DIO
> to be done without holding the i_rwsem at all, as well as being able
> to elide two atomic operations per DIO to track in-flight DIOs.
>
> We'd get this whilst maintaining buffered/DIO coherency without
> adding any new overhead to the DIO path, and allow concurrent
> buffered reads and writes that have their atomicity defined by the
> batched folio locking strategy that Brian is working on...
>
> This only leaves DIO coherency issues with mmap() based IO as an
> issue, but that's a problem for a different day...
>
> > 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) that I don't think anybody
does in practice and nobody should care how it performs.
Am I wrong?

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

Thanks,
Amir.

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

[Dave Chinner]

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@fromorbit.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@fromorbit.com

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

[Amir Goldstein]

On Fri, Jan 17, 2025 at 11:19 PM Dave Chinner <david@fromorbit.com> wrote:
>
> 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@fromorbit.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".
>

I was questioning the need to optimize the mixed buffered read/write
*while file is also open for O_DIRECT*
I thought that the solution would be easier if can take Christof's
"Let's ignore DIRECT I/O for the first step" and make it testable.
Then if the inode is open O_DIRECT, no change to locking.

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

Still feels like we can forego performance of mixed buffered rw
*while the backup program reads the database file*, should a backup
program ever really read a large database file...

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

I did not understand that you are proposing an improvement over the
existing state of affairs. Yeh, I certainly have no objections to fixing
things that are wrong.

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

A shared-locked buffered write can only start when there is no
O_DIRECT file open on the inode.

The first open for O_DIRECT to increment i_dio_open_count
needs to take exclusive iolock to wait for all in-flight buffered writes
then release the iolock.

All DIO submitted via O_DIRECT fds will be safe against in-flight
share-locked buffered write.

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

I will be happy to see this improvement.

Thanks,
Amir.

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

[Dave Chinner]

On Sat, Jan 18, 2025 at 02:03:41PM +0100, Amir Goldstein wrote:
> On Fri, Jan 17, 2025 at 11:19 PM Dave Chinner
> <david@fromorbit.com> wrote:
> > 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@fromorbit.com> wrote: 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....
> >
> 
> A shared-locked buffered write can only start when there is no
> O_DIRECT file open on the inode.  The first open for O_DIRECT to
> increment i_dio_open_count needs to take exclusive iolock to wait
> for all in-flight buffered writes then release the iolock.
> 
> All DIO submitted via O_DIRECT fds will be safe against in-flight
> share-locked buffered write.

Hooking ->open() and ->release() isn't sufficient. O_DIRECT can be
changed at any time via fcntl(F_SETFL) which isn't synchronised
against IO in progress at all. i.e. we can't track that, nor can we
even use f_ops->check_flags to reject changes to O_DIRECT state
because it doesn't pass either the filp or the inode....

IOWs, as it stands logic like "is the file opened for O_DIRECT" in
the IO path is inherently racy and the racing mechanisms are
directly under the control of unprivileged userspace applications.
Without mods from the VFS down and new hooks being implemented in
XFS along with all the whacky "which serialisiation method do we
use" logic and dealing with the complexities and switching
between them, tracking struct files that are open for O_DIRECT isn't
really a viable mechanism for enabling shared buffered writes...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

On Sat, Jan 18, 2025 at 09:19:15AM +1100, Dave Chinner wrote:
> 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...

The original i_rwsem (still i_iolock then) scheme did that, but the
core locking maintainers asked us to remove the non-owner unlocks,
so I did that.  It turns out later we got officially sanctioned
non-owner unlocks, so we could have easily add this back.  I did that
5 years ago, but reception was lukewarm:

http://git.infradead.org/?p=users/hch/xfs.git;a=shortlog;h=refs/heads/i_rwsem-non_owner

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

[Dave Chinner]

On Tue, Jan 21, 2025 at 10:08:10PM -0800, Christoph Hellwig wrote:
> On Sat, Jan 18, 2025 at 09:19:15AM +1100, Dave Chinner wrote:
> > 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...
> 
> The original i_rwsem (still i_iolock then) scheme did that, but the
> core locking maintainers asked us to remove the non-owner unlocks,
> so I did that.  It turns out later we got officially sanctioned
> non-owner unlocks, so we could have easily add this back.  I did that
> 5 years ago, but reception was lukewarm:
> 
> http://git.infradead.org/?p=users/hch/xfs.git;a=shortlog;h=refs/heads/i_rwsem-non_owner

I have no issues with the concept - I have always thought that
holding the IOLOCK to completion was how the IO locking should work
(like we do with the xfs_buf semaphore). The i_dio_count submission
barrier solution was a necessary hack because rwsems did not work
the way we needed to use them.

What I didn't like about the proposal above was the implementation
(i.e. the encoding of rwsem semantics in the iomap dio API and
implementation), but there was never any followup that addressed the
issues that were raised.....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Chi Zhiling]

On 2025/1/16 05:41, Dave Chinner 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.
> 
> 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.
> 
> The lock also has non-owner semantics, so DIO can take it during
> submission and then drop it during IO completion. This solves the
> problem we currently use the i_rwsem and
> inode_dio_{start,end/wait}() to solve (i.e. create a DIO submission
> barrier and waiting for all existing DIO to drain).
> 
> IOWs, a two-state shared lock provides the mechanism to allow DIO
> to be done without holding the i_rwsem at all, as well as being able
> to elide two atomic operations per DIO to track in-flight DIOs.
> 
> We'd get this whilst maintaining buffered/DIO coherency without
> adding any new overhead to the DIO path, and allow concurrent
> buffered reads and writes that have their atomicity defined by the
> batched folio locking strategy that Brian is working on...
> 
> This only leaves DIO coherency issues with mmap() based IO as an
> issue, but that's a problem for a different day...
> 
>> 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 really think it's time for someone to submit the basic patch, and then
we can continue with the discussion and testing. :)

BTW, I support the solution with folio batch and two-state shared lock.


Thanks,
Chi Zhiling

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

[Chi Zhiling]

On 2025/1/16 05:41, Dave Chinner 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.
> 
> 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.
> 
> The lock also has non-owner semantics, so DIO can take it during
> submission and then drop it during IO completion. This solves the
> problem we currently use the i_rwsem and
> inode_dio_{start,end/wait}() to solve (i.e. create a DIO submission
> barrier and waiting for all existing DIO to drain).
> 
> IOWs, a two-state shared lock provides the mechanism to allow DIO
> to be done without holding the i_rwsem at all, as well as being able
> to elide two atomic operations per DIO to track in-flight DIOs.
> 
> We'd get this whilst maintaining buffered/DIO coherency without
> adding any new overhead to the DIO path, and allow concurrent
> buffered reads and writes that have their atomicity defined by the
> batched folio locking strategy that Brian is working on...
> 
> This only leaves DIO coherency issues with mmap() based IO as an
> issue, but that's a problem for a different day...

When I try to implement those features, I think we could use exclusive
locks for RWF_APPEND writes, and shared locks for other writes.

The reason is that concurrent operations are also possible for extended
writes if there is no overlap in the regions being written.
So there is no need to determine whether it is an extended write in
advance.

As for why an exclusive lock is needed for append writes, it's because
we don't want the EOF to be modified during the append write.

The code is like that:
+       if (iocb->ki_flags & IOCB_APPEND)
+               iolock = XFS_IOLOCK_EXCL;
+       else
+               iolock = XFS_IOLOCK_SHARED;


If we use exclusive locks for all extended writes, we need to check if
it is an extended write before acquiring the lock, but this value could
become outdated if we do not hold the lock.

So if we do an extended write,
we might need to follow this process:

1. Get read lock.
2. Check if it is an extended write.
3. Release read lock.
4. Get write lock.
5. Do the write operation.


Thanks,
Chi Zhiling

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

[Dave Chinner]

On Fri, Jan 24, 2025 at 03:57:43PM +0800, Chi Zhiling wrote:
> On 2025/1/16 05:41, Dave Chinner wrote:
> > IOWs, a two-state shared lock provides the mechanism to allow DIO
> > to be done without holding the i_rwsem at all, as well as being able
> > to elide two atomic operations per DIO to track in-flight DIOs.
> > 
> > We'd get this whilst maintaining buffered/DIO coherency without
> > adding any new overhead to the DIO path, and allow concurrent
> > buffered reads and writes that have their atomicity defined by the
> > batched folio locking strategy that Brian is working on...
> > 
> > This only leaves DIO coherency issues with mmap() based IO as an
> > issue, but that's a problem for a different day...
> 
> When I try to implement those features, I think we could use exclusive
> locks for RWF_APPEND writes, and shared locks for other writes.
> 
> The reason is that concurrent operations are also possible for extended
> writes if there is no overlap in the regions being written.
> So there is no need to determine whether it is an extended write in
> advance.
> 
> As for why an exclusive lock is needed for append writes, it's because
> we don't want the EOF to be modified during the append write.

We don't care if the EOF moves during the append write at the
filesystem level. We set kiocb->ki_pos = i_size_read() from
generic_write_checks() under shared locking, and if we then race
with another extending append write there are two cases:

	1. the other task has already extended i_size; or
	2. we have two IOs at the same offset (i.e. at i_size).

In either case, we don't need exclusive locking for the IO because
the worst thing that happens is that two IOs hit the same file
offset. IOWs, it has always been left up to the application
serialise RWF_APPEND writes on XFS, not the filesystem.

There is good reason for not doing exclusive locking for extending
writes. When extending the file naturally (think sequential writes),
we need those IOs be able to run concurrently with other writes
in progress. i.e. it is common for
applications to submit multiple sequential extending writes in a
batch, and as long as we submit them all in order, they all hit the
(moving) EOF exactly and hence get run concurrently.

This also works with batch-submitted RWF_APPEND AIO-DIO - they just
turn into concurrent in-flight extending writes...

IOWs, forcing exclusive locking for writes at exactly EOF is
definitely not desirable, and existing behaviour is that they use
shared locking.

The only time we use exclusive locking for extending writes is when
they -start- beyond EOF (i.e. leave a hole between EOF and
kiocb->ki_pos) and so we have to guarantee that range does not have
stale data exposed from it while the write is in progress. i.e. we
have to zero the hole that moving EOF exposes if it contains
written extents, and we cannot allow reads or writes to that hole
before we are done.

> The code is like that:
> +       if (iocb->ki_flags & IOCB_APPEND)
> +               iolock = XFS_IOLOCK_EXCL;
> +       else
> +               iolock = XFS_IOLOCK_SHARED;
> 
> 
> If we use exclusive locks for all extended writes, we need to check if
> it is an extended write before acquiring the lock, but this value could
> become outdated if we do not hold the lock.
> 
> So if we do an extended write,
> we might need to follow this process:
> 
> 1. Get read lock.
> 2. Check if it is an extended write.
> 3. Release read lock.
> 4. Get write lock.
> 5. Do the write operation.

Please see xfs_file_write_checks() - it should already have all the
shared/exclusive relocking logic we need for temporarily excluding
buffered reads whilst submitting concurrent buffered writes (i.e. it
is the same as what we already do for concurrent DIO writes).

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

On Tue, Jan 28, 2025 at 07:49:17AM +1100, Dave Chinner wrote:
> > As for why an exclusive lock is needed for append writes, it's because
> > we don't want the EOF to be modified during the append write.
> 
> We don't care if the EOF moves during the append write at the
> filesystem level. We set kiocb->ki_pos = i_size_read() from
> generic_write_checks() under shared locking, and if we then race
> with another extending append write there are two cases:
> 
> 	1. the other task has already extended i_size; or
> 	2. we have two IOs at the same offset (i.e. at i_size).
> 
> In either case, we don't need exclusive locking for the IO because
> the worst thing that happens is that two IOs hit the same file
> offset. IOWs, it has always been left up to the application
> serialise RWF_APPEND writes on XFS, not the filesystem.

I disagree.  O_APPEND (RWF_APPEND is just the Linux-specific
per-I/O version of that) is extensively used for things like
multi-thread loggers where you have multiple threads doing O_APPEND
writes to a single log file, and they expect to not lose data
that way.  The fact that we currently don't do that for O_DIRECT
is a bug, which is just papered over that barely anyone uses
O_DIRECT | O_APPEND as that's not a very natural use case for
most applications (in fact NFS got away with never allowing it
at all).  But extending racy O_APPEND to buffered writes would
break a lot of applications.

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

[David Laight]

On Mon, 27 Jan 2025 21:15:41 -0800
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jan 28, 2025 at 07:49:17AM +1100, Dave Chinner wrote:
> > > As for why an exclusive lock is needed for append writes, it's because
> > > we don't want the EOF to be modified during the append write.  
> > 
> > We don't care if the EOF moves during the append write at the
> > filesystem level. We set kiocb->ki_pos = i_size_read() from
> > generic_write_checks() under shared locking, and if we then race
> > with another extending append write there are two cases:
> > 
> > 	1. the other task has already extended i_size; or
> > 	2. we have two IOs at the same offset (i.e. at i_size).
> > 
> > In either case, we don't need exclusive locking for the IO because
> > the worst thing that happens is that two IOs hit the same file
> > offset. IOWs, it has always been left up to the application
> > serialise RWF_APPEND writes on XFS, not the filesystem.  
> 
> I disagree.  O_APPEND (RWF_APPEND is just the Linux-specific
> per-I/O version of that) is extensively used for things like
> multi-thread loggers where you have multiple threads doing O_APPEND
> writes to a single log file, and they expect to not lose data
> that way.  The fact that we currently don't do that for O_DIRECT
> is a bug, which is just papered over that barely anyone uses
> O_DIRECT | O_APPEND as that's not a very natural use case for
> most applications (in fact NFS got away with never allowing it
> at all).  But extending racy O_APPEND to buffered writes would
> break a lot of applications.

It is broken in windows :-)
You get two writes to the same file offset and then (IIRC) two advances of EOF
(usually) giving a block of '\0' bytes in the file.

You might get away with doing an atomic update of EOF and then writing
into the gap.
But you have to decide what to do if there is a seg fault on the user buffer
It could be a multi-TB write from an mmap-ed file (maybe even over NFS) that
hits a disk read error.

Actually I suspect that if you let the two writes proceed in parallel you can't
let later ones complete first.
If the returns are sequenced a write can then be redone if an earlier write
got shortened.

	David

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

[Dave Chinner]

On Mon, Jan 27, 2025 at 09:15:41PM -0800, Christoph Hellwig wrote:
> On Tue, Jan 28, 2025 at 07:49:17AM +1100, Dave Chinner wrote:
> > > As for why an exclusive lock is needed for append writes, it's because
> > > we don't want the EOF to be modified during the append write.
> > 
> > We don't care if the EOF moves during the append write at the
> > filesystem level. We set kiocb->ki_pos = i_size_read() from
> > generic_write_checks() under shared locking, and if we then race
> > with another extending append write there are two cases:
> > 
> > 	1. the other task has already extended i_size; or
> > 	2. we have two IOs at the same offset (i.e. at i_size).
> > 
> > In either case, we don't need exclusive locking for the IO because
> > the worst thing that happens is that two IOs hit the same file
> > offset. IOWs, it has always been left up to the application
> > serialise RWF_APPEND writes on XFS, not the filesystem.
> 
> I disagree.  O_APPEND (RWF_APPEND is just the Linux-specific
> per-I/O version of that) is extensively used for things like
> multi-thread loggers where you have multiple threads doing O_APPEND
> writes to a single log file, and they expect to not lose data
> that way.

Sure, but we don't think we need full file offset-scope IO exclusion
to solve that problem.  We can still safely do concurrent writes
within EOF to occur whilst another buffered append write is doing
file extension work.

IOWs, where we really need to get to is a model that allows
concurrent buffered IO at all times, except for the case where IO
operations that change the inode size need to serialise against
other similar operations (e.g. other EOF extending IOs, truncate,
etc).

Hence I think we can largely ignore O_APPEND for the
purposes of prototyping shared buffered IO and getting rid of the
IOLOCK from the XFS IO path. I may end up re-using the i_rwsem as
a "EOF modification" serialisation mechanism for O_APPEND and
extending writes in general, but I don't think we need a general
write IO exclusion mechanism for this...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

On Wed, Jan 29, 2025 at 11:59:53AM +1100, Dave Chinner wrote:
> Sure, but we don't think we need full file offset-scope IO exclusion
> to solve that problem.  We can still safely do concurrent writes
> within EOF to occur whilst another buffered append write is doing
> file extension work.

Sure.  The previous mail just sounded like you'd want to do away
with exclusion for assigning the offset.

> IOWs, where we really need to get to is a model that allows
> concurrent buffered IO at all times, except for the case where IO
> operations that change the inode size need to serialise against
> other similar operations (e.g. other EOF extending IOs, truncate,
> etc).
> 
> Hence I think we can largely ignore O_APPEND for the
> purposes of prototyping shared buffered IO and getting rid of the
> IOLOCK from the XFS IO path. I may end up re-using the i_rwsem as
> a "EOF modification" serialisation mechanism for O_APPEND and
> extending writes in general, but I don't think we need a general
> write IO exclusion mechanism for this...

That might be a chance to also fix O_DIRECT while we're at it..

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

[Chi Zhiling]

On 2025/1/28 04:49, Dave Chinner wrote:
> On Fri, Jan 24, 2025 at 03:57:43PM +0800, Chi Zhiling wrote:
>> On 2025/1/16 05:41, Dave Chinner wrote:
>>> IOWs, a two-state shared lock provides the mechanism to allow DIO
>>> to be done without holding the i_rwsem at all, as well as being able
>>> to elide two atomic operations per DIO to track in-flight DIOs.
>>>
>>> We'd get this whilst maintaining buffered/DIO coherency without
>>> adding any new overhead to the DIO path, and allow concurrent
>>> buffered reads and writes that have their atomicity defined by the
>>> batched folio locking strategy that Brian is working on...
>>>
>>> This only leaves DIO coherency issues with mmap() based IO as an
>>> issue, but that's a problem for a different day...
>>
>> When I try to implement those features, I think we could use exclusive
>> locks for RWF_APPEND writes, and shared locks for other writes.
>>
>> The reason is that concurrent operations are also possible for extended
>> writes if there is no overlap in the regions being written.
>> So there is no need to determine whether it is an extended write in
>> advance.
>>
>> As for why an exclusive lock is needed for append writes, it's because
>> we don't want the EOF to be modified during the append write.
> 
> We don't care if the EOF moves during the append write at the
> filesystem level. We set kiocb->ki_pos = i_size_read() from
> generic_write_checks() under shared locking, and if we then race
> with another extending append write there are two cases:
> 
> 	1. the other task has already extended i_size; or
> 	2. we have two IOs at the same offset (i.e. at i_size).
> 
> In either case, we don't need exclusive locking for the IO because
> the worst thing that happens is that two IOs hit the same file
> offset. IOWs, it has always been left up to the application
> serialise RWF_APPEND writes on XFS, not the filesystem.
> 
> There is good reason for not doing exclusive locking for extending
> writes. When extending the file naturally (think sequential writes),
> we need those IOs be able to run concurrently with other writes
> in progress. i.e. it is common for
> applications to submit multiple sequential extending writes in a
> batch, and as long as we submit them all in order, they all hit the
> (moving) EOF exactly and hence get run concurrently.
> 
> This also works with batch-submitted RWF_APPEND AIO-DIO - they just
> turn into concurrent in-flight extending writes...
> 
> IOWs, forcing exclusive locking for writes at exactly EOF is
> definitely not desirable, and existing behaviour is that they use
> shared locking.
> 
> The only time we use exclusive locking for extending writes is when
> they -start- beyond EOF (i.e. leave a hole between EOF and
> kiocb->ki_pos) and so we have to guarantee that range does not have
> stale data exposed from it while the write is in progress. i.e. we
> have to zero the hole that moving EOF exposes if it contains
> written extents, and we cannot allow reads or writes to that hole
> before we are done.

Sorry for the late reply.

I'm not sure why stale data would be exposed. If we zero this hole
first, then write data, and finally update the i_size, before we move
the EOF, other operations won't be able to read the data in this hole,
So I think the stale data won't be exposed.

And I think an exclusive lock is indeed needed here. The main reason is
that we need to zero this hole, and the offset of this hole is based
on EOF, so an exclusive lock is required to ensure that the EOF is not
moved.

Thanks,
Chi Zhiling

> 
>> The code is like that:
>> +       if (iocb->ki_flags & IOCB_APPEND)
>> +               iolock = XFS_IOLOCK_EXCL;
>> +       else
>> +               iolock = XFS_IOLOCK_SHARED;
>>
>>
>> If we use exclusive locks for all extended writes, we need to check if
>> it is an extended write before acquiring the lock, but this value could
>> become outdated if we do not hold the lock.
>>
>> So if we do an extended write,
>> we might need to follow this process:
>>
>> 1. Get read lock.
>> 2. Check if it is an extended write.
>> 3. Release read lock.
>> 4. Get write lock.
>> 5. Do the write operation.
> 
> Please see xfs_file_write_checks() - it should already have all the
> shared/exclusive relocking logic we need for temporarily excluding
> buffered reads whilst submitting concurrent buffered writes (i.e. it
> is the same as what we already do for concurrent DIO writes).
> 
> -Dave.

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

[Dave Chinner]

On Fri, Jan 10, 2025 at 06:07:48PM +0100, Amir Goldstein wrote:
> On Fri, Jan 10, 2025 at 12:28 AM Dave Chinner <david@fromorbit.com> wrote:
> > 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 that's a very neat idea, but it will not address the reference
> benchmark.
> The reference benchmark I started the original report with which is similar
> to my understanding to the benchmark that Chi is running simulates the
> workload of a database writing with buffered IO.
> 
> That means a very large file and small IO size ~64K.
> Leaving the probability of intersecting writes in the same segment quite high.

Likely - I recognised this granularity problem, though:

| If we need more concurrency than an unsigned long worth of bits for
| buffered writes, then maybe we can enlarge the bitmap further.

We could also hash offsets into the bitmap so that offset-local IO
hit different locks - the bit lock doesn't necessarily need to be
range based. i.e. this is a "finer grained" lock that will typically
increase concurrency. If we keep striving for perfect (i.e. scalable
range locks) we're not going to improve the situation any time
soon...

> Can we do this opportunistically based on available large folios?
> If IO size is within an existing folio, use the folio lock and IOLOCK_SHARED
> if it is not, use IOLOCK_EXCL?

The biggest problem with this is that direct IO will -not- do
folio-by folio locking, and so folio based locking does not work for
direct IO exclusion. Currently we get coherency against buffered
writes by writing back or invalidation dirty folios before doing
the DIO read/write. Because we hold the IOLOCK shared, a buffered
write will not redirty the page cache until the DIO write has been
submitted (and completed for non-async DIO).

Hence moving to shared i_rwsem, folio-based range locking for
buffered writes will lose all serialisation against DIO operations.
We will lose what coherency we currently have between buffered write
ops and DIO, and I don't think that's an acceptible trade-off.

i.e. The problem with using the mapping tree for DIO coherency is,
once again, locking overhead. If we have to insert exceptional
entries to lock the range in the mapping tree because there is no
folio present (for DIO to serialise against new buffered IOs), then
we are simply back to the same exclusive tree update scalability
problem that tree based range lock algorithms have....

That's why I suggested a bitmap lock external to both buffered and
direct IO...

> for a benchmark that does all buffered IO 64K aligned, wouldn't large folios
> naturally align to IO size and above?

Maybe, but I don't think folio/mapping tree based locking is a
workable solution. Hence the external bitmap idea...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Jinliang Zheng]

On Tue, 7 Jan 2025 13:13:17 +0100, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
> >
> >
> >
> > On 2024/12/29 06:17, Dave Chinner wrote:
> > > On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> > >>
> > >>
> > >> On 2024/12/27 05:50, Dave Chinner wrote:
> > >>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> > >>>> From: Chi Zhiling <chizhiling@kylinos.cn>
> > >>>>
> > >>>> Using an rwsem to protect file data ensures that we can always obtain a
> > >>>> completed modification. But due to the lock, we need to wait for the
> > >>>> write process to release the rwsem before we can read it, even if we are
> > >>>> reading a different region of the file. This could take a lot of time
> > >>>> when many processes need to write and read this file.
> > >>>>
> > >>>> On the other hand, The ext4 filesystem and others do not hold the lock
> > >>>> during buffered reading, which make the ext4 have better performance in
> > >>>> that case. Therefore, I think it will be fine if we remove the lock in
> > >>>> xfs, as most applications can handle this situation.
> > >>>
> > >>> Nope.
> > >>>
> > >>> This means that XFS loses high level serialisation of incoming IO
> > >>> against operations like truncate, fallocate, pnfs operations, etc.
> > >>>
> > >>> We've been through this multiple times before; the solution lies in
> > >>> doing the work to make buffered writes use shared locking, not
> > >>> removing shared locking from buffered reads.
> > >>
> > >> You mean using shared locking for buffered reads and writes, right?
> > >>
> > >> I think it's a great idea. In theory, write operations can be performed
> > >> simultaneously if they write to different ranges.
> > >
> > > Even if they overlap, the folio locks will prevent concurrent writes
> > > to the same range.
> > >
> > > Now that we have atomic write support as native functionality (i.e.
> > > RWF_ATOMIC), we really should not have to care that much about
> > > normal buffered IO being atomic. i.e. if the application wants
> > > atomic writes, it can now specify that it wants atomic writes and so
> > > we can relax the constraints we have on existing IO...
> >
> > Yes, I'm not particularly concerned about whether buffered I/O is
> > atomic. I'm more concerned about the multithreading performance of
> > buffered I/O.
> 
> Hi Chi,
> 
> Sorry for joining late, I was on vacation.
> I am very happy that you have taken on this task and your timing is good,
> because  John Garry just posted his patches for large atomic writes [1].
> 
> I need to explain the relation to atomic buffered I/O, because it is not
> easy to follow it from the old discussions and also some of the discussions
> about the solution were held in-person during LSFMM2024.
> 
> Naturally, your interest is improving multithreading performance of
> buffered I/O, so was mine when I first posted this question [2].
> 
> The issue with atomicity of buffered I/O is the xfs has traditionally
> provided atomicity of write vs. read (a.k.a no torn writes), which is
> not required by POSIX standard (because POSIX was not written with
> threads in mind) and is not respected by any other in-tree filesystem.
> 
> It is obvious that the exclusive rw lock for buffered write hurts performance
> in the case of mixed read/write on xfs, so the question was - if xfs provides
> atomic semantics that portable applications cannot rely on, why bother
> providing these atomic semantics at all?
> 
> 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
> 3. The upcoming ability of application to opt-in for atomic writes larger
>     than fs block size [1] can be used to facilitate the applications that
>     want to legacy xfs semantics and avoid the need to enforce the legacy
>     semantics all the time for no good reason
> 
> Disclaimer: this is not a standard way to deal with potentially breaking
> legacy semantics, because old applications will not usually be rebuilt
> to opt-in for the old semantics, but the use case in hand is probably
> so specific, of a specific application that relies on xfs specific semantics
> that there are currently no objections for trying this solution.
> 
> [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
> [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> 
> >
> > Last week, it was mentioned that removing i_rwsem would have some
> > impacts on truncate, fallocate, and PNFS operations.
> >
> > (I'm not familiar with pNFS, so please correct me if I'm wrong.)
> 
> You are not wrong. pNFS uses a "layout lease", which requires
> that the blockmap of the file will not be modified while the lease is held.
> but I think that block that are not mapped (i.e. holes) can be mapped
> while the lease is held.
> 
> >
> > My understanding is that the current i_rwsem is used to protect both
> > the file's data and its size. Operations like truncate, fallocate,
> > and PNFS use i_rwsem because they modify both the file's data and its
> > size. So, I'm thinking whether it's possible to use i_rwsem to protect
> > only the file's size, without protecting the file's data.
> >
> 
> It also protects the file's blockmap, for example, punch hole
> does not change the size, but it unmaps blocks from the blockmap,
> leading to races that could end up reading stale data from disk
> if the lock wasn't taken.
> 
> > So operations that modify the file's size need to be executed
> > sequentially. For example, buffered writes to the EOF, fallocate
> > operations without the "keep size" requirement, and truncate operations,
> > etc, all need to hold an exclusive lock.
> >
> > Other operations require a shared lock because they only need to access
> > the file's size without modifying it.
> >
> 
> As far as I understand, exclusive lock is not needed for non-extending
> writes, because it is ok to map new blocks.
> I guess the need for exclusive lock for extending writes is related to
> update of file size, but not 100% sure.


> Anyway, exclusive lock on extending write is widely common in other fs,
> while exclusive lock for non-extending write is unique to xfs.

I am sorry but I can't understand, because I found ext4_buffered_write_iter()
take exclusive lock unconditionally.

Did I miss some context of the discussion?

Thank you very much.
Jinliang Zheng

> 
> > >
> > >> So we should track all the ranges we are reading or writing,
> > >> and check whether the new read or write operations can be performed
> > >> concurrently with the current operations.
> > >
> > > That is all discussed in detail in the discussions I linked.
> >
> > Sorry, I overlooked some details from old discussion last time.
> > It seems that you are not satisfied with the effectiveness of
> > range locks.
> >
> 
> Correct. This solution was taken off the table.
> 
> I hope my explanation was correct and clear.
> If anything else is not clear, please feel free to ask.
> 
> Thanks,
> Amir.

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

[Amir Goldstein]

On Sat, Jan 25, 2025 at 9:43 AM Jinliang Zheng <alexjlzheng@gmail.com> wrote:
>
> On Tue, 7 Jan 2025 13:13:17 +0100, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Mon, Dec 30, 2024 at 3:43 AM Chi Zhiling <chizhiling@163.com> wrote:
> > >
> > >
> > >
> > > On 2024/12/29 06:17, Dave Chinner wrote:
> > > > On Sat, Dec 28, 2024 at 03:37:41PM +0800, Chi Zhiling wrote:
> > > >>
> > > >>
> > > >> On 2024/12/27 05:50, Dave Chinner wrote:
> > > >>> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> > > >>>> From: Chi Zhiling <chizhiling@kylinos.cn>
> > > >>>>
> > > >>>> Using an rwsem to protect file data ensures that we can always obtain a
> > > >>>> completed modification. But due to the lock, we need to wait for the
> > > >>>> write process to release the rwsem before we can read it, even if we are
> > > >>>> reading a different region of the file. This could take a lot of time
> > > >>>> when many processes need to write and read this file.
> > > >>>>
> > > >>>> On the other hand, The ext4 filesystem and others do not hold the lock
> > > >>>> during buffered reading, which make the ext4 have better performance in
> > > >>>> that case. Therefore, I think it will be fine if we remove the lock in
> > > >>>> xfs, as most applications can handle this situation.
> > > >>>
> > > >>> Nope.
> > > >>>
> > > >>> This means that XFS loses high level serialisation of incoming IO
> > > >>> against operations like truncate, fallocate, pnfs operations, etc.
> > > >>>
> > > >>> We've been through this multiple times before; the solution lies in
> > > >>> doing the work to make buffered writes use shared locking, not
> > > >>> removing shared locking from buffered reads.
> > > >>
> > > >> You mean using shared locking for buffered reads and writes, right?
> > > >>
> > > >> I think it's a great idea. In theory, write operations can be performed
> > > >> simultaneously if they write to different ranges.
> > > >
> > > > Even if they overlap, the folio locks will prevent concurrent writes
> > > > to the same range.
> > > >
> > > > Now that we have atomic write support as native functionality (i.e.
> > > > RWF_ATOMIC), we really should not have to care that much about
> > > > normal buffered IO being atomic. i.e. if the application wants
> > > > atomic writes, it can now specify that it wants atomic writes and so
> > > > we can relax the constraints we have on existing IO...
> > >
> > > Yes, I'm not particularly concerned about whether buffered I/O is
> > > atomic. I'm more concerned about the multithreading performance of
> > > buffered I/O.
> >
> > Hi Chi,
> >
> > Sorry for joining late, I was on vacation.
> > I am very happy that you have taken on this task and your timing is good,
> > because  John Garry just posted his patches for large atomic writes [1].
> >
> > I need to explain the relation to atomic buffered I/O, because it is not
> > easy to follow it from the old discussions and also some of the discussions
> > about the solution were held in-person during LSFMM2024.
> >
> > Naturally, your interest is improving multithreading performance of
> > buffered I/O, so was mine when I first posted this question [2].
> >
> > The issue with atomicity of buffered I/O is the xfs has traditionally
> > provided atomicity of write vs. read (a.k.a no torn writes), which is
> > not required by POSIX standard (because POSIX was not written with
> > threads in mind) and is not respected by any other in-tree filesystem.
> >
> > It is obvious that the exclusive rw lock for buffered write hurts performance
> > in the case of mixed read/write on xfs, so the question was - if xfs provides
> > atomic semantics that portable applications cannot rely on, why bother
> > providing these atomic semantics at all?
> >
> > 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
> > 3. The upcoming ability of application to opt-in for atomic writes larger
> >     than fs block size [1] can be used to facilitate the applications that
> >     want to legacy xfs semantics and avoid the need to enforce the legacy
> >     semantics all the time for no good reason
> >
> > Disclaimer: this is not a standard way to deal with potentially breaking
> > legacy semantics, because old applications will not usually be rebuilt
> > to opt-in for the old semantics, but the use case in hand is probably
> > so specific, of a specific application that relies on xfs specific semantics
> > that there are currently no objections for trying this solution.
> >
> > [1] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
> > [2] https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> >
> > >
> > > Last week, it was mentioned that removing i_rwsem would have some
> > > impacts on truncate, fallocate, and PNFS operations.
> > >
> > > (I'm not familiar with pNFS, so please correct me if I'm wrong.)
> >
> > You are not wrong. pNFS uses a "layout lease", which requires
> > that the blockmap of the file will not be modified while the lease is held.
> > but I think that block that are not mapped (i.e. holes) can be mapped
> > while the lease is held.
> >
> > >
> > > My understanding is that the current i_rwsem is used to protect both
> > > the file's data and its size. Operations like truncate, fallocate,
> > > and PNFS use i_rwsem because they modify both the file's data and its
> > > size. So, I'm thinking whether it's possible to use i_rwsem to protect
> > > only the file's size, without protecting the file's data.
> > >
> >
> > It also protects the file's blockmap, for example, punch hole
> > does not change the size, but it unmaps blocks from the blockmap,
> > leading to races that could end up reading stale data from disk
> > if the lock wasn't taken.
> >
> > > So operations that modify the file's size need to be executed
> > > sequentially. For example, buffered writes to the EOF, fallocate
> > > operations without the "keep size" requirement, and truncate operations,
> > > etc, all need to hold an exclusive lock.
> > >
> > > Other operations require a shared lock because they only need to access
> > > the file's size without modifying it.
> > >
> >
> > As far as I understand, exclusive lock is not needed for non-extending
> > writes, because it is ok to map new blocks.
> > I guess the need for exclusive lock for extending writes is related to
> > update of file size, but not 100% sure.
>
>
> > Anyway, exclusive lock on extending write is widely common in other fs,
> > while exclusive lock for non-extending write is unique to xfs.
>
> I am sorry but I can't understand, because I found ext4_buffered_write_iter()
> take exclusive lock unconditionally.
>
> Did I miss some context of the discussion?
>

No, I misspoke. Sorry.
The behavior that is unique to xfs is shared iolock on buffered read.
ext4 (and the rest) do not take iolock on buffered reads.

Internally, filemap_read() takes filemap_invalidate_lock_shared() as needed
to avoid races with truncate and punch hole.

Before the introduction of filemap_invalidate_lock(), xfs was the only fs that
was safe from races of punch hole vs. buffered read leading to reading
stale data.

One way to fix the mixed buffered rw performance issue is to follow suit with
other fs and not take shared iolock on buffered reads in xfs, but that has the
downside of regressing the legacy "untorn writes" xfs behavior and it will make
it hard to maintain the existing buffered/dio coherency in xfs.

So instead of going in the direction of "follow ext4 buffered io
locking scheme",
this discussion took the direction of "follow dio locking scheme" when foilios
can be used to provide "untorn writes" and use another method to mutually
exclude dio writes and buffered writes.

I hope that my summary is correct and helps to clarify where we stand.

Thanks,
Amir.

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

[Jinliang Zheng]

On Fri, 27 Dec 2024 08:50:46 +1100, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Dec 26, 2024 at 02:16:02PM +0800, Chi Zhiling wrote:
> > From: Chi Zhiling <chizhiling@kylinos.cn>
> > 
> > Using an rwsem to protect file data ensures that we can always obtain a
> > completed modification. But due to the lock, we need to wait for the
> > write process to release the rwsem before we can read it, even if we are
> > reading a different region of the file. This could take a lot of time
> > when many processes need to write and read this file.
> > 
> > On the other hand, The ext4 filesystem and others do not hold the lock
> > during buffered reading, which make the ext4 have better performance in
> > that case. Therefore, I think it will be fine if we remove the lock in
> > xfs, as most applications can handle this situation.
> 
> Nope.
> 
> This means that XFS loses high level serialisation of incoming IO
> against operations like truncate, fallocate, pnfs operations, etc.
> 
> We've been through this multiple times before; the solution lies in
> doing the work to make buffered writes use shared locking, not
> removing shared locking from buffered reads.

Hi, Dave

I have a question that I haven't figured out: If shared locking are used
in buffer writes, how can the read/write atomicity mentioned by [0] provided
by xfs be guaranteed?

  "  "I/O is intended to be atomic to ordinary files and pipes and FIFOs.
  "  Atomic means that all the bytes from a single operation that started
  "  out together end up together, without interleaving from other I/O
  "  operations."
  "  
  "  i.e. that independent read()s should see a write() as a single
  "  atomic change. hence if you do a read() concurrently with a write(),
  "  the read should either run to completion before the write, or the
  "  write run to completion before the read().
  "  
  "  XFS is the only linux filesystem that provides this behaviour.

[0] https://lore.kernel.org/linux-xfs/20190325001044.GA23020@dastard/

thanks,
Jinliang Zheng :)

> 
> A couple of old discussions from the list:
> 
> https://lore.kernel.org/linux-xfs/CAOQ4uxi0pGczXBX7GRAFs88Uw0n1ERJZno3JSeZR71S1dXg+2w@mail.gmail.com/
> https://lore.kernel.org/linux-xfs/20190404165737.30889-1-amir73il@gmail.com/
> 
> There are likely others - you can search for them yourself to get
> more background information.
> 
> Fundamentally, though, removing locking from the read side is not
> the answer to this buffered write IO exclusion problem....
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com