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