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 ;-/
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?
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...
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?
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.
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.
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.
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