fix for Incorrect number of segments after building list problem

This is a rather nasty hack at the momen, but it seems to persuade blk_recalc_rq_segments() not to underestimate.

Could you try it in your setup to see if it fixes the problem?

Thanks,

James

===== drivers/block/ll_rw_blk.c 1.271 vs edited =====

drivers/block/ll_rw_blk.c +26 -6

@@ -921,7 +921,8 @@
 		}
 new_segment:
 		if (BIOVEC_VIRT_MERGEABLE(bvprv, bv) &&
-		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len)) {
+		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len) &&
+		    hw_seg_size + bv->bv_len <= q->max_segment_size) {
 			hw_seg_size += bv->bv_len;
 		} else {
 new_hw_segment:

@@ -2723,30 +2724,49 @@
 void blk_recalc_rq_segments(struct request *rq)
 {
 	struct bio *bio, *prevbio = NULL;
-	int nr_phys_segs, nr_hw_segs;
+	int nr_phys_segs, nr_hw_segs, tot_phys_size = 0, tot_hw_size = 0;
 
 	if (!rq->bio)
 		return;
 
 	nr_phys_segs = nr_hw_segs = 0;
 	rq_for_each_bio(bio, rq) {
+		int bi_phys_segs, bi_hw_segs;
 		/* Force bio hw/phys segs to be recalculated. */
 		bio->bi_flags &= ~(1 << BIO_SEG_VALID);
 
-		nr_phys_segs += bio_phys_segments(rq->q, bio);
-		nr_hw_segs += bio_hw_segments(rq->q, bio);
+		bi_phys_segs = bio_phys_segments(rq->q, bio);
+		bi_hw_segs = bio_hw_segments(rq->q, bio);
+		nr_phys_segs += bi_phys_segs;
+		nr_hw_segs += bi_hw_segs;
 		if (prevbio) {
-			if (blk_phys_contig_segment(rq->q, prevbio, bio))
+			if (blk_phys_contig_segment(rq->q, prevbio, bio) &&
+			    bio->bi_size + tot_phys_size < rq->q->max_segment_size)
 				nr_phys_segs--;
-			if (blk_hw_contig_segment(rq->q, prevbio, bio))
+			else
+				tot_phys_size = 0;
+			if (blk_hw_contig_segment(rq->q, prevbio, bio) &&
+			    bio->bi_size + tot_hw_size < rq->q->max_segment_size)
 				nr_hw_segs--;
+			else
+				tot_hw_size = 0;
 		}
+		if (bi_phys_segs > 1)
+			tot_phys_size = bio->bi_size;
+		else
+			tot_phys_size += bio->bi_size;
+		if (bi_hw_segs > 1)
+			tot_hw_size = bio->bi_size;
+		else
+			tot_hw_size += bio->bi_size;
+
 		prevbio = bio;
 	}
 
 	rq->nr_phys_segments = nr_phys_segs;
 	rq->nr_hw_segments = nr_hw_segs;
 }
+EXPORT_SYMBOL(blk_recalc_rq_segments);
 
 void blk_recalc_rq_sectors(struct request *rq, int nsect)
 {

This is a rather nasty hack at the momen, but it seems to persuade blk_recalc_rq_segments() not to underestimate.

Could you try it in your setup to see if it fixes the problem?

Thanks,

James

===== drivers/block/ll_rw_blk.c 1.271 vs edited =====

drivers/block/ll_rw_blk.c +26 -7

@@ -921,7 +921,8 @@
 		}
 new_segment:
 		if (BIOVEC_VIRT_MERGEABLE(bvprv, bv) &&
-		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len)) {
+		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len) &&
+		    hw_seg_size + bv->bv_len <= q->max_segment_size) {
 			hw_seg_size += bv->bv_len;
 		} else {
 new_hw_segment:

@@ -2723,30 +2724,49 @@
 void blk_recalc_rq_segments(struct request *rq)
 {
 	struct bio *bio, *prevbio = NULL;
-	int nr_phys_segs, nr_hw_segs;
+	int nr_phys_segs, nr_hw_segs, tot_phys_size = 0, tot_hw_size = 0;
 
 	if (!rq->bio)
 		return;
 
 	nr_phys_segs = nr_hw_segs = 0;
 	rq_for_each_bio(bio, rq) {
+		int bi_phys_segs, bi_hw_segs;
 		/* Force bio hw/phys segs to be recalculated. */
 		bio->bi_flags &= ~(1 << BIO_SEG_VALID);
 
-		nr_phys_segs += bio_phys_segments(rq->q, bio);
-		nr_hw_segs += bio_hw_segments(rq->q, bio);
+		bi_phys_segs = bio_phys_segments(rq->q, bio);
+		bi_hw_segs = bio_hw_segments(rq->q, bio);
+		nr_phys_segs += bi_phys_segs;
+		nr_hw_segs += bi_hw_segs;
 		if (prevbio) {
-			if (blk_phys_contig_segment(rq->q, prevbio, bio))
+			if (blk_phys_contig_segment(rq->q, prevbio, bio) &&
+			    bio->bi_size + tot_phys_size < rq->q->max_segment_size)
 				nr_phys_segs--;
-			if (blk_hw_contig_segment(rq->q, prevbio, bio))
+			else
+				tot_phys_size = 0;
+			if (blk_hw_contig_segment(rq->q, prevbio, bio) &&
+			    bio->bi_size + tot_hw_size < rq->q->max_segment_size)
 				nr_hw_segs--;
+			else
+				tot_hw_size = 0;
 		}
+		if (bi_phys_segs > 1)
+			tot_phys_size = bio->bi_size;
+		else
+			tot_phys_size += bio->bi_size;
+		if (bi_hw_segs > 1)
+			tot_hw_size = bio->bi_size;
+		else
+			tot_hw_size += bio->bi_size;
+
 		prevbio = bio;
 	}
 
 	rq->nr_phys_segments = nr_phys_segs;
 	rq->nr_hw_segments = nr_hw_segs;
 }
+EXPORT_SYMBOL(blk_recalc_rq_segments);
 
 void blk_recalc_rq_sectors(struct request *rq, int nsect)
 {
 
-

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

James,

I'm running your patch right now and watching for errors. After 10 minutes, everything looks good. Usually the Incorrect segment count errors show up almost immediately.

I'd say this patch fixes my problem.

Now, I'll retry the dm multipath driver, and see if it triggers any problems.

If you come up with a final patch you'd like me to test, just send it my way.

Thanks!
Dave

Quoted 82 lines · James Bottomley

This is a rather nasty hack at the momen, but it seems to persuade blk_recalc_rq_segments() not to underestimate.

Could you try it in your setup to see if it fixes the problem?

Thanks,

James

===== drivers/block/ll_rw_blk.c 1.271 vs edited =====

drivers/block/ll_rw_blk.c +26 -7

@@ -921,7 +921,8 @@
 		}
 new_segment:
 		if (BIOVEC_VIRT_MERGEABLE(bvprv, bv) &&
-		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len)) {
+		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len) &&
+		    hw_seg_size + bv->bv_len <= q->max_segment_size) {
 			hw_seg_size += bv->bv_len;
 		} else {
 new_hw_segment:

@@ -2723,30 +2724,49 @@
 void blk_recalc_rq_segments(struct request *rq)
 {
 	struct bio *bio, *prevbio = NULL;
-	int nr_phys_segs, nr_hw_segs;
+	int nr_phys_segs, nr_hw_segs, tot_phys_size = 0, tot_hw_size = 0;
 
 	if (!rq->bio)
 		return;
 
 	nr_phys_segs = nr_hw_segs = 0;
 	rq_for_each_bio(bio, rq) {
+		int bi_phys_segs, bi_hw_segs;
 		/* Force bio hw/phys segs to be recalculated. */
 		bio->bi_flags &= ~(1 << BIO_SEG_VALID);
 
-		nr_phys_segs += bio_phys_segments(rq->q, bio);
-		nr_hw_segs += bio_hw_segments(rq->q, bio);
+		bi_phys_segs = bio_phys_segments(rq->q, bio);
+		bi_hw_segs = bio_hw_segments(rq->q, bio);
+		nr_phys_segs += bi_phys_segs;
+		nr_hw_segs += bi_hw_segs;
 		if (prevbio) {
-			if (blk_phys_contig_segment(rq->q, prevbio, bio))
+			if (blk_phys_contig_segment(rq->q, prevbio, bio) &&
+			    bio->bi_size + tot_phys_size < rq->q->max_segment_size)
 				nr_phys_segs--;
-			if (blk_hw_contig_segment(rq->q, prevbio, bio))
+			else
+				tot_phys_size = 0;
+			if (blk_hw_contig_segment(rq->q, prevbio, bio) &&
+			    bio->bi_size + tot_hw_size < rq->q->max_segment_size)
 				nr_hw_segs--;
+			else
+				tot_hw_size = 0;
 		}
+		if (bi_phys_segs > 1)
+			tot_phys_size = bio->bi_size;
+		else
+			tot_phys_size += bio->bi_size;
+		if (bi_hw_segs > 1)
+			tot_hw_size = bio->bi_size;
+		else
+			tot_hw_size += bio->bi_size;
+
 		prevbio = bio;
 	}
 
 	rq->nr_phys_segments = nr_phys_segs;
 	rq->nr_hw_segments = nr_hw_segs;
 }
+EXPORT_SYMBOL(blk_recalc_rq_segments);
 
 void blk_recalc_rq_sectors(struct request *rq, int nsect)
 {
 
-

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

This is a rather nasty hack at the momen, but it seems to persuade blk_recalc_rq_segments() not to underestimate.

Could you try it in your setup to see if it fixes the problem?

Quoted 4 lines · James Bottomley

This is a rather nasty hack at the momen, but it seems to persuade blk_recalc_rq_segments() not to underestimate.

Could you try it in your setup to see if it fixes the problem?

Strange how this survived so long, thanks for debugging this James. The patch does look a little hackish, I'll see if I can beat it into submission.

Quoted 3 lines

Strange how this survived so long, thanks for debugging this James. The patch does look a little hackish, I'll see if I can beat it into submission.

Should this be enough to fix it?

===== drivers/block/ll_rw_blk.c 1.273 vs edited =====

drivers/block/ll_rw_blk.c +0 -0

@@ -922,9 +922,10 @@
 		}
 new_segment:

drivers/block/ll_rw_blk.c +13 -5

@@ -2766,22 +2767,36 @@
 {
 	struct bio *bio, *prevbio = NULL;
 	int nr_phys_segs, nr_hw_segs;
+	unsigned int phys_size, hw_size;
+	request_queue_t *q = rq->q;
 
 	if (!rq->bio)
 		return;
 
-	nr_phys_segs = nr_hw_segs = 0;
+	phys_size = hw_size = nr_phys_segs = nr_hw_segs = 0;
 	rq_for_each_bio(bio, rq) {
 		/* Force bio hw/phys segs to be recalculated. */
 		bio->bi_flags &= ~(1 << BIO_SEG_VALID);
 
-		nr_phys_segs += bio_phys_segments(rq->q, bio);
-		nr_hw_segs += bio_hw_segments(rq->q, bio);
+		nr_phys_segs += bio_phys_segments(q, bio);
+		nr_hw_segs += bio_hw_segments(q, bio);
 		if (prevbio) {
-			if (blk_phys_contig_segment(rq->q, prevbio, bio))
+			int pseg = phys_size + prevbio->bi_size + bio->bi_size;
+			int hseg = hw_size + prevbio->bi_size + bio->bi_size;
+
+			if (blk_phys_contig_segment(q, prevbio, bio) &&
+			    pseg <= q->max_segment_size) {
 				nr_phys_segs--;
-			if (blk_hw_contig_segment(rq->q, prevbio, bio))
+				phys_size += prevbio->bi_size + bio->bi_size;
+			} else
+				phys_size = 0;

Quoted 3 lines · Jens Axboe

Quoted 3 lines

Strange how this survived so long, thanks for debugging this James. The patch does look a little hackish, I'll see if I can beat it into submission.

That's polite of you ... but you know it was my fault from the last round of IOMMU merges ...

Quoted 8 lines

Should this be enough to fix it?

===== drivers/block/ll_rw_blk.c 1.273 vs edited =====

drivers/block/ll_rw_blk.c +0 -0

@@ -922,9 +922,10 @@
 		}
 new_segment:

drivers/block/ll_rw_blk.c line 931

Quoted 10 lines

drivers/block/ll_rw_blk.c +3 -2

 		if (BIOVEC_VIRT_MERGEABLE(bvprv, bv) &&
-		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len)) {
+		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len) &&
+		    hw_seg_size + bv->bv_len <= q->max_segment_size)
 			hw_seg_size += bv->bv_len;
-		} else {
+		else {
 new_hw_segment:
 			if (hw_seg_size > bio->bi_hw_front_size)
 				bio->bi_hw_front_size = hw_seg_size;

This piece is actually contamination from my tree. Since q->max_segment_size is supposed to represent the parameters of the actual card sg descriptor table, and hence cannot theoretically be exceeded on either phys or virt merges, there's currently no way to communicate this parameter to the iommu, so the dma mapping will violate it even if the block layer respects it. We're just lucky most cards (barring ide ones which have their own hack) have 32 bit DMA length descriptors.

Quoted 38 lines

drivers/block/ll_rw_blk.c +18 -5

@@ -2766,22 +2767,36 @@
 {
 	struct bio *bio, *prevbio = NULL;
 	int nr_phys_segs, nr_hw_segs;
+	unsigned int phys_size, hw_size;
+	request_queue_t *q = rq->q;
 
 	if (!rq->bio)
 		return;
 
-	nr_phys_segs = nr_hw_segs = 0;
+	phys_size = hw_size = nr_phys_segs = nr_hw_segs = 0;
 	rq_for_each_bio(bio, rq) {
 		/* Force bio hw/phys segs to be recalculated. */
 		bio->bi_flags &= ~(1 << BIO_SEG_VALID);
 
-		nr_phys_segs += bio_phys_segments(rq->q, bio);
-		nr_hw_segs += bio_hw_segments(rq->q, bio);
+		nr_phys_segs += bio_phys_segments(q, bio);
+		nr_hw_segs += bio_hw_segments(q, bio);
 		if (prevbio) {
-			if (blk_phys_contig_segment(rq->q, prevbio, bio))
+			int pseg = phys_size + prevbio->bi_size + bio->bi_size;
+			int hseg = hw_size + prevbio->bi_size + bio->bi_size;
+
+			if (blk_phys_contig_segment(q, prevbio, bio) &&
+			    pseg <= q->max_segment_size) {
 				nr_phys_segs--;
-			if (blk_hw_contig_segment(rq->q, prevbio, bio))
+				phys_size += prevbio->bi_size + bio->bi_size;
+			} else
+				phys_size = 0;
+
+			if (blk_hw_contig_segment(q, prevbio, bio) &&
+			    hseg <= q->max_segment_size) {
 				nr_hw_segs--;
+				hw_size += prevbio->bi_size + bio->bi_size;
+			} else

drivers/block/ll_rw_blk.c line 2802

Quoted 4 lines

drivers/block/ll_rw_blk.c +1 -0

+				hw_size = 0;
 		}
 		prevbio = bio;
 	}

Yes, that looks much better ... thanks! I was plotting to enhance this later to use bi_hw_front_size and bi_hw_back_size, but not until we get the IOMMU descriptor length issue sorted out.

Ah yes, now I remember. How is the fix for that coming along, btw?

Quoted 1 line · Jens Axboe

Ah yes, now I remember. How is the fix for that coming along, btw?

Erm, it didn't cross my mind until I realised what this problem was about. All SCSI drivers are setting 65536 (the default) for the max segment length. However, this is being violated wholesale by all the IOMMUs and it all works because their length descriptors are 32 bit anyway ...

However, I'll add it to the list and see if I can get something out. My slight problem is testing. Only SATA and IDE have this short descriptor problem, so I've nothing really to test the actual fix with.

slight problem is testing. Only SATA and IDE have this short descriptor problem, so I've nothing really to test the actual fix with.

Quoted 2 lines · James Bottomley

slight problem is testing. Only SATA and IDE have this short descriptor problem, so I've nothing really to test the actual fix with.

Clearly we need to remedy this :)

Quoted 2 lines · Jeff Garzik

James Bottomley wrote:

Quoted 5 lines

Quoted 2 lines

slight problem is testing. Only SATA and IDE have this short descriptor problem, so I've nothing really to test the actual fix with.

Clearly we need to remedy this :)

By improving James' imagination? :)

Quoted 1 line · Jeff Garzik

Clearly we need to remedy this :)

By improving James' imagination? :)

Quoted 2 lines · Jens Axboe

Quoted 1 line · Jeff Garzik

Clearly we need to remedy this :)

Shamelessly beg for hardware? Moi? How well^Wlittle you know me.

Quoted 1 line

By improving James' imagination? :)

I meant that 50% of the fix is pulling out the hacks that permeate the IDE drivers to resplit SG lists after iommu merging. That's the bit I can't test.