[Cluster-devel] iomap_dio_rw: lockdep_assert_held(&inode->i_rwsem)

[Cluster-devel] iomap_dio_rw: lockdep_assert_held(&inode->i_rwsem)

[Andreas Gruenbacher]

Hi Christoph,

there's an assertion that the inode rwsem is taken in iomap_dio_rw
which currently triggers on gfs2 via gfs2_file_read_iter ->
gfs2_file_direct_read -> iomap_dio_rw in fs/gfs2/file.c.

We're not using inode_[un]lock_shared on that code path because gfs2
uses its own cluster-wide locking already. Is there a reason why the
inode needs to be locked locally anyway? If not, I'd like to avoid
adding unnecessary lock taking in gfs2_file_direct_read just to
silence iomap_dio_rw.

Thanks,
Andreas

[Cluster-devel] iomap_dio_rw: lockdep_assert_held(&inode->i_rwsem)

[Andreas Gruenbacher]

Hi Christoph,

I've tried to get your feedback on the lockdep_assert_held in
iomap_dio_rw in January and didn't hear back from you. Could you
please have a look?

On Tue, 22 Jan 2019 at 22:26, Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> Hi Christoph,
>
> there's an assertion that the inode rwsem is taken in iomap_dio_rw
> which currently triggers on gfs2 via gfs2_file_read_iter ->
> gfs2_file_direct_read -> iomap_dio_rw in fs/gfs2/file.c.
>
> We're not using inode_[un]lock_shared on that code path because gfs2
> uses its own cluster-wide locking already. Is there a reason why the
> inode needs to be locked locally anyway? If not, I'd like to avoid
> adding unnecessary lock taking in gfs2_file_direct_read just to
> silence iomap_dio_rw.

Thanks,
Andreas

[Cluster-devel] iomap_dio_rw: lockdep_assert_held(&inode->i_rwsem)

[Christoph Hellwig]

On Wed, Feb 27, 2019 at 07:03:59PM +0100, Andreas Gruenbacher wrote:
> Hi Christoph,
> 
> I've tried to get your feedback on the lockdep_assert_held in
> iomap_dio_rw in January and didn't hear back from you. Could you
> please have a look?

Well, I guess we can drop the assert.  But I'd really like to retain
a comment that we require the caller to have a lock - unlike what
most filesystems do with the old generic direct I/O code, which
leads to all kinds of issues.

[Cluster-devel] iomap_dio_rw: lockdep_assert_held(&inode->i_rwsem)

[Andreas Gruenbacher]

On Fri, 8 Mar 2019 at 08:31, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Feb 27, 2019 at 07:03:59PM +0100, Andreas Gruenbacher wrote:
> > Hi Christoph,
> >
> > I've tried to get your feedback on the lockdep_assert_held in
> > iomap_dio_rw in January and didn't hear back from you. Could you
> > please have a look?
>
> Well, I guess we can drop the assert.  But I'd really like to retain
> a comment that we require the caller to have a lock - unlike what
> most filesystems do with the old generic direct I/O code, which
> leads to all kinds of issues.

Something like the below perhaps?

Thanks,
Andreas

--

[PATCH] iomap: iomap_dio_rw: Allow filesystem specific locking

Allow filesystem specific locking for reads via iomap_dio_rw.  Document that
some form of locking is required.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/iomap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index a3088fae567ba..03b6ccc2b94fc 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1793,6 +1793,9 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
  * can be mapped into multiple disjoint IOs and only a subset of the IOs issued
  * may be pure data writes. In that case, we still need to do a full data sync
  * completion.
+ *
+ * The caller must hold the inode lock or an equivalent filesystem specific
+ * lock.
  */
 ssize_t
 iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
@@ -1807,7 +1810,8 @@ 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 (iov_iter_rw(iter) == WRITE)
+		lockdep_assert_held(&inode->i_rwsem);
 
 	if (!count)
 		return 0;
-- 
2.20.1