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

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