block: makes bio_split support bio without data

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.
Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

Thanks,
NeilBrown

From: Shaohua Li <[email protected]>
Date: Thu, 20 Sep 2012 09:36:03 +1000
Subject: [PATCH] block: makes bio_split support bio without data

discard bio hasn't data attached. We hit a BUG_ON with such bio. This makes bio_split works for such bio.

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by NeilBrown <[email protected]>

fs/bio.c +0 -0

index 71072ab..dbb7a6c 100644

fs/bio.c +11 -9

 	BUG_ON(bi->bi_idx != 0);
 	atomic_set(&bp->cnt, 3);
 	bp->error = 0;

@@ -1511,17 +1511,19 @@ struct bio_pair *bio_split(struct bio *bi, int first_sectors)
 	bp->bio2.bi_size -= first_sectors << 9;
 	bp->bio1.bi_size = first_sectors << 9;
 
-	bp->bv1 = bi->bi_io_vec[0];
-	bp->bv2 = bi->bi_io_vec[0];
-	bp->bv2.bv_offset += first_sectors << 9;
-	bp->bv2.bv_len -= first_sectors << 9;
-	bp->bv1.bv_len = first_sectors << 9;
+	if (bi->bi_vcnt != 0) {
+		bp->bv1 = bi->bi_io_vec[0];
+		bp->bv2 = bi->bi_io_vec[0];
+		bp->bv2.bv_offset += first_sectors << 9;
+		bp->bv2.bv_len -= first_sectors << 9;
+		bp->bv1.bv_len = first_sectors << 9;
 
-	bp->bio1.bi_io_vec = &bp->bv1;
-	bp->bio2.bi_io_vec = &bp->bv2;
+		bp->bio1.bi_io_vec = &bp->bv1;
+		bp->bio2.bi_io_vec = &bp->bv2;
 
-	bp->bio1.bi_max_vecs = 1;
-	bp->bio2.bi_max_vecs = 1;
+		bp->bio1.bi_max_vecs = 1;
+		bp->bio2.bi_max_vecs = 1;
+	}
 
 	bp->bio1.bi_end_io = bio_pair_end_1;
 	bp->bio2.bi_end_io = bio_pair_end_2;

Hi,

Quoted 30 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.
Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

Thanks,
NeilBrown

From: Shaohua Li <[email protected]>
Date: Thu, 20 Sep 2012 09:36:03 +1000
Subject: [PATCH] block: makes bio_split support bio without data

discard bio hasn't data attached. We hit a BUG_ON with such bio. This makes bio_split works for such bio.

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by NeilBrown <[email protected]>

fs/bio.c +0 -0

index 71072ab..dbb7a6c 100644
@@ -1501,7 +1501,7 @@ struct bio_pair *bio_split(struct bio *bi, int first_sectors)
 	trace_block_split(bdev_get_queue(bi->bi_bdev), bi,
 				bi->bi_sector + first_sectors);
 

fs/bio.c struct bio_pair *bio_split(struct bio *bi, int first_sectors) · line 1504

Quoted 2 lines

fs/bio.c +1 -1

-	BUG_ON(bi->bi_vcnt != 1);
+	BUG_ON(bi->bi_vcnt != 1 && bi->bi_vcnt != 0);

Why not

	BUG_ON(bi->bi_vcnt > 1);

?

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.
Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

Quoted 8 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.
Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

It should go through my tree, especially since we've got conflicts with other changes. In other words, your patch does not apply to for-3.7/core as-is...

Shaohua, could you resend an updated variant?

Quoted 10 lines · Jens Axboe

Quoted 8 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.
Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

Quoted 5 lines

It should go through my tree, especially since we've got conflicts with other changes. In other words, your patch does not apply to for-3.7/core as-is...

Shaohua, could you resend an updated variant?

Here is the one applied to for-3.7/core

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.

Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

Thanks,
NeilBrown

From: Shaohua Li <[email protected]>
Date: Thu, 20 Sep 2012 09:36:03 +1000
Subject: [PATCH] block: makes bio_split support bio without data

discard bio hasn't data attached. We hit a BUG_ON with such bio. This makes bio_split works for such bio.

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by NeilBrown <[email protected]>

fs/bio.c +12 -10

index 71072ab..dbb7a6c 100644
@@ -1501,7 +1501,7 @@ struct bio_pair *bio_split(struct bio *bi, int first_sectors)
 	trace_block_split(bdev_get_queue(bi->bi_bdev), bi,
 				bi->bi_sector + first_sectors);
 
-	BUG_ON(bi->bi_vcnt != 1);
+	BUG_ON(bi->bi_vcnt != 1 && bi->bi_vcnt != 0);
 	BUG_ON(bi->bi_idx != 0);
 	atomic_set(&bp->cnt, 3);
 	bp->error = 0;

@@ -1511,17 +1511,19 @@ struct bio_pair *bio_split(struct bio *bi, int first_sectors)
 	bp->bio2.bi_size -= first_sectors << 9;
 	bp->bio1.bi_size = first_sectors << 9;
 
-	bp->bv1 = bi->bi_io_vec[0];
-	bp->bv2 = bi->bi_io_vec[0];
-	bp->bv2.bv_offset += first_sectors << 9;
-	bp->bv2.bv_len -= first_sectors << 9;
-	bp->bv1.bv_len = first_sectors << 9;
+	if (bi->bi_vcnt != 0) {
+		bp->bv1 = bi->bi_io_vec[0];
+		bp->bv2 = bi->bi_io_vec[0];
+		bp->bv2.bv_offset += first_sectors << 9;
+		bp->bv2.bv_len -= first_sectors << 9;
+		bp->bv1.bv_len = first_sectors << 9;
 
-	bp->bio1.bi_io_vec = &bp->bv1;
-	bp->bio2.bi_io_vec = &bp->bv2;
+		bp->bio1.bi_io_vec = &bp->bv1;
+		bp->bio2.bi_io_vec = &bp->bv2;
 
-	bp->bio1.bi_max_vecs = 1;
-	bp->bio2.bi_max_vecs = 1;
+		bp->bio1.bi_max_vecs = 1;
+		bp->bio2.bi_max_vecs = 1;
+	}
 
 	bp->bio1.bi_end_io = bio_pair_end_1;
 	bp->bio2.bi_end_io = bio_pair_end_2;

Quoted 6 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.

Thing is, at some point in the stack a discard bio is going to have data
- see blk_add_rquest_payload(), and it used to be the single page was added to discard bios above generic_make_request(), in blkdev_issue_discard() or whatever it's called.

So while I'm sure your code works, it's just a fragile way of doing it.

There's also other types of bios where bi_size has nothing to do with the amount of data in the bi_io_vec - actually I think this is a new thing, since Martin Petersen just added REQ_WRITE_SAME and I don't think there were any other instances besides REQ_DISCARD before.

So my preference would be defining a mask (REQ_DISCARD|REQ_WRITE_SAME), and if bio->bi_rw & that mask is true, just duplicate the bvec or whatever.

That way it's much more explicit and less likely to trip someone else up later.

(I've actually got a patch in my tree that does just that, but it's special cased in bio_advance() which makes things work out really nicely).

Quoted 49 lines

Are you happy with it going in though my 'md' tree, or would you rather take it though your 'block' tree?

Thanks,
NeilBrown

From: Shaohua Li <[email protected]>
Date: Thu, 20 Sep 2012 09:36:03 +1000
Subject: [PATCH] block: makes bio_split support bio without data

discard bio hasn't data attached. We hit a BUG_ON with such bio. This makes bio_split works for such bio.

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by NeilBrown <[email protected]>

fs/bio.c +8 -8

index 71072ab..dbb7a6c 100644
@@ -1501,7 +1501,7 @@ struct bio_pair *bio_split(struct bio *bi, int first_sectors)
 	trace_block_split(bdev_get_queue(bi->bi_bdev), bi,
 				bi->bi_sector + first_sectors);
 
-	BUG_ON(bi->bi_vcnt != 1);
+	BUG_ON(bi->bi_vcnt != 1 && bi->bi_vcnt != 0);
 	BUG_ON(bi->bi_idx != 0);
 	atomic_set(&bp->cnt, 3);
 	bp->error = 0;

@@ -1511,17 +1511,19 @@ struct bio_pair *bio_split(struct bio *bi, int first_sectors)
 	bp->bio2.bi_size -= first_sectors << 9;
 	bp->bio1.bi_size = first_sectors << 9;
 
-	bp->bv1 = bi->bi_io_vec[0];
-	bp->bv2 = bi->bi_io_vec[0];
-	bp->bv2.bv_offset += first_sectors << 9;
-	bp->bv2.bv_len -= first_sectors << 9;
-	bp->bv1.bv_len = first_sectors << 9;
+	if (bi->bi_vcnt != 0) {
+		bp->bv1 = bi->bi_io_vec[0];
+		bp->bv2 = bi->bi_io_vec[0];
+		bp->bv2.bv_offset += first_sectors << 9;
+		bp->bv2.bv_len -= first_sectors << 9;
+		bp->bv1.bv_len = first_sectors << 9;
 
-	bp->bio1.bi_io_vec = &bp->bv1;
-	bp->bio2.bi_io_vec = &bp->bv2;
+		bp->bio1.bi_io_vec = &bp->bv1;

fs/bio.c struct bio_pair *bio_split(struct bio *bi, int first_sectors) · line 1529

Quoted 10 lines

fs/bio.c +4 -2

+		bp->bio2.bi_io_vec = &bp->bv2;
 
-	bp->bio1.bi_max_vecs = 1;
-	bp->bio2.bi_max_vecs = 1;
+		bp->bio1.bi_max_vecs = 1;
+		bp->bio2.bi_max_vecs = 1;
+	}
 
 	bp->bio1.bi_end_io = bio_pair_end_1;
 	bp->bio2.bi_end_io = bio_pair_end_2;

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

Quoted 15 lines · On Fri, 28 Sep 2012 09:23:43 -0700 Kent Overstreet <[email protected]>…

Quoted 6 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.

Thing is, at some point in the stack a discard bio is going to have data
- see blk_add_rquest_payload(), and it used to be the single page was added to discard bios above generic_make_request(), in blkdev_issue_discard() or whatever it's called.

So while I'm sure your code works, it's just a fragile way of doing it.

Quoted 8 lines

There's also other types of bios where bi_size has nothing to do with the amount of data in the bi_io_vec - actually I think this is a new thing, since Martin Petersen just added REQ_WRITE_SAME and I don't think there were any other instances besides REQ_DISCARD before.

So my preference would be defining a mask (REQ_DISCARD|REQ_WRITE_SAME), and if bio->bi_rw & that mask is true, just duplicate the bvec or whatever.

Hi Kent,
I'm afraid I don't see the relevance of your comments to the patch.

The current bio_split code can successfully split a bio with zero or one bi_vec entry. If there are more than that, we cannot split.

How does it matter whether the bio is a DISCARD or a WRITE_SAME or a DATA or whatever?

Quoted 27 lines · NeilBrown

Quoted 15 lines · On Fri, 28 Sep 2012 09:23:43 -0700 Kent Overstreet <[email protected]>…

Quoted 6 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.

Thing is, at some point in the stack a discard bio is going to have data
- see blk_add_rquest_payload(), and it used to be the single page was added to discard bios above generic_make_request(), in blkdev_issue_discard() or whatever it's called.

So while I'm sure your code works, it's just a fragile way of doing it.

Quoted 8 lines

There's also other types of bios where bi_size has nothing to do with the amount of data in the bi_io_vec - actually I think this is a new thing, since Martin Petersen just added REQ_WRITE_SAME and I don't think there were any other instances besides REQ_DISCARD before.

So my preference would be defining a mask (REQ_DISCARD|REQ_WRITE_SAME), and if bio->bi_rw & that mask is true, just duplicate the bvec or whatever.

Quoted 8 lines

Hi Kent,
I'm afraid I don't see the relevance of your comments to the patch.

The current bio_split code can successfully split a bio with zero or one bi_vec entry. If there are more than that, we cannot split.

How does it matter whether the bio is a DISCARD or a WRITE_SAME or a DATA or whatever?

Hrm, I think I didn't explain very well.

After your change, if bio->bi_vcnt != 0, then it splits the bvec.

The trouble is that discard bios do under certain circumstances have bio->bi_vcnt != 0, in which case splitting the bvec is the wrong thing to do - first_sectors will quite likely be bigger than the bvec.

In practice this isn't currently a problem for discard bios, because since Christoph added blk_add_request_payload(), discard bios won't have that bvec added until they hit the scsi layer which will be after any splitting. But this is a fairly recent and unrelated change, and IMO not the kind of behaviour I'd want to rely on.

WRITE_SAME is a problem for the same reason - bio_sectors(bio) may be large, but the bio will always have a single bvec and splitting the bvec is always the wrong thing to do for WRITE_SAME.

So, I think it makes more sense to make the splitting conditional on !(bio->bi_rw & (REQ_DISCARD|REQ_WRITE_SAME)), in addition to bio->bi_vcnt == 1.

..That make more sense?

Quoted 55 lines · On Tue, 2 Oct 2012 14:09:23 -0700 Kent Overstreet <[email protected]>…

Quoted 27 lines · NeilBrown

Quoted 15 lines · On Fri, 28 Sep 2012 09:23:43 -0700 Kent Overstreet <[email protected]>…

Quoted 6 lines · NeilBrown

Hi Jens,
this patch has been sitting in my -next tree for a little while and I was hoping for it to go in for the next merge window.
It simply allows bio_split() to be used on bios without a payload, such as 'discard'.

Thing is, at some point in the stack a discard bio is going to have data
- see blk_add_rquest_payload(), and it used to be the single page was added to discard bios above generic_make_request(), in blkdev_issue_discard() or whatever it's called.

So while I'm sure your code works, it's just a fragile way of doing it.

Quoted 8 lines

There's also other types of bios where bi_size has nothing to do with the amount of data in the bi_io_vec - actually I think this is a new thing, since Martin Petersen just added REQ_WRITE_SAME and I don't think there were any other instances besides REQ_DISCARD before.

So my preference would be defining a mask (REQ_DISCARD|REQ_WRITE_SAME), and if bio->bi_rw & that mask is true, just duplicate the bvec or whatever.

Quoted 8 lines

Hi Kent,
I'm afraid I don't see the relevance of your comments to the patch.

The current bio_split code can successfully split a bio with zero or one bi_vec entry. If there are more than that, we cannot split.

How does it matter whether the bio is a DISCARD or a WRITE_SAME or a DATA or whatever?

Hrm, I think I didn't explain very well.

After your change, if bio->bi_vcnt != 0, then it splits the bvec.

The trouble is that discard bios do under certain circumstances have bio->bi_vcnt != 0, in which case splitting the bvec is the wrong thing to do - first_sectors will quite likely be bigger than the bvec.

In practice this isn't currently a problem for discard bios, because since Christoph added blk_add_request_payload(), discard bios won't have that bvec added until they hit the scsi layer which will be after any splitting. But this is a fairly recent and unrelated change, and IMO not the kind of behaviour I'd want to rely on.

WRITE_SAME is a problem for the same reason - bio_sectors(bio) may be large, but the bio will always have a single bvec and splitting the bvec is always the wrong thing to do for WRITE_SAME.

Quoted 5 lines

So, I think it makes more sense to make the splitting conditional on !(bio->bi_rw & (REQ_DISCARD|REQ_WRITE_SAME)), in addition to bio->bi_vcnt == 1.

..That make more sense?

Yes, that does make some more sense, thanks. However it doesn't convince me that we need to change the patch.

I guess my position is that once we get to this code, we absolutely have to split the bio - it maps to two separate devices in a RAID0 or similar so not-splitting is not an option.

Maybe various md devices need to detect and reject REQ_DISCARD requests that have a payload and REQ_WRITE_SAME requests? Or would they need to explicitly set a flag to say they accept them?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Kent" == Kent Overstreet <[email protected]> writes: