Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1262434 > unrolled thread

[RFC PATCH 00/32] separate operations from flags in the bio/request structs

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 00/32] separate operations from flags in the bio/request structs mchristi@redhat.com - 2015-11-04 17:40 +0100
    [PATCH 03/32] dio/btrfs: prep dio->submit_bio users for bi_rw split. mchristi@redhat.com - 2015-11-04 17:40 +0100
    Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in  the bio/request structs Mike Christie <michaelc@cs.wisc.edu> - 2015-11-04 18:50 +0100
      Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in  the bio/request structs Christoph Hellwig <hch@infradead.org> - 2015-11-07 11:30 +0100
        Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in  the bio/request structs Mike Christie <michaelc@cs.wisc.edu> - 2015-11-11 09:00 +0100
          Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in  the bio/request structs Christoph Hellwig <hch@infradead.org> - 2015-11-11 12:30 +0100
            Re: [RFC PATCH 00/32] separate operations from flags in the  bio/request structs Mike Snitzer <snitzer@redhat.com> - 2015-11-11 18:40 +0100

#1262434 — [RFC PATCH 00/32] separate operations from flags in the bio/request structs

There are a couple new block layer commands we are trying to add support
for in the near term:

compare and write
http://www.spinics.net/lists/target-devel/msg07826.html

copy offload/extended copy/xcopy
https://www.redhat.com/archives/dm-devel/2014-July/msg00070.html

The problem is if we contine to add more commands we will have to one day
extend the cmd_flags/bi_rw fields again. To prevent that, this patchset
separates the operation (REQ_WRITE, REQ_DISCARD, REQ_WRITE_SAME, etc) from
the flags (REQ_SYNC, REQ_QUIET, etc) in the bio and request structs. In the
end of this set, we will have two fields bio->bi_op/request->op and
bio->bi_rw/request->cmd_flags.

The patches were made against Jens's linux-block tree's for-linus branch:
https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git/log/?h=for-linus
(last commit a22c4d7e34402ccdf3414f64c50365436eba7b93).

I have done some basic testing for a lot of the drivers and filesystems,
but I wanted to get comments before trying to track down more hardware/
systems for testing.


Known issues:
- REQ_FLUSH is still a flag, but should probably be a operation.
 For lower level drivers like SCSI where we only get a flush, it makes
more sense to be a operation. However, upper layers like filesystems
can send down flushes with writes, so it is more of a flag for them.
I am still working on this.

- There is a regression with the dm flakey target. It currently
cannot corrupt the operation values.

- The patchset is a little awkward. It touches so much code,
but I wanted to maintain git bisectibility, so there is lots of compat code
left around until the last patches where everyting is cleaned up.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1262435 — [PATCH 03/32] dio/btrfs: prep dio->submit_bio users for bi_rw split.

From: Mike Christie <mchristi@redhat.com>

Instead of passing around a bitmap of ops and flags, the
next patches separate it into a op field and a flags field.
This patch prepares the dio code and dio->submit_bio users
for the split.

Note that the next patches will fix up the submit_bio() call
with along other users of that function.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 fs/btrfs/inode.c   |  9 ++++-----
 fs/direct-io.c     | 34 +++++++++++++++++++++-------------
 include/linux/fs.h |  4 ++--
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 611b66d..0ad8bab 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8196,14 +8196,13 @@ out_err:
 	return 0;
 }
 
-static void btrfs_submit_direct(int rw, struct bio *dio_bio,
+static void btrfs_submit_direct(int op, int op_flags, struct bio *dio_bio,
 				struct inode *inode, loff_t file_offset)
 {
 	struct btrfs_dio_private *dip = NULL;
 	struct bio *io_bio = NULL;
 	struct btrfs_io_bio *btrfs_bio;
 	int skip_sum;
-	int write = rw & REQ_WRITE;
 	int ret = 0;
 
 	skip_sum = BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM;
@@ -8232,14 +8231,14 @@ static void btrfs_submit_direct(int rw, struct bio *dio_bio,
 	btrfs_bio = btrfs_io_bio(io_bio);
 	btrfs_bio->logical = file_offset;
 
-	if (write) {
+	if (op == REQ_OP_WRITE) {
 		io_bio->bi_end_io = btrfs_endio_direct_write;
 	} else {
 		io_bio->bi_end_io = btrfs_endio_direct_read;
 		dip->subio_endio = btrfs_subio_endio_read;
 	}
 
-	ret = btrfs_submit_direct_hook(rw, dip, skip_sum);
+	ret = btrfs_submit_direct_hook(op | op_flags, dip, skip_sum);
 	if (!ret)
 		return;
 
@@ -8267,7 +8266,7 @@ free_ordered:
 		dip = NULL;
 		io_bio = NULL;
 	} else {
-		if (write) {
+		if (op == REQ_OP_WRITE) {
 			struct btrfs_ordered_extent *ordered;
 
 			ordered = btrfs_lookup_ordered_extent(inode,
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 1125629..5e1b1a0 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -108,7 +108,8 @@ struct dio_submit {
 /* dio_state communicated between submission path and end_io */
 struct dio {
 	int flags;			/* doesn't change */
-	int rw;
+	int op;
+	int op_flags;
 	struct inode *inode;
 	loff_t i_size;			/* i_size when submitted */
 	dio_iodone_t *end_io;		/* IO completion function */
@@ -160,7 +161,7 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
 	ret = iov_iter_get_pages(sdio->iter, dio->pages, LONG_MAX, DIO_PAGES,
 				&sdio->from);
 
-	if (ret < 0 && sdio->blocks_available && (dio->rw & WRITE)) {
+	if (ret < 0 && sdio->blocks_available && (dio->op == REQ_OP_WRITE)) {
 		struct page *page = ZERO_PAGE(0);
 		/*
 		 * A memory fault, but the filesystem has some outstanding
@@ -239,7 +240,8 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret,
 		transferred = dio->result;
 
 		/* Check for short read case */
-		if ((dio->rw == READ) && ((offset + transferred) > dio->i_size))
+		if ((dio->op == REQ_OP_READ) &&
+		    ((offset + transferred) > dio->i_size))
 			transferred = dio->i_size - offset;
 	}
 
@@ -257,7 +259,7 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret,
 		inode_dio_end(dio->inode);
 
 	if (is_async) {
-		if (dio->rw & WRITE) {
+		if (dio->op == REQ_OP_WRITE) {
 			int err;
 
 			err = generic_write_sync(dio->iocb->ki_filp, offset,
@@ -393,14 +395,14 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio)
 	dio->refcount++;
 	spin_unlock_irqrestore(&dio->bio_lock, flags);
 
-	if (dio->is_async && dio->rw == READ)
+	if (dio->is_async && dio->op == REQ_OP_READ)
 		bio_set_pages_dirty(bio);
 
 	if (sdio->submit_io)
-		sdio->submit_io(dio->rw, bio, dio->inode,
+		sdio->submit_io(dio->op, dio->op_flags, bio, dio->inode,
 			       sdio->logical_offset_in_bio);
 	else
-		submit_bio(dio->rw, bio);
+		submit_bio(dio->op | dio->op_flags, bio);
 
 	sdio->bio = NULL;
 	sdio->boundary = 0;
@@ -464,14 +466,14 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
 	if (bio->bi_error)
 		dio->io_error = -EIO;
 
-	if (dio->is_async && dio->rw == READ) {
+	if (dio->is_async && dio->op == REQ_OP_READ) {
 		bio_check_pages_dirty(bio);	/* transfers ownership */
 		err = bio->bi_error;
 	} else {
 		bio_for_each_segment_all(bvec, bio, i) {
 			struct page *page = bvec->bv_page;
 
-			if (dio->rw == READ && !PageCompound(page))
+			if (dio->op == REQ_OP_READ && !PageCompound(page))
 				set_page_dirty_lock(page);
 			page_cache_release(page);
 		}
@@ -623,7 +625,7 @@ static int get_more_blocks(struct dio *dio, struct dio_submit *sdio,
 		 * which may decide to handle it or also return an unmapped
 		 * buffer head.
 		 */
-		create = dio->rw & WRITE;
+		create = dio->op == REQ_OP_WRITE;
 		if (dio->flags & DIO_SKIP_HOLES) {
 			if (sdio->block_in_file < (i_size_read(dio->inode) >>
 							sdio->blkbits))
@@ -773,7 +775,7 @@ submit_page_section(struct dio *dio, struct dio_submit *sdio, struct page *page,
 {
 	int ret = 0;
 
-	if (dio->rw & WRITE) {
+	if (dio->op == REQ_OP_WRITE) {
 		/*
 		 * Read accounting is performed in submit_bio()
 		 */
@@ -973,7 +975,7 @@ do_holes:
 				loff_t i_size_aligned;
 
 				/* AKPM: eargh, -ENOTBLK is a hack */
-				if (dio->rw & WRITE) {
+				if (dio->op == REQ_OP_WRITE) {
 					page_cache_release(page);
 					return -ENOTBLK;
 				}
@@ -1176,7 +1178,13 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 		dio->is_async = true;
 
 	dio->inode = inode;
-	dio->rw = iov_iter_rw(iter) == WRITE ? WRITE_ODIRECT : READ;
+	if (iov_iter_rw(iter) == WRITE) {
+		dio->op = REQ_OP_WRITE;
+		dio->op_flags = WRITE_ODIRECT;
+	} else {
+		dio->op = REQ_OP_READ;
+		dio->op_flags = 0;
+	}
 
 	/*
 	 * For AIO O_(D)SYNC writes we need to defer completions to a workqueue
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 72d8a84..601b842 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2678,8 +2678,8 @@ extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 
 #ifdef CONFIG_BLOCK
-typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode,
-			    loff_t file_offset);
+typedef void (dio_submit_t)(int op, int op_flags, struct bio *bio,
+			    struct inode *inode, loff_t file_offset);
 
 enum {
 	/* need locking between buffered and direct access */
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262466 — Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in the bio/request structs

On 11/04/2015 10:49 AM, Bart Van Assche wrote:
> Hello Mike,
> 
> If you have to touch submit_bio() and submit_bio_wait(), how about
> requiring the callers of these functions to set the cmd and flags
> arguments in the bio structure and to leave out the cmd and flags
> arguments from the submit_bio() and submit_bio_wait() functions ? A
> (compile tested only) patch that implements this idea is available at
> https://lkml.org/lkml/2014/6/2/173.
> 

Yeah, I can do that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264804 — Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in the bio/request structs

On Wed, Nov 04, 2015 at 10:53:39AM -0600, Mike Christie wrote:
> > If you have to touch submit_bio() and submit_bio_wait(), how about
> > requiring the callers of these functions to set the cmd and flags
> > arguments in the bio structure and to leave out the cmd and flags
> > arguments from the submit_bio() and submit_bio_wait() functions ? A
> > (compile tested only) patch that implements this idea is available at
> > https://lkml.org/lkml/2014/6/2/173.
> > 
> 
> Yeah, I can do that.

I think this would be useful to do at the beginning of the series.
While this will have a huge trickle effect through the series it should
make thing a bit simpler overall.  By leaving op 0 undefined it should
also help to create a nice error trap for unconverted code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267020 — Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in the bio/request structs

On 11/07/2015 04:23 AM, Christoph Hellwig wrote:
> On Wed, Nov 04, 2015 at 10:53:39AM -0600, Mike Christie wrote:
>>> If you have to touch submit_bio() and submit_bio_wait(), how about
>>> requiring the callers of these functions to set the cmd and flags
>>> arguments in the bio structure and to leave out the cmd and flags
>>> arguments from the submit_bio() and submit_bio_wait() functions ? A
>>> (compile tested only) patch that implements this idea is available at
>>> https://lkml.org/lkml/2014/6/2/173.
>>>
>>
>> Yeah, I can do that.
> 
> I think this would be useful to do at the beginning of the series.

I just wanted to double check that we wanted to do this.

We no longer have the bvec merge functions so the original reason given
in the thread/patch Bart referenced is no longer valid.

Offlist it was suggested that dropping the argument from submit_bio
might still improve performance, but I modified xfs and dm and did some
testing and did not see anything.

So the change is not needed, and it would only be done because people
feel it would improve the interface.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267114 — Re: [dm-devel] [RFC PATCH 00/32] separate operations from flags in the bio/request structs

On Wed, Nov 11, 2015 at 01:53:24AM -0600, Mike Christie wrote:
> We no longer have the bvec merge functions so the original reason given
> in the thread/patch Bart referenced is no longer valid.
> 
> Offlist it was suggested that dropping the argument from submit_bio
> might still improve performance, but I modified xfs and dm and did some
> testing and did not see anything.
> 
> So the change is not needed, and it would only be done because people
> feel it would improve the interface.

I would defintively prefer the changed interface, and to me it also
looks like it would make your overall patch simpler.  If you disagree
feel free to keep it as-is for now and I'll do another pass later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267341 — Re: [RFC PATCH 00/32] separate operations from flags in the bio/request structs

On Wed, Nov 11 2015 at  6:28am -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Nov 11, 2015 at 01:53:24AM -0600, Mike Christie wrote:
> > We no longer have the bvec merge functions so the original reason given
> > in the thread/patch Bart referenced is no longer valid.
> > 
> > Offlist it was suggested that dropping the argument from submit_bio
> > might still improve performance, but I modified xfs and dm and did some
> > testing and did not see anything.
> > 
> > So the change is not needed, and it would only be done because people
> > feel it would improve the interface.
> 
> I would defintively prefer the changed interface, and to me it also
> looks like it would make your overall patch simpler.  If you disagree
> feel free to keep it as-is for now and I'll do another pass later.

The less churn the better.  But honestly, not quite following the logic
on why all this flag-day churn is needed.  BUT if it needs to happen,
because we'll soon hit a wall on supported operations via REQ_*, now is
as good a time as any -- but best to limit the change to the flags IMHO.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web