* 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 related [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
* [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 related [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 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
* [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 related [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 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
end of thread, other threads:[~2019-09-23 13:19 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).