Re: SCSI bug

[John David Anglin <dave.anglin@bell.net>]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]

On 2016-02-21, at 10:24 PM, John David Anglin wrote:

> On 2016-02-21, at 7:53 PM, John David Anglin wrote:
> 
>> Backtrace:                                                                      
>> [<000000004046f4b0>] scsi_init_sgtable+0x70/0xb8                               
>> [<000000004046f564>] scsi_init_io+0x6c/0x220                                   
>> [<000000000811c5c0>] sd_setup_read_write_cmnd+0x58/0x968 [sd_mod]              
>> [<000000000811cf14>] sd_init_command+0x44/0x130 [sd_mod]                       
>> [<000000004046f81c>] scsi_setup_cmnd+0x104/0x1b0                               
>> [<000000004046fab8>] scsi_prep_fn+0x100/0x1a0                                  
>> [<000000004035b9b0>] blk_peek_request+0x1b8/0x298                              
>> [<0000000040471028>] scsi_request_fn+0xf8/0xa90                                
>> [<0000000040357244>] __blk_run_queue+0x4c/0x70                                 
>> [<00000000403802c4>] cfq_insert_request+0x2dc/0x580                            
>> [<0000000040356404>] __elv_add_request+0x1b4/0x300                             
>> 
>> We have in blk-merge.c:
>> 
>>               else {
>>                       /*
>>                        * If the driver previously mapped a shorter
>>                        * list, we could see a termination bit
>>                        * prematurely unless it fully inits the sg
>>                        * table on each mapping. We KNOW that there
>>                        * must be more entries here or the driver
>>                        * would be buggy, so force clear the
>>                        * termination bit to avoid doing a full
>>                        * sg_init_table() in drivers for each command.
>>                        */
>>                       if (sg_is_last (*sg))
>>                         printk ("__blk_segment_map_sg: clearing termination bi
>> t\n");
>>                       sg_unmark_end(*sg);
>>                       *sg = sg_next(*sg);
>>                       BUG_ON (!*sg);
>>               }
>> 
>> The comment suggests there must be more entries...
> 
> I'm thinking with the split the scsi driver needs to provide one or two extra entires in the sg list.


With the attached patch, I'm able to boot 4.2.0-rc2+ on linux-block at commit
54efd50bfd873e2dbf784e0b21a8027ba4299a3e.

I didn't try to optimize the number of extra entries but I know one is not enough.

I guess the puzzle is why the number of entries isn't calculated correctly in the first place.
Further, why does blk-merge believe that it's okay to go beyond the terminator?  Clearly,
the magic number isn't always set, etc.

I added the WARN_ON so I'd know when we run off the end of the the list.

Dave
--
John David Anglin	dave.anglin@bell.net



[-- Attachment #2: scsi-nents.d.txt --]
[-- Type: text/plain, Size: 1395 bytes --]

diff --git a/block/blk-merge.c b/block/blk-merge.c
index d9c3a75..8e2566b 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -327,6 +327,7 @@ new_segment:
 			 * termination bit to avoid doing a full
 			 * sg_init_table() in drivers for each command.
 			 */
+			WARN_ON(sg_is_last (*sg));
 			sg_unmark_end(*sg);
 			*sg = sg_next(*sg);
 		}
@@ -392,6 +393,9 @@ int blk_rq_map_sg(struct request_queue *q, struct request *rq,
 	if (rq->bio)
 		nsegs = __blk_bios_map_sg(q, rq->bio, sglist, &sg);
 
+	if (!sg)
+		return nsegs;
+
 	if (unlikely(rq->cmd_flags & REQ_COPY_USER) &&
 	    (blk_rq_bytes(rq) & q->dma_pad_mask)) {
 		unsigned int pad_len =
@@ -415,8 +419,7 @@ int blk_rq_map_sg(struct request_queue *q, struct request *rq,
 		rq->extra_len += q->dma_drain_size;
 	}
 
-	if (sg)
-		sg_mark_end(sg);
+	sg_mark_end(sg);
 
 	return nsegs;
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b1a2631..b421f03 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -595,6 +595,11 @@ static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)
 
 	BUG_ON(!nents);
 
+	/* Provide extra entries in case of split.  */
+	nents += 8;
+	if (nents > SCSI_MAX_SG_SEGMENTS)
+		nents = SCSI_MAX_SG_SEGMENTS;
+
 	if (mq) {
 		if (nents <= SCSI_MAX_SG_SEGMENTS) {
 			sdb->table.nents = nents;