Regression introduced with "block: split bios to max possible length"

Hi,

unfortunately commit e36f62042880 "block: split bios to maxpossible length" breaks the DASD driver on s390. We expect the block requests to be multiple of 4k in size. With the patch applied I see the requests split up in multiple of 512 byte and therefore the requests get rejected and lots of I/Os fail.

Could you please fix or revert the patch?

We have following values set for the block queue:

hw_sector_size         4096
logical_block_size     4096
minimum_io_size        4096
physical_block_size    4096

max_hw_sectors_kb 760
max_sectors_kb 760
max_segment_size 4096
max_segments 65535

Regards,
Stefan

Sigh, that's definitely a bug. I'll take a look at it and see if I can get a tested fix in for 4.5-rc1. If not, we'll revert it, again.

Jens Axboe

My apologies for the trouble. I trust it really is broken, but I don't quite see how. The patch supposedly splits the transfer to the max size the request queue says it allows. How does the max allowed size end up an invalid multiple?

I assume that in this case it's simply that

- max_sectors is some odd number in sectors (ie 65535)

- the block size is larger than a sector (ie 4k)

- the device probably doesn't even have any silly chunk size issue
(so chunk_sectors is zero).

and because the block size is 4k, no valid IO can ever generate anything but 4k-aligned IO's, and everything is fine.

Except now the "split bios" patch will split blindly at the max_sectors size, which is pure and utter garbage, since it doesn't take the minimum block size into account.

Also, quite frankly, I think that whole "split bios" patch is garbage *anyway*.

The thing is, the whole "blk_max_size_offset()" use there is broken. What I think it _should_ do is:

(a) check against max sectors like it used to do:

                if (sectors + (bv.bv_len >> 9) > queue_max_sectors(q))
                        goto split;

(b) completely separately, and _independently_ of that max sector
check, it should check against the "chunk_sectors" limit if it exists.

instead, it uses that nasty blk_max_size_offset() crap, which is broken because it's confusing, but also because it doesn't honor max_sectors AT ALL if there is a chunking size.

So I think chunking size should be independent of max_sectors. I could see some device that has some absolute max sector size, but that _also_ wants to split so that the bio never crosses a particular chunk size (perhaps due to RAID, perhaps due to some internal device block handling rules).

Trying to mix the two things with those "blk_max_size_offset()" games is just wrong.

Linus

Wouldn't that make max sectors non-sensical? Or am I mistaken to think max sectors is supposed to be a valid transfer in multiples of the physical sector size?

I do think this is what's happening though. A recent commit (ca369d51b) limits the max_sectors to 255 max by default, which isn't right for 4k. A driver has to override the queue's limits.max_dev_sectors first to get the desired limit for their storage.

I'm not sure if that was the intention. There are lots of drivers requesting more than 255 and probably unaware they're not getting it, DASD included. I don't think we'd have seen this problem if the requested setting wasn't overridden.

This can create less optimal splits for h/w that advertise chunk size. I know it's a quirky feature (wasn't my idea), but the h/w is very slow to not split at the necessary alignments, and we used to handle this split correctly.

Point taken, though. The implementation needs some cleaning up.

If the controller interface is some 16-bit register, then the maximum number of sectors you can specify is 65535.

But if the disk then doesn't like 512-byte accesses, but wants 4kB or whatever, then clearly you can't actually *feed* it that maximum number. Not because it's a maximal, but because it's not aligned.

But that doesn't mean that it's non-sensical. It just means that you have to take both things into account. There may be two totally independent things that cause the two (very different) rules on what the IO can look like.

Obviously there are probably games we could play, like always limiting the maximum sector number to a multiple of the sector size. That would presumably work for Stefan's case, by simply "artificially" making max_sectors be 65528 instead.

But I do think it's better to consider them independent issues, and just make sure that we always honor those things independently.

That "honor things independently" used to happen automatically before, simply because we'd never split in the middle of a bio segment. And since each bio segment was created with the limitations of the device in mind, that all worked.

Now that it splits in the middle of a vector entry, that splitting just needs to honor _all_ the rules. Not just the max sector one.

I suspect few high-performance controllers will really have big issues with the max_sectors thing. If you have big enough IO that you could hit the maximum sector number, you're already pretty well off, you might as well split at that point.

So I think it's ok to split at the max sector case early.

For the case of nvme, for example, I think the max sector number is so high that you'll never hit that anyway, and you'll only ever hit the chunk limit. No?

So in practice it won't matter, I suspect.

Linus

The device's max transfer and chunk size are not very large, both fixed at 128KB. We can lose ~70% of potential throughput when IO isn't aligned, and end users reported this when the block layer stopped splitting on alignment for the NVMe drive.

So it's a big deal for this h/w, but now I feel awkward defending a device specific feature for the generic block layer.

Anyway, the patch was developed with incorrect assumptions. I'd still like to try again after reconciling the queue limit constraints, but I defer to Jens for the near term.

Thanks!

Honestly, the splitting code is what is a piece of crap, we never should have gone down that route. Hopefully we can get rid of it soon. In the mean time, this does need to work. It's an odd hw construct (basically two devices bolted together), but it's not really an esoteric thing to support.

Instead of scrambling for -rc1, I'd suggest we just revert again and ensure what we merge for -rc2 is clean and passes the test cases.

Jens Axboe

Yes, it is one problem, something like below does fix my test with 4K block size.

--

index 1699df5..49e0394 100644
@@ -81,6 +81,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
 	unsigned front_seg_size = bio->bi_seg_front_size;
 	bool do_split = true;
 	struct bio *new = NULL;
+	unsigned max_sectors;
 
 	bio_for_each_segment(bv, bio, iter) {
 		/*

@@ -90,20 +91,23 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
 		if (bvprvp && bvec_gap_to_prev(q, bvprvp, bv.bv_offset))
 			goto split;
 
-		if (sectors + (bv.bv_len >> 9) >
-				blk_max_size_offset(q, bio->bi_iter.bi_sector)) {
+		/* I/O shoule be aligned to logical block size */
+		max_sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector);
+		max_sectors = ((max_sectors << 9) &
+				~(queue_logical_block_size(q) - 1)) >> 9;
+		if (sectors + (bv.bv_len >> 9) > max_sectors) {
 			/*
 			 * Consider this a new segment if we're splitting in
 			 * the middle of this vector.
 			 */
 			if (nsegs < queue_max_segments(q) &&
-			    sectors < blk_max_size_offset(q,
-						bio->bi_iter.bi_sector)) {
+			    sectors < max_sectors) {
 				nsegs++;
-				sectors = blk_max_size_offset(q,
-						bio->bi_iter.bi_sector);
+				sectors = max_sectors;
 			}
-			goto split;
+			if (sectors)
+				goto split;
+			/* It is OK to put single bvec into one segment */
 		}
 
 		if (bvprvp && blk_queue_cluster(q)) {

It just doesn't look very legible.

Also, how could this

OK, I will try to make it better.

I guess that is possible because blk_max_size_offset() depends on offset and may return a small size which is less than logical block size.

--
Ming Lei