Odd locking pattern introduced as part of "nowait aio support"

Odd locking pattern introduced as part of "nowait aio support"

[Andres Freund]

Hi,

Especially with buffered io it's fairly easy to hit contention on the
inode lock, during writes. With something like io_uring, it's even
easier, because it currently (but see [1]) farms out buffered writes to
workers, which then can easily contend on the inode lock, even if only
one process submits writes.  But I've seen it in plenty other cases too.

Looking at the code I noticed that several parts of the "nowait aio
support" (cf 728fbc0e10b7f3) series introduced code like:

static ssize_t
ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
{
...
	if (!inode_trylock(inode)) {
		if (iocb->ki_flags & IOCB_NOWAIT)
			return -EAGAIN;
		inode_lock(inode);
	}

isn't trylocking and then locking in a blocking fashion an inefficient
pattern? I.e. I think this should be

	if (iocb->ki_flags & IOCB_NOWAIT) {
		if (!inode_trylock(inode))
			return -EAGAIN;
	}
        else
        	inode_lock(inode);

Obviously this isn't going to improve scalability to a very significant
degree. But not unnecessarily doing two atomic ops on a contended lock
can't hurt scalability either. Also, the current code just seems
confusing.

Am I missing something?

Greetings,

Andres Freund

[1] https://lore.kernel.org/linux-block/20190910164245.14625-1-axboe@kernel.dk/

Re: Odd locking pattern introduced as part of "nowait aio support"

[Dave Chinner]

On Tue, Sep 10, 2019 at 03:33:27PM -0700, Andres Freund wrote:
> Hi,
> 
> Especially with buffered io it's fairly easy to hit contention on the
> inode lock, during writes. With something like io_uring, it's even
> easier, because it currently (but see [1]) farms out buffered writes to
> workers, which then can easily contend on the inode lock, even if only
> one process submits writes.  But I've seen it in plenty other cases too.
> 
> Looking at the code I noticed that several parts of the "nowait aio
> support" (cf 728fbc0e10b7f3) series introduced code like:
> 
> static ssize_t
> ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> {
> ...
> 	if (!inode_trylock(inode)) {
> 		if (iocb->ki_flags & IOCB_NOWAIT)
> 			return -EAGAIN;
> 		inode_lock(inode);
> 	}

The ext4 code is just buggy here - we don't support RWF_NOWAIT on
buffered writes. Buffered reads, and dio/dax reads and writes, yes,
but not buffered writes because they are almost guaranteed to block
somewhere. See xfs_file_buffered_aio_write():

	if (iocb->ki_flags & IOCB_NOWAIT)
		return -EOPNOTSUPP;

generic_write_checks() will also reject IOCB_NOWAIT on buffered
writes, so that code in ext4 is likely in the wrong place...

> 
> isn't trylocking and then locking in a blocking fashion an inefficient
> pattern? I.e. I think this should be
> 
> 	if (iocb->ki_flags & IOCB_NOWAIT) {
> 		if (!inode_trylock(inode))
> 			return -EAGAIN;
> 	}
>         else
>         	inode_lock(inode);

Yes, you are right.

History: commit 91f9943e1c7b ("fs: support RWF_NOWAIT
for buffered reads") which introduced the first locking pattern
you describe in XFS.

That was followed soon after by:

commit 942491c9e6d631c012f3c4ea8e7777b0b02edeab
Author: Christoph Hellwig <hch@lst.de>
Date:   Mon Oct 23 18:31:50 2017 -0700

    xfs: fix AIM7 regression
    
    Apparently our current rwsem code doesn't like doing the trylock, then
    lock for real scheme.  So change our read/write methods to just do the
    trylock for the RWF_NOWAIT case.  This fixes a ~25% regression in
    AIM7.
    
    Fixes: 91f9943e ("fs: support RWF_NOWAIT for buffered reads")
    Reported-by: kernel test robot <xiaolong.ye@intel.com>
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
    Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Which changed all the trylock/eagain/lock patterns to the second
form you quote. None of the other filesystems had AIM7 regressions
reported against them, so nobody changed them....

> Obviously this isn't going to improve scalability to a very significant
> degree. But not unnecessarily doing two atomic ops on a contended lock
> can't hurt scalability either. Also, the current code just seems
> confusing.
> 
> Am I missing something?

Just that the sort of performance regression testing that uncovers
this sort of thing isn't widely done, and most filesystems are
concurrency limited in some way before they hit inode lock
scalability issues. Hence filesystem concurrency foccussed
benchmarks that could uncover it (like aim7) won't because the inode
locks don't end up stressed enough to make a difference to
benchmark performance.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Odd locking pattern introduced as part of "nowait aio support"

[Andres Freund]

Hi,

On 2019-09-11 14:04:20 +1000, Dave Chinner wrote:
> On Tue, Sep 10, 2019 at 03:33:27PM -0700, Andres Freund wrote:
> > Hi,
> > 
> > Especially with buffered io it's fairly easy to hit contention on the
> > inode lock, during writes. With something like io_uring, it's even
> > easier, because it currently (but see [1]) farms out buffered writes to
> > workers, which then can easily contend on the inode lock, even if only
> > one process submits writes.  But I've seen it in plenty other cases too.
> > 
> > Looking at the code I noticed that several parts of the "nowait aio
> > support" (cf 728fbc0e10b7f3) series introduced code like:
> > 
> > static ssize_t
> > ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > {
> > ...
> > 	if (!inode_trylock(inode)) {
> > 		if (iocb->ki_flags & IOCB_NOWAIT)
> > 			return -EAGAIN;
> > 		inode_lock(inode);
> > 	}
> 
> The ext4 code is just buggy here - we don't support RWF_NOWAIT on
> buffered writes.

But both buffered and non-buffered writes go through
ext4_file_write_iter(). And there's a preceding check, at least these
days, preventing IOCB_NOWAIT to apply to buffered writes:

	if (!o_direct && (iocb->ki_flags & IOCB_NOWAIT))
		return -EOPNOTSUPP;


I do really wish buffered NOWAIT was supported... The overhead of having
to do async buffered writes through the workqueue in io_uring, even if
an already existing page is targeted, is quite noticable. Even if it
failed with EAGAIN as soon as the buffered write's target isn't in the
page cache, it'd be worthwhile.


> > isn't trylocking and then locking in a blocking fashion an inefficient
> > pattern? I.e. I think this should be
> > 
> > 	if (iocb->ki_flags & IOCB_NOWAIT) {
> > 		if (!inode_trylock(inode))
> > 			return -EAGAIN;
> > 	}
> >         else
> >         	inode_lock(inode);
> 
> Yes, you are right.
> 
> History: commit 91f9943e1c7b ("fs: support RWF_NOWAIT
> for buffered reads") which introduced the first locking pattern
> you describe in XFS.

Seems, as part of the nowait work, the pattern was introduced in ext4,
xfs and btrfs. And fixed in xfs.


> That was followed soon after by:
> 
> commit 942491c9e6d631c012f3c4ea8e7777b0b02edeab
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Mon Oct 23 18:31:50 2017 -0700
> 
>     xfs: fix AIM7 regression
>     
>     Apparently our current rwsem code doesn't like doing the trylock, then
>     lock for real scheme.  So change our read/write methods to just do the
>     trylock for the RWF_NOWAIT case.  This fixes a ~25% regression in
>     AIM7.
>     
>     Fixes: 91f9943e ("fs: support RWF_NOWAIT for buffered reads")
>     Reported-by: kernel test robot <xiaolong.ye@intel.com>
>     Signed-off-by: Christoph Hellwig <hch@lst.de>
>     Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>     Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Which changed all the trylock/eagain/lock patterns to the second
> form you quote. None of the other filesystems had AIM7 regressions
> reported against them, so nobody changed them....

:(


> > Obviously this isn't going to improve scalability to a very significant
> > degree. But not unnecessarily doing two atomic ops on a contended lock
> > can't hurt scalability either. Also, the current code just seems
> > confusing.
> > 
> > Am I missing something?
> 
> Just that the sort of performance regression testing that uncovers
> this sort of thing isn't widely done, and most filesystems are
> concurrency limited in some way before they hit inode lock
> scalability issues. Hence filesystem concurrency foccussed
> benchmarks that could uncover it (like aim7) won't because the inode
> locks don't end up stressed enough to make a difference to
> benchmark performance.

Ok.  Goldwyn, do you want to write a patch, or do you want me to write
one up?


Greetings,

Andres Freund

Re: Odd locking pattern introduced as part of "nowait aio support"

[Christoph Hellwig]

On Wed, Sep 11, 2019 at 02:39:26AM -0700, Andres Freund wrote:
> I do really wish buffered NOWAIT was supported...

Send a patch..

Re: Odd locking pattern introduced as part of "nowait aio support"

[Ritesh Harjani]

Hi,

On 9/11/19 3:09 PM, Andres Freund wrote:
> Hi,
> 
> On 2019-09-11 14:04:20 +1000, Dave Chinner wrote:
>> On Tue, Sep 10, 2019 at 03:33:27PM -0700, Andres Freund wrote:
>>> Hi,
>>>
>>> Especially with buffered io it's fairly easy to hit contention on the
>>> inode lock, during writes. With something like io_uring, it's even
>>> easier, because it currently (but see [1]) farms out buffered writes to
>>> workers, which then can easily contend on the inode lock, even if only
>>> one process submits writes.  But I've seen it in plenty other cases too.
>>>
>>> Looking at the code I noticed that several parts of the "nowait aio
>>> support" (cf 728fbc0e10b7f3) series introduced code like:
>>>
>>> static ssize_t
>>> ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>> {
>>> ...
>>> 	if (!inode_trylock(inode)) {
>>> 		if (iocb->ki_flags & IOCB_NOWAIT)
>>> 			return -EAGAIN;
>>> 		inode_lock(inode);
>>> 	}
>>
>> The ext4 code is just buggy here - we don't support RWF_NOWAIT on
>> buffered write >
> But both buffered and non-buffered writes go through
> ext4_file_write_iter(). And there's a preceding check, at least these
> days, preventing IOCB_NOWAIT to apply to buffered writes:
> 
> 	if (!o_direct && (iocb->ki_flags & IOCB_NOWAIT))
> 		return -EOPNOTSUPP;
> 

-EOPNOTSUPP is now taken care in ext4 iomap patch series as well.


> 
> I do really wish buffered NOWAIT was supported... The overhead of having
> to do async buffered writes through the workqueue in io_uring, even if
> an already existing page is targeted, is quite noticable. Even if it
> failed with EAGAIN as soon as the buffered write's target isn't in the
> page cache, it'd be worthwhile.
> 
> 
>>> isn't trylocking and then locking in a blocking fashion an inefficient
>>> pattern? I.e. I think this should be
>>>
>>> 	if (iocb->ki_flags & IOCB_NOWAIT) {
>>> 		if (!inode_trylock(inode))
>>> 			return -EAGAIN;
>>> 	}
>>>          else
>>>          	inode_lock(inode);
>>
>> Yes, you are right.
>>
>> History: commit 91f9943e1c7b ("fs: support RWF_NOWAIT
>> for buffered reads") which introduced the first locking pattern
>> you describe in XFS.
> 
> Seems, as part of the nowait work, the pattern was introduced in ext4,
> xfs and btrfs. And fixed in xfs.
> 
> 
>> That was followed soon after by:
>>
>> commit 942491c9e6d631c012f3c4ea8e7777b0b02edeab
>> Author: Christoph Hellwig <hch@lst.de>
>> Date:   Mon Oct 23 18:31:50 2017 -0700
>>
>>      xfs: fix AIM7 regression
>>      
>>      Apparently our current rwsem code doesn't like doing the trylock, then
>>      lock for real scheme.  So change our read/write methods to just do the
>>      trylock for the RWF_NOWAIT case.  This fixes a ~25% regression in
>>      AIM7.
>>      
>>      Fixes: 91f9943e ("fs: support RWF_NOWAIT for buffered reads")
>>      Reported-by: kernel test robot <xiaolong.ye@intel.com>
>>      Signed-off-by: Christoph Hellwig <hch@lst.de>
>>      Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>>      Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Which changed all the trylock/eagain/lock patterns to the second
>> form you quote. None of the other filesystems had AIM7 regressions
>> reported against them, so nobody changed them....
> 
> :(
> 
> 
>>> Obviously this isn't going to improve scalability to a very significant
>>> degree. But not unnecessarily doing two atomic ops on a contended lock
>>> can't hurt scalability either. Also, the current code just seems
>>> confusing.
>>>
>>> Am I missing something?
>>
>> Just that the sort of performance regression testing that uncovers
>> this sort of thing isn't widely done, and most filesystems are
>> concurrency limited in some way before they hit inode lock
>> scalability issues. Hence filesystem concurrency foccussed
>> benchmarks that could uncover it (like aim7) won't because the inode
>> locks don't end up stressed enough to make a difference to
>> benchmark performance.
> 
> Ok.  Goldwyn, do you want to write a patch, or do you want me to write
> one up?

I am anyways looking into ext4 performance issue of mixed parallel DIO 
workload. This will require some new APIs for inode locking similar to
that of XFS.
In that I can take care of this symantics reported here by you (which is 
taken care by XFS in above patch) for ext4.


Thanks
-ritesh

> 
> 
> Greetings,
> 
> Andres Freund
> 

Re: Odd locking pattern introduced as part of "nowait aio support"

[Goldwyn Rodrigues]

On  2:39 11/09, Andres Freund wrote:
> 
> Ok.  Goldwyn, do you want to write a patch, or do you want me to write
> one up?

I'll post one shortly. Thanks for bringing this up.

-- 
Goldwyn

Fix inode sem regression for nowait

[Goldwyn Rodrigues]

This changes the way we acquire the inode semaphore when
the I/O is marked with IOCB_NOWAIT. The regression was discovered
in AIM7 and later by Andres in ext4. This has been fixed in
XFS by 942491c9e6d6 ("xfs: fix AIM7 regression")

I realized f2fs and btrfs also have the same code and need to
be updated.

Regards,

-- 
Goldwyn

[PATCH 1/3] btrfs: fix inode rwsem regression

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
Apparently our current rwsem code doesn't like doing the trylock, then
lock for real scheme.  So change our read/write methods to just do the
trylock for the RWF_NOWAIT case.

Fixes: edf064e7c6fe ("btrfs: nowait aio support")
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/file.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 58a18ed11546..651b2b1f4219 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1893,9 +1893,10 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	    (iocb->ki_flags & IOCB_NOWAIT))
 		return -EOPNOTSUPP;
 
-	if (!inode_trylock(inode)) {
-		if (iocb->ki_flags & IOCB_NOWAIT)
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock(inode))
 			return -EAGAIN;
+	} else {
 		inode_lock(inode);
 	}
 
-- 
2.16.4

Re: [PATCH 1/3] btrfs: fix inode rwsem regression

[David Sterba]

On Wed, Sep 11, 2019 at 11:45:15AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> Apparently our current rwsem code doesn't like doing the trylock, then
> lock for real scheme.  So change our read/write methods to just do the
> trylock for the RWF_NOWAIT case.
> 
> Fixes: edf064e7c6fe ("btrfs: nowait aio support")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

The subject seems to be a bit confusing so I'll update it, otherwise
patch added to devel queue, thanks.

[PATCH 2/3] ext4: fix inode rwsem regression

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
Apparently our current rwsem code doesn't like doing the trylock, then
lock for real scheme.  So change our read/write methods to just do the
trylock for the RWF_NOWAIT case.

Fixes: 728fbc0e10b7 ("ext4: nowait aio support")
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ext4/file.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 70b0438dbc94..d5b2d0cc325d 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -40,11 +40,13 @@ static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	struct inode *inode = file_inode(iocb->ki_filp);
 	ssize_t ret;
 
-	if (!inode_trylock_shared(inode)) {
-		if (iocb->ki_flags & IOCB_NOWAIT)
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock_shared(inode))
 			return -EAGAIN;
+	} else {
 		inode_lock_shared(inode);
 	}
+
 	/*
 	 * Recheck under inode lock - at this point we are sure it cannot
 	 * change anymore
@@ -190,11 +192,13 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	struct inode *inode = file_inode(iocb->ki_filp);
 	ssize_t ret;
 
-	if (!inode_trylock(inode)) {
-		if (iocb->ki_flags & IOCB_NOWAIT)
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock(inode))
 			return -EAGAIN;
+	} else {
 		inode_lock(inode);
 	}
+
 	ret = ext4_write_checks(iocb, from);
 	if (ret <= 0)
 		goto out;
@@ -233,9 +237,10 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (!o_direct && (iocb->ki_flags & IOCB_NOWAIT))
 		return -EOPNOTSUPP;
 
-	if (!inode_trylock(inode)) {
-		if (iocb->ki_flags & IOCB_NOWAIT)
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock(inode))
 			return -EAGAIN;
+	} else {
 		inode_lock(inode);
 	}
 
-- 
2.16.4

Re: [PATCH 2/3] ext4: fix inode rwsem regression

[Ritesh Harjani]

cc'd Matthew as well.

On 9/11/19 10:15 PM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> Apparently our current rwsem code doesn't like doing the trylock, then
> lock for real scheme.  So change our read/write methods to just do the
> trylock for the RWF_NOWAIT case.
> 
> Fixes: 728fbc0e10b7 ("ext4: nowait aio support")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

This patch will conflict with recent iomap patch series.
So if this is getting queued up before, so iomap patch series will
need to rebase and factor these changes in the new APIs.

Otherwise looks good to me!

Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>


> ---
>   fs/ext4/file.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 70b0438dbc94..d5b2d0cc325d 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -40,11 +40,13 @@ static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
>   	struct inode *inode = file_inode(iocb->ki_filp);
>   	ssize_t ret;
>   
> -	if (!inode_trylock_shared(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock_shared(inode))
>   			return -EAGAIN;
> +	} else {
>   		inode_lock_shared(inode);
>   	}
> +
>   	/*
>   	 * Recheck under inode lock - at this point we are sure it cannot
>   	 * change anymore
> @@ -190,11 +192,13 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   	struct inode *inode = file_inode(iocb->ki_filp);
>   	ssize_t ret;
>   
> -	if (!inode_trylock(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock(inode))
>   			return -EAGAIN;
> +	} else {
>   		inode_lock(inode);
>   	}
> +
>   	ret = ext4_write_checks(iocb, from);
>   	if (ret <= 0)
>   		goto out;
> @@ -233,9 +237,10 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   	if (!o_direct && (iocb->ki_flags & IOCB_NOWAIT))
>   		return -EOPNOTSUPP;
>   
> -	if (!inode_trylock(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock(inode))
>   			return -EAGAIN;
> +	} else {
>   		inode_lock(inode);
>   	}
>   
> 

Re: [PATCH 2/3] ext4: fix inode rwsem regression

[Matthew Bobrowski]

On Thu, Sep 12, 2019 at 02:22:35PM +0530, Ritesh Harjani wrote:
> cc'd Matthew as well.
> 
> > This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> > Apparently our current rwsem code doesn't like doing the trylock, then
> > lock for real scheme.  So change our read/write methods to just do the
> > trylock for the RWF_NOWAIT case.
> > 
> > Fixes: 728fbc0e10b7 ("ext4: nowait aio support")
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This patch will conflict with recent iomap patch series.
> So if this is getting queued up before, so iomap patch series will
> need to rebase and factor these changes in the new APIs.

Noted. I've been keeping my eye on this thread, so I'm aware of this.

--<M>--

Re: [PATCH 2/3] ext4: fix inode rwsem regression

[Jan Kara]

On Wed 11-09-19 11:45:16, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> Apparently our current rwsem code doesn't like doing the trylock, then
> lock for real scheme.  So change our read/write methods to just do the
> trylock for the RWF_NOWAIT case.
> 
> Fixes: 728fbc0e10b7 ("ext4: nowait aio support")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Thanks for fixing this! The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

BTW, I've also added Ted as ext4 maintainer to CC.

								Honza

> ---
>  fs/ext4/file.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 70b0438dbc94..d5b2d0cc325d 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -40,11 +40,13 @@ static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	ssize_t ret;
>  
> -	if (!inode_trylock_shared(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock_shared(inode))
>  			return -EAGAIN;
> +	} else {
>  		inode_lock_shared(inode);
>  	}
> +
>  	/*
>  	 * Recheck under inode lock - at this point we are sure it cannot
>  	 * change anymore
> @@ -190,11 +192,13 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	ssize_t ret;
>  
> -	if (!inode_trylock(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock(inode))
>  			return -EAGAIN;
> +	} else {
>  		inode_lock(inode);
>  	}
> +
>  	ret = ext4_write_checks(iocb, from);
>  	if (ret <= 0)
>  		goto out;
> @@ -233,9 +237,10 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	if (!o_direct && (iocb->ki_flags & IOCB_NOWAIT))
>  		return -EOPNOTSUPP;
>  
> -	if (!inode_trylock(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock(inode))
>  			return -EAGAIN;
> +	} else {
>  		inode_lock(inode);
>  	}
>  
> -- 
> 2.16.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/3] ext4: fix inode rwsem regression

[Theodore Y. Ts'o]

On Mon, Sep 23, 2019 at 12:10:42PM +0200, Jan Kara wrote:
> On Wed 11-09-19 11:45:16, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> > Apparently our current rwsem code doesn't like doing the trylock, then
> > lock for real scheme.  So change our read/write methods to just do the
> > trylock for the RWF_NOWAIT case.
> > 
> > Fixes: 728fbc0e10b7 ("ext4: nowait aio support")
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Thanks for fixing this! The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> BTW, I've also added Ted as ext4 maintainer to CC.

Thanks, I've been following along, and once the merge window is over
I'll start going through the patch backlog.

Cheers,

						- Ted

[PATCH 3/3] f2fs: fix inode rwsem regression

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
Apparently our current rwsem code doesn't like doing the trylock, then
lock for real scheme.  So change our read/write methods to just do the
trylock for the RWF_NOWAIT case.

We don't need a check for IOCB_NOWAIT and !direct-IO because it
is checked in generic_write_checks().

Fixes: b91050a80cec ("f2fs: add nowait aio support")
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/f2fs/file.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3e58a6f697dd..c6f3ef815c05 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3134,16 +3134,12 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 	}
 
-	if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (!inode_trylock(inode)) {
-		if (iocb->ki_flags & IOCB_NOWAIT) {
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock(inode)) {
 			ret = -EAGAIN;
 			goto out;
 		}
+	} else {
 		inode_lock(inode);
 	}
 
-- 
2.16.4

Re: [PATCH 3/3] f2fs: fix inode rwsem regression

[Chao Yu]

On 2019/9/12 0:45, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> Apparently our current rwsem code doesn't like doing the trylock, then
> lock for real scheme.  So change our read/write methods to just do the
> trylock for the RWF_NOWAIT case.
> 
> We don't need a check for IOCB_NOWAIT and !direct-IO because it
> is checked in generic_write_checks().
> 
> Fixes: b91050a80cec ("f2fs: add nowait aio support")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/f2fs/file.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 3e58a6f697dd..c6f3ef815c05 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3134,16 +3134,12 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  		goto out;
>  	}
>  
> -	if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}

We have removed above redundant check, could you rebase to dev-test branch of
Jaegeuk's git tree?

Otherwise it looks good to me.

Thanks,

> -
> -	if (!inode_trylock(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT) {
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock(inode)) {
>  			ret = -EAGAIN;
>  			goto out;
>  		}
> +	} else {
>  		inode_lock(inode);
>  	}
>  
> 

Re: [PATCH 3/3] f2fs: fix inode rwsem regression

[Jaegeuk Kim]

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=ebef4d7eda0d06a6ab6dc0f9e9f848276e605962

Merged. Thanks,

On 09/11, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
> Apparently our current rwsem code doesn't like doing the trylock, then
> lock for real scheme.  So change our read/write methods to just do the
> trylock for the RWF_NOWAIT case.
> 
> We don't need a check for IOCB_NOWAIT and !direct-IO because it
> is checked in generic_write_checks().
> 
> Fixes: b91050a80cec ("f2fs: add nowait aio support")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/f2fs/file.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 3e58a6f697dd..c6f3ef815c05 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3134,16 +3134,12 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  		goto out;
>  	}
>  
> -	if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	if (!inode_trylock(inode)) {
> -		if (iocb->ki_flags & IOCB_NOWAIT) {
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock(inode)) {
>  			ret = -EAGAIN;
>  			goto out;
>  		}
> +	} else {
>  		inode_lock(inode);
>  	}
>  
> -- 
> 2.16.4

Re: [PATCH 3/3] f2fs: fix inode rwsem regression

[Chao Yu]

On 2019/9/14 3:46, Jaegeuk Kim wrote:
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=ebef4d7eda0d06a6ab6dc0f9e9f848276e605962

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 
> Merged. Thanks,
> 
> On 09/11, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> This is similar to 942491c9e6d6 ("xfs: fix AIM7 regression")
>> Apparently our current rwsem code doesn't like doing the trylock, then
>> lock for real scheme.  So change our read/write methods to just do the
>> trylock for the RWF_NOWAIT case.
>>
>> We don't need a check for IOCB_NOWAIT and !direct-IO because it
>> is checked in generic_write_checks().
>>
>> Fixes: b91050a80cec ("f2fs: add nowait aio support")
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>>  fs/f2fs/file.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 3e58a6f697dd..c6f3ef815c05 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -3134,16 +3134,12 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>  		goto out;
>>  	}
>>  
>> -	if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT)) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>> -	if (!inode_trylock(inode)) {
>> -		if (iocb->ki_flags & IOCB_NOWAIT) {
>> +	if (iocb->ki_flags & IOCB_NOWAIT) {
>> +		if (!inode_trylock(inode)) {
>>  			ret = -EAGAIN;
>>  			goto out;
>>  		}
>> +	} else {
>>  		inode_lock(inode);
>>  	}
>>  
>> -- 
>> 2.16.4
> .
> 

Re: Odd locking pattern introduced as part of "nowait aio support"

[Goldwyn Rodrigues]

On 14:04 11/09, Dave Chinner wrote:
> On Tue, Sep 10, 2019 at 03:33:27PM -0700, Andres Freund wrote:
> > Hi,
> > 
> > Especially with buffered io it's fairly easy to hit contention on the
> > inode lock, during writes. With something like io_uring, it's even
> > easier, because it currently (but see [1]) farms out buffered writes to
> > workers, which then can easily contend on the inode lock, even if only
> > one process submits writes.  But I've seen it in plenty other cases too.
> > 
> > Looking at the code I noticed that several parts of the "nowait aio
> > support" (cf 728fbc0e10b7f3) series introduced code like:
> > 
> > static ssize_t
> > ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > {
> > ...
> > 	if (!inode_trylock(inode)) {
> > 		if (iocb->ki_flags & IOCB_NOWAIT)
> > 			return -EAGAIN;
> > 		inode_lock(inode);
> > 	}
> 
> The ext4 code is just buggy here - we don't support RWF_NOWAIT on
> buffered writes. Buffered reads, and dio/dax reads and writes, yes,
> but not buffered writes because they are almost guaranteed to block
> somewhere. See xfs_file_buffered_aio_write():
> 
> 	if (iocb->ki_flags & IOCB_NOWAIT)
> 		return -EOPNOTSUPP;
> 
> generic_write_checks() will also reject IOCB_NOWAIT on buffered
> writes, so that code in ext4 is likely in the wrong place...

Yes, but inode_trylock is checking if we can get inode sem immidiately,
and if not bail, instead of waiting for the sem, as opposed to rejecting
bufferd I/O nowait writes.

generic_write_checks() has the checks which disallows nowait without direct
writes, so we can do away with those checks in ext4_file_write_iter().
However, the return code in generic_write_checks() is -EINVAL and
-ENOTSUPP in ext4_file_write_iter(). Removing the check in write_iter()
will change the error code to -EINVAL from -EOPNOTSUPP.


-- 
Goldwyn