diff mbox

[1/3] fs: add initial bh_result->b_private value to __blockdev_direct_IO()

Message ID 9a09b9878759e377b138336886b3e2c6e5d7eae9.1525933432.git.osandov@fb.com (mailing list archive)
State New, archived
Headers show

Commit Message

Omar Sandoval May 11, 2018, 6:30 a.m. UTC
From: Omar Sandoval <osandov@fb.com>

Btrfs abuses current->journal_info in btrfs_direct_IO() in order to pass
around some state to get_block() and submit_io(). The generic DIO code
already provides bh_result->b_private as a way to pass data between
calls to get_block() and end_io(), but it is always initialized to NULL.
Let's allow an initial value to be passed in from the caller of
__blockdev_direct_IO().

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/inode.c   | 6 +++---
 fs/direct-io.c     | 9 +++++----
 fs/ext4/inode.c    | 6 +++---
 fs/gfs2/aops.c     | 2 +-
 fs/ocfs2/aops.c    | 5 ++---
 include/linux/fs.h | 8 ++++----
 6 files changed, 18 insertions(+), 18 deletions(-)

Comments

Al Viro May 11, 2018, 8:05 p.m. UTC | #1
On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
>  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
>  		      struct block_device *bdev, struct iov_iter *iter,
>  		      get_block_t get_block, dio_iodone_t end_io,
> -		      dio_submit_t submit_io, int flags)
> +		      dio_submit_t submit_io, int flags, void *private)

Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
is obscene, but so is this ;-/
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Sandoval May 11, 2018, 8:30 p.m. UTC | #2
On Fri, May 11, 2018 at 09:05:38PM +0100, Al Viro wrote:
> On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
> >  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> >  		      struct block_device *bdev, struct iov_iter *iter,
> >  		      get_block_t get_block, dio_iodone_t end_io,
> > -		      dio_submit_t submit_io, int flags)
> > +		      dio_submit_t submit_io, int flags, void *private)
> 
> Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
> is obscene, but so is this ;-/

So looking at these one by one, obviously needed:

- iocb
- inode
- iter

bdev is almost always inode->i_sb->s_bdev, except for Btrfs :(

These could _maybe_ go in struct kiocb:

- flags could maybe be folded into ki_flags
- private could maybe go in iocb->private, but I haven't yet read
  through to figure out if we're already using iocb->private for direct
  I/O

That leaves the callbacks, get_block, end_io, and submit_io. Perhaps we
can add those to inode_operations?

Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro May 11, 2018, 8:32 p.m. UTC | #3
On Fri, May 11, 2018 at 01:30:01PM -0700, Omar Sandoval wrote:
> On Fri, May 11, 2018 at 09:05:38PM +0100, Al Viro wrote:
> > On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
> > >  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> > >  		      struct block_device *bdev, struct iov_iter *iter,
> > >  		      get_block_t get_block, dio_iodone_t end_io,
> > > -		      dio_submit_t submit_io, int flags)
> > > +		      dio_submit_t submit_io, int flags, void *private)
> > 
> > Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
> > is obscene, but so is this ;-/
> 
> So looking at these one by one, obviously needed:
> 
> - iocb
> - inode
> - iter
> 
> bdev is almost always inode->i_sb->s_bdev, except for Btrfs :(
> 
> These could _maybe_ go in struct kiocb:
> 
> - flags could maybe be folded into ki_flags
> - private could maybe go in iocb->private, but I haven't yet read
>   through to figure out if we're already using iocb->private for direct
>   I/O
> 
> That leaves the callbacks, get_block, end_io, and submit_io. Perhaps we
> can add those to inode_operations?

Or, perhaps, btrfs shouldn't be using the common helper?  The question
is not where to stash the bits and pieces - it's how unreadable the callers
are and how much boilerplate/hidden information is involved...
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Sandoval May 11, 2018, 8:41 p.m. UTC | #4
On Fri, May 11, 2018 at 09:32:28PM +0100, Al Viro wrote:
> On Fri, May 11, 2018 at 01:30:01PM -0700, Omar Sandoval wrote:
> > On Fri, May 11, 2018 at 09:05:38PM +0100, Al Viro wrote:
> > > On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
> > > >  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> > > >  		      struct block_device *bdev, struct iov_iter *iter,
> > > >  		      get_block_t get_block, dio_iodone_t end_io,
> > > > -		      dio_submit_t submit_io, int flags)
> > > > +		      dio_submit_t submit_io, int flags, void *private)
> > > 
> > > Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
> > > is obscene, but so is this ;-/
> > 
> > So looking at these one by one, obviously needed:
> > 
> > - iocb
> > - inode
> > - iter
> > 
> > bdev is almost always inode->i_sb->s_bdev, except for Btrfs :(
> > 
> > These could _maybe_ go in struct kiocb:
> > 
> > - flags could maybe be folded into ki_flags
> > - private could maybe go in iocb->private, but I haven't yet read
> >   through to figure out if we're already using iocb->private for direct
> >   I/O

Modifying kiocb isn't going to pan out, it's constructed way up in the
stack so that'd be a mess.

> > That leaves the callbacks, get_block, end_io, and submit_io. Perhaps we
> > can add those to inode_operations?
> 
> Or, perhaps, btrfs shouldn't be using the common helper?  The question
> is not where to stash the bits and pieces - it's how unreadable the callers
> are and how much boilerplate/hidden information is involved...

I need to call through to do_blockdev_direct_IO() eventually, I'm sure
no one wants me to reimplement the 200 lines in there :) so I'd be happy
to add a separate helper that only Btrfs uses, but if we're going to
call do_blockdev_direct_IO() eventually then we still need the 9
arguments in some form. Am I misunderstanding?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba May 14, 2018, 4:35 p.m. UTC | #5
On Fri, May 11, 2018 at 01:30:01PM -0700, Omar Sandoval wrote:
> On Fri, May 11, 2018 at 09:05:38PM +0100, Al Viro wrote:
> > On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
> > >  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> > >  		      struct block_device *bdev, struct iov_iter *iter,
> > >  		      get_block_t get_block, dio_iodone_t end_io,
> > > -		      dio_submit_t submit_io, int flags)
> > > +		      dio_submit_t submit_io, int flags, void *private)
> > 
> > Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
> > is obscene, but so is this ;-/
> 
> So looking at these one by one, obviously needed:
> 
> - iocb
> - inode
> - iter
> 
> bdev is almost always inode->i_sb->s_bdev, except for Btrfs :(
> 
> These could _maybe_ go in struct kiocb:
> 
> - flags could maybe be folded into ki_flags
> - private could maybe go in iocb->private, but I haven't yet read
>   through to figure out if we're already using iocb->private for direct
>   I/O

I think the kiocb::private can be used for the purpose. There's only one
user, ext4, that also passes some DIO data around so it would in line
with the interface AFAICS.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba June 25, 2018, 5:16 p.m. UTC | #6
On Mon, May 14, 2018 at 06:35:48PM +0200, David Sterba wrote:
> On Fri, May 11, 2018 at 01:30:01PM -0700, Omar Sandoval wrote:
> > On Fri, May 11, 2018 at 09:05:38PM +0100, Al Viro wrote:
> > > On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
> > > >  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> > > >  		      struct block_device *bdev, struct iov_iter *iter,
> > > >  		      get_block_t get_block, dio_iodone_t end_io,
> > > > -		      dio_submit_t submit_io, int flags)
> > > > +		      dio_submit_t submit_io, int flags, void *private)
> > > 
> > > Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
> > > is obscene, but so is this ;-/
> > 
> > So looking at these one by one, obviously needed:
> > 
> > - iocb
> > - inode
> > - iter
> > 
> > bdev is almost always inode->i_sb->s_bdev, except for Btrfs :(
> > 
> > These could _maybe_ go in struct kiocb:
> > 
> > - flags could maybe be folded into ki_flags
> > - private could maybe go in iocb->private, but I haven't yet read
> >   through to figure out if we're already using iocb->private for direct
> >   I/O
> 
> I think the kiocb::private can be used for the purpose. There's only one
> user, ext4, that also passes some DIO data around so it would in line
> with the interface AFAICS.

Omar, do you have an update to the patchset? Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Sandoval June 29, 2018, 7:02 a.m. UTC | #7
On Mon, Jun 25, 2018 at 07:16:38PM +0200, David Sterba wrote:
> On Mon, May 14, 2018 at 06:35:48PM +0200, David Sterba wrote:
> > On Fri, May 11, 2018 at 01:30:01PM -0700, Omar Sandoval wrote:
> > > On Fri, May 11, 2018 at 09:05:38PM +0100, Al Viro wrote:
> > > > On Thu, May 10, 2018 at 11:30:10PM -0700, Omar Sandoval wrote:
> > > > >  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> > > > >  		      struct block_device *bdev, struct iov_iter *iter,
> > > > >  		      get_block_t get_block, dio_iodone_t end_io,
> > > > > -		      dio_submit_t submit_io, int flags)
> > > > > +		      dio_submit_t submit_io, int flags, void *private)
> > > > 
> > > > Oh, dear...  That's what, 9 arguments?  I agree that the hack in question
> > > > is obscene, but so is this ;-/
> > > 
> > > So looking at these one by one, obviously needed:
> > > 
> > > - iocb
> > > - inode
> > > - iter
> > > 
> > > bdev is almost always inode->i_sb->s_bdev, except for Btrfs :(
> > > 
> > > These could _maybe_ go in struct kiocb:
> > > 
> > > - flags could maybe be folded into ki_flags
> > > - private could maybe go in iocb->private, but I haven't yet read
> > >   through to figure out if we're already using iocb->private for direct
> > >   I/O
> > 
> > I think the kiocb::private can be used for the purpose. There's only one
> > user, ext4, that also passes some DIO data around so it would in line
> > with the interface AFAICS.
> 
> Omar, do you have an update to the patchset? Thanks.

Al, what do you think of changing all users of map_bh->b_private to use
iocb->private? We'd have to pass the iocb to get_block() and
submit_io(), but we could get rid of dio->private.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d241285a0d2a..d8b0a7eacd19 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8618,9 +8618,9 @@  static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	}
 
 	ret = __blockdev_direct_IO(iocb, inode,
-				   fs_info->fs_devices->latest_bdev,
-				   iter, btrfs_get_blocks_direct, NULL,
-				   btrfs_submit_direct, flags);
+				   fs_info->fs_devices->latest_bdev, iter,
+				   btrfs_get_blocks_direct, NULL,
+				   btrfs_submit_direct, flags, NULL);
 	if (iov_iter_rw(iter) == WRITE) {
 		up_read(&BTRFS_I(inode)->dio_sem);
 		current->journal_info = NULL;
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 874607bb6e02..9bb33d9c206c 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -1171,7 +1171,7 @@  static inline ssize_t
 do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 		      struct block_device *bdev, struct iov_iter *iter,
 		      get_block_t get_block, dio_iodone_t end_io,
-		      dio_submit_t submit_io, int flags)
+		      dio_submit_t submit_io, int flags, void *private)
 {
 	unsigned i_blkbits = READ_ONCE(inode->i_blkbits);
 	unsigned blkbits = i_blkbits;
@@ -1182,7 +1182,7 @@  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 	const loff_t end = offset + count;
 	struct dio *dio;
 	struct dio_submit sdio = { 0, };
-	struct buffer_head map_bh = { 0, };
+	struct buffer_head map_bh = { .b_private = private, };
 	struct blk_plug plug;
 	unsigned long align = offset | iov_iter_alignment(iter);
 
@@ -1304,6 +1304,7 @@  do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 
 	sdio.get_block = get_block;
 	dio->end_io = end_io;
+	dio->private = map_bh.b_private;
 	sdio.submit_io = submit_io;
 	sdio.final_block_in_bio = -1;
 	sdio.next_block_for_io = -1;
@@ -1400,7 +1401,7 @@  ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 			     struct block_device *bdev, struct iov_iter *iter,
 			     get_block_t get_block,
 			     dio_iodone_t end_io, dio_submit_t submit_io,
-			     int flags)
+			     int flags, void *private)
 {
 	/*
 	 * The block device state is needed in the end to finally
@@ -1415,7 +1416,7 @@  ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 	prefetch((char *)bdev->bd_queue + SMP_CACHE_BYTES);
 
 	return do_blockdev_direct_IO(iocb, inode, bdev, iter, get_block,
-				     end_io, submit_io, flags);
+				     end_io, submit_io, flags, private);
 }
 
 EXPORT_SYMBOL(__blockdev_direct_IO);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1e50c5efae67..e453e7529a3e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3739,7 +3739,7 @@  static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter)
 	}
 	ret = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev, iter,
 				   get_block_func, ext4_end_io_dio, NULL,
-				   dio_flags);
+				   dio_flags, NULL);
 
 	if (ret > 0 && !overwrite && ext4_test_inode_state(inode,
 						EXT4_STATE_DIO_UNWRITTEN)) {
@@ -3830,8 +3830,8 @@  static ssize_t ext4_direct_IO_read(struct kiocb *iocb, struct iov_iter *iter)
 					   iocb->ki_pos + count - 1);
 	if (ret)
 		goto out_unlock;
-	ret = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev,
-				   iter, ext4_dio_get_block, NULL, NULL, 0);
+	ret = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev, iter,
+				   ext4_dio_get_block, NULL, NULL, 0, NULL);
 out_unlock:
 	inode_unlock_shared(inode);
 	return ret;
diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index f58716567972..d42e68543baa 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -1113,7 +1113,7 @@  static ssize_t gfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	}
 
 	rv = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev, iter,
-				  gfs2_get_block_direct, NULL, NULL, 0);
+				  gfs2_get_block_direct, NULL, NULL, 0, NULL);
 out:
 	gfs2_glock_dq(&gh);
 out_uninit:
diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 302cd7caa4a7..58b74d9da931 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -2446,9 +2446,8 @@  static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	else
 		get_block = ocfs2_dio_wr_get_block;
 
-	return __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev,
-				    iter, get_block,
-				    ocfs2_dio_end_io, NULL, 0);
+	return __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev, iter,
+				    get_block, ocfs2_dio_end_io, NULL, 0, NULL);
 }
 
 const struct address_space_operations ocfs2_aops = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..b9ef2fb1224b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,9 +2993,8 @@  void dio_warn_stale_pagecache(struct file *filp);
 
 ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 			     struct block_device *bdev, struct iov_iter *iter,
-			     get_block_t get_block,
-			     dio_iodone_t end_io, dio_submit_t submit_io,
-			     int flags);
+			     get_block_t get_block, dio_iodone_t end_io,
+			     dio_submit_t submit_io, int flags, void *private);
 
 static inline ssize_t blockdev_direct_IO(struct kiocb *iocb,
 					 struct inode *inode,
@@ -3003,7 +3002,8 @@  static inline ssize_t blockdev_direct_IO(struct kiocb *iocb,
 					 get_block_t get_block)
 {
 	return __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev, iter,
-			get_block, NULL, NULL, DIO_LOCKING | DIO_SKIP_HOLES);
+				    get_block, NULL, NULL,
+				    DIO_LOCKING | DIO_SKIP_HOLES, NULL);
 }
 #endif