Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* Re: Odd locking pattern introduced as part of "nowait aio support"
       [not found] ` <20190911040420.GB27547@dread.disaster.area>
@ 2019-09-11  9:39   ` Andres Freund
  2019-09-11 10:19     ` Christoph Hellwig
                       ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Andres Freund @ 2019-09-11  9:39 UTC (permalink / raw)
  To: Dave Chinner, David Sterba
  Cc: Goldwyn Rodrigues, linux-fsdevel, jack, hch, linux-ext4, linux-btrfs

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Odd locking pattern introduced as part of "nowait aio support"
  2019-09-11  9:39   ` Odd locking pattern introduced as part of "nowait aio support" Andres Freund
@ 2019-09-11 10:19     ` Christoph Hellwig
  2019-09-11 10:31     ` Ritesh Harjani
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2019-09-11 10:19 UTC (permalink / raw)
  To: Andres Freund
  Cc: Dave Chinner, David Sterba, Goldwyn Rodrigues, linux-fsdevel,
	jack, hch, linux-ext4, linux-btrfs

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

Send a patch..

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Odd locking pattern introduced as part of "nowait aio support"
  2019-09-11  9:39   ` Odd locking pattern introduced as part of "nowait aio support" Andres Freund
  2019-09-11 10:19     ` Christoph Hellwig
@ 2019-09-11 10:31     ` Ritesh Harjani
  2019-09-11 10:55     ` Goldwyn Rodrigues
  2019-09-11 16:45     ` Fix inode sem regression for nowait Goldwyn Rodrigues
  3 siblings, 0 replies; 16+ messages in thread
From: Ritesh Harjani @ 2019-09-11 10:31 UTC (permalink / raw)
  To: Andres Freund, Dave Chinner, David Sterba
  Cc: Goldwyn Rodrigues, linux-fsdevel, jack, hch, linux-ext4, linux-btrfs

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
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Odd locking pattern introduced as part of "nowait aio support"
  2019-09-11  9:39   ` Odd locking pattern introduced as part of "nowait aio support" Andres Freund
  2019-09-11 10:19     ` Christoph Hellwig
  2019-09-11 10:31     ` Ritesh Harjani
@ 2019-09-11 10:55     ` Goldwyn Rodrigues
  2019-09-11 16:45     ` Fix inode sem regression for nowait Goldwyn Rodrigues
  3 siblings, 0 replies; 16+ messages in thread
From: Goldwyn Rodrigues @ 2019-09-11 10:55 UTC (permalink / raw)
  To: Andres Freund
  Cc: Dave Chinner, David Sterba, linux-fsdevel, jack, hch, linux-ext4,
	linux-btrfs

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Fix inode sem regression for nowait
  2019-09-11  9:39   ` Odd locking pattern introduced as part of "nowait aio support" Andres Freund
                       ` (2 preceding siblings ...)
  2019-09-11 10:55     ` Goldwyn Rodrigues
@ 2019-09-11 16:45     ` Goldwyn Rodrigues
  2019-09-11 16:45       ` [PATCH 1/3] btrfs: fix inode rwsem regression Goldwyn Rodrigues
                         ` (2 more replies)
  3 siblings, 3 replies; 16+ messages in thread
From: Goldwyn Rodrigues @ 2019-09-11 16:45 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: linux-ext4, linux-btrfs, hch, andres, david, riteshh, linux-f2fs-devel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] btrfs: fix inode rwsem regression
  2019-09-11 16:45     ` Fix inode sem regression for nowait Goldwyn Rodrigues
@ 2019-09-11 16:45       ` Goldwyn Rodrigues
  2019-09-11 17:21         ` David Sterba
  2019-09-11 16:45       ` [PATCH 2/3] ext4: " Goldwyn Rodrigues
  2019-09-11 16:45       ` [PATCH 3/3] f2fs: " Goldwyn Rodrigues
  2 siblings, 1 reply; 16+ messages in thread
From: Goldwyn Rodrigues @ 2019-09-11 16:45 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: linux-ext4, linux-btrfs, hch, andres, david, riteshh,
	linux-f2fs-devel, 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/3] ext4: fix inode rwsem regression
  2019-09-11 16:45     ` Fix inode sem regression for nowait Goldwyn Rodrigues
  2019-09-11 16:45       ` [PATCH 1/3] btrfs: fix inode rwsem regression Goldwyn Rodrigues
@ 2019-09-11 16:45       ` " Goldwyn Rodrigues
  2019-09-12  8:52         ` Ritesh Harjani
  2019-09-23 10:10         ` Jan Kara
  2019-09-11 16:45       ` [PATCH 3/3] f2fs: " Goldwyn Rodrigues
  2 siblings, 2 replies; 16+ messages in thread
From: Goldwyn Rodrigues @ 2019-09-11 16:45 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: linux-ext4, linux-btrfs, hch, andres, david, riteshh,
	linux-f2fs-devel, 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/3] f2fs: fix inode rwsem regression
  2019-09-11 16:45     ` Fix inode sem regression for nowait Goldwyn Rodrigues
  2019-09-11 16:45       ` [PATCH 1/3] btrfs: fix inode rwsem regression Goldwyn Rodrigues
  2019-09-11 16:45       ` [PATCH 2/3] ext4: " Goldwyn Rodrigues
@ 2019-09-11 16:45       ` " Goldwyn Rodrigues
  2019-09-12  6:17         ` Chao Yu
  2019-09-13 19:46         ` Jaegeuk Kim
  2 siblings, 2 replies; 16+ messages in thread
From: Goldwyn Rodrigues @ 2019-09-11 16:45 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: linux-ext4, linux-btrfs, hch, andres, david, riteshh,
	linux-f2fs-devel, 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] btrfs: fix inode rwsem regression
  2019-09-11 16:45       ` [PATCH 1/3] btrfs: fix inode rwsem regression Goldwyn Rodrigues
@ 2019-09-11 17:21         ` David Sterba
  0 siblings, 0 replies; 16+ messages in thread
From: David Sterba @ 2019-09-11 17:21 UTC (permalink / raw)
  To: Goldwyn Rodrigues
  Cc: linux-fsdevel, linux-ext4, linux-btrfs, hch, andres, david,
	riteshh, linux-f2fs-devel, Goldwyn Rodrigues

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] f2fs: fix inode rwsem regression
  2019-09-11 16:45       ` [PATCH 3/3] f2fs: " Goldwyn Rodrigues
@ 2019-09-12  6:17         ` Chao Yu
  2019-09-13 19:46         ` Jaegeuk Kim
  1 sibling, 0 replies; 16+ messages in thread
From: Chao Yu @ 2019-09-12  6:17 UTC (permalink / raw)
  To: Goldwyn Rodrigues, linux-fsdevel
  Cc: linux-ext4, linux-btrfs, hch, andres, david, riteshh,
	linux-f2fs-devel, Goldwyn Rodrigues

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] ext4: fix inode rwsem regression
  2019-09-11 16:45       ` [PATCH 2/3] ext4: " Goldwyn Rodrigues
@ 2019-09-12  8:52         ` Ritesh Harjani
  2019-09-12  9:26           ` Matthew Bobrowski
  2019-09-23 10:10         ` Jan Kara
  1 sibling, 1 reply; 16+ messages in thread
From: Ritesh Harjani @ 2019-09-12  8:52 UTC (permalink / raw)
  To: Goldwyn Rodrigues, linux-fsdevel
  Cc: linux-ext4, linux-btrfs, hch, andres, david, linux-f2fs-devel,
	Goldwyn Rodrigues, Matthew Bobrowski, aneesh.kumar

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] ext4: fix inode rwsem regression
  2019-09-12  8:52         ` Ritesh Harjani
@ 2019-09-12  9:26           ` Matthew Bobrowski
  0 siblings, 0 replies; 16+ messages in thread
From: Matthew Bobrowski @ 2019-09-12  9:26 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: Goldwyn Rodrigues, linux-fsdevel, linux-ext4, linux-btrfs, hch,
	andres, david, linux-f2fs-devel, Goldwyn Rodrigues, aneesh.kumar

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] f2fs: fix inode rwsem regression
  2019-09-11 16:45       ` [PATCH 3/3] f2fs: " Goldwyn Rodrigues
  2019-09-12  6:17         ` Chao Yu
@ 2019-09-13 19:46         ` Jaegeuk Kim
  2019-09-16  1:16           ` Chao Yu
  1 sibling, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2019-09-13 19:46 UTC (permalink / raw)
  To: Goldwyn Rodrigues
  Cc: linux-fsdevel, linux-ext4, linux-btrfs, hch, andres, david,
	riteshh, linux-f2fs-devel, Goldwyn Rodrigues

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] f2fs: fix inode rwsem regression
  2019-09-13 19:46         ` Jaegeuk Kim
@ 2019-09-16  1:16           ` Chao Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2019-09-16  1:16 UTC (permalink / raw)
  To: Jaegeuk Kim, Goldwyn Rodrigues
  Cc: linux-fsdevel, linux-ext4, linux-btrfs, hch, andres, david,
	riteshh, linux-f2fs-devel, Goldwyn Rodrigues

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] ext4: fix inode rwsem regression
  2019-09-11 16:45       ` [PATCH 2/3] ext4: " Goldwyn Rodrigues
  2019-09-12  8:52         ` Ritesh Harjani
@ 2019-09-23 10:10         ` Jan Kara
  2019-09-23 13:18           ` Theodore Y. Ts'o
  1 sibling, 1 reply; 16+ messages in thread
From: Jan Kara @ 2019-09-23 10:10 UTC (permalink / raw)
  To: Goldwyn Rodrigues
  Cc: linux-fsdevel, linux-ext4, linux-btrfs, hch, andres, david,
	riteshh, linux-f2fs-devel, Ted Tso, Goldwyn Rodrigues

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] ext4: fix inode rwsem regression
  2019-09-23 10:10         ` Jan Kara
@ 2019-09-23 13:18           ` Theodore Y. Ts'o
  0 siblings, 0 replies; 16+ messages in thread
From: Theodore Y. Ts'o @ 2019-09-23 13:18 UTC (permalink / raw)
  To: Jan Kara
  Cc: Goldwyn Rodrigues, linux-fsdevel, linux-ext4, linux-btrfs, hch,
	andres, david, riteshh, linux-f2fs-devel, Goldwyn Rodrigues

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190910223327.mnegfoggopwqqy33@alap3.anarazel.de>
     [not found] ` <20190911040420.GB27547@dread.disaster.area>
2019-09-11  9:39   ` Odd locking pattern introduced as part of "nowait aio support" Andres Freund
2019-09-11 10:19     ` Christoph Hellwig
2019-09-11 10:31     ` Ritesh Harjani
2019-09-11 10:55     ` Goldwyn Rodrigues
2019-09-11 16:45     ` Fix inode sem regression for nowait Goldwyn Rodrigues
2019-09-11 16:45       ` [PATCH 1/3] btrfs: fix inode rwsem regression Goldwyn Rodrigues
2019-09-11 17:21         ` David Sterba
2019-09-11 16:45       ` [PATCH 2/3] ext4: " Goldwyn Rodrigues
2019-09-12  8:52         ` Ritesh Harjani
2019-09-12  9:26           ` Matthew Bobrowski
2019-09-23 10:10         ` Jan Kara
2019-09-23 13:18           ` Theodore Y. Ts'o
2019-09-11 16:45       ` [PATCH 3/3] f2fs: " Goldwyn Rodrigues
2019-09-12  6:17         ` Chao Yu
2019-09-13 19:46         ` Jaegeuk Kim
2019-09-16  1:16           ` Chao Yu

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox