diff mbox series

[3/8] iomap: Move lockdep_assert_held() to iomap_dio_rw() calls

Message ID 20191213195750.32184-4-rgoldwyn@suse.de (mailing list archive)
State New, archived
Headers show
Series btrfs direct-io using iomap | expand

Commit Message

Goldwyn Rodrigues Dec. 13, 2019, 7:57 p.m. UTC
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Filesystems such as btrfs can perform direct I/O without holding the
inode->i_rwsem in some of the cases like writing within i_size.
So, remove the check for lockdep_assert_held() in iomap_dio_rw()

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/iomap/direct-io.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Darrick J. Wong Dec. 14, 2019, 12:32 a.m. UTC | #1
On Fri, Dec 13, 2019 at 01:57:45PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Filesystems such as btrfs can perform direct I/O without holding the
> inode->i_rwsem in some of the cases like writing within i_size.
> So, remove the check for lockdep_assert_held() in iomap_dio_rw()
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Mildly scary, but OTOH filesystems are supposed to take care of their
own locking before calling iomap...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/iomap/direct-io.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 1a3bf3bd86fb..41c1e7c20a1f 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -415,8 +415,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	struct blk_plug plug;
>  	struct iomap_dio *dio;
>  
> -	lockdep_assert_held(&inode->i_rwsem);
> -
>  	if (!count)
>  		return 0;
>  
> -- 
> 2.16.4
>
Darrick J. Wong Dec. 18, 2019, 2:04 a.m. UTC | #2
On Fri, Dec 13, 2019 at 01:57:45PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Filesystems such as btrfs can perform direct I/O without holding the
> inode->i_rwsem in some of the cases like writing within i_size.
> So, remove the check for lockdep_assert_held() in iomap_dio_rw()
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

/me wishes there was a way for iomap to verify that the fs has indeed
taken /some/ measure to ensure correct concurrent operations, but that's
probably just asking for liar functions. :)

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/iomap/direct-io.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 1a3bf3bd86fb..41c1e7c20a1f 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -415,8 +415,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	struct blk_plug plug;
>  	struct iomap_dio *dio;
>  
> -	lockdep_assert_held(&inode->i_rwsem);
> -
>  	if (!count)
>  		return 0;
>  
> -- 
> 2.16.4
>
Christoph Hellwig Dec. 21, 2019, 1:41 p.m. UTC | #3
On Fri, Dec 13, 2019 at 01:57:45PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Filesystems such as btrfs can perform direct I/O without holding the
> inode->i_rwsem in some of the cases like writing within i_size.
> So, remove the check for lockdep_assert_held() in iomap_dio_rw()

As said last time: in the callers the assert is completely pointless,
as it is always very close to taking the lock.  This was just intended
to deal with callers not adhering to the iomap_dio_rw calling
conventins, and moving the assert to the calllers doesn't help with
that at all.

So if you think you need to remove it do just that and write a changelog
explaining the why much better.
Christoph Hellwig Dec. 21, 2019, 1:42 p.m. UTC | #4
On Sat, Dec 21, 2019 at 05:41:18AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 13, 2019 at 01:57:45PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Filesystems such as btrfs can perform direct I/O without holding the
> > inode->i_rwsem in some of the cases like writing within i_size.
> > So, remove the check for lockdep_assert_held() in iomap_dio_rw()
> 
> As said last time: in the callers the assert is completely pointless,
> as it is always very close to taking the lock.  This was just intended
> to deal with callers not adhering to the iomap_dio_rw calling
> conventins, and moving the assert to the calllers doesn't help with
> that at all.

And the actual patch even does that, it just is the commit log that
doesn't match it..
Darrick J. Wong Dec. 21, 2019, 6:02 p.m. UTC | #5
On Sat, Dec 21, 2019 at 05:41:18AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 13, 2019 at 01:57:45PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Filesystems such as btrfs can perform direct I/O without holding the
> > inode->i_rwsem in some of the cases like writing within i_size.
> > So, remove the check for lockdep_assert_held() in iomap_dio_rw()
> 
> As said last time: in the callers the assert is completely pointless,
> as it is always very close to taking the lock.  This was just intended
> to deal with callers not adhering to the iomap_dio_rw calling
> conventins, and moving the assert to the calllers doesn't help with
> that at all.
> 
> So if you think you need to remove it do just that and write a changelog
> explaining the why much better.

Uh... that's exactly what this patch does, and the commit message says
that btrfs doesn't need to hold i_rwsem to guarantee concurrency
correctness.

Hm, but maybe the commit message is simply too subtle here?  How about:

"Filesystems do not necessarily need i_rwsem to ensure correct operation
with multiple threads, so remove the i_rwsem assertion in iomap_dio_rw.
For example, btrfs can perform directio within i_size without needing to
hold i_rwsem."

--D
diff mbox series

Patch

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 1a3bf3bd86fb..41c1e7c20a1f 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -415,8 +415,6 @@  iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct blk_plug plug;
 	struct iomap_dio *dio;
 
-	lockdep_assert_held(&inode->i_rwsem);
-
 	if (!count)
 		return 0;