Re: [PATCH V4 3/3] scsi: core: avoid to pre-allocate big chunk for sg list

From: Guenter Roeck <linux@roeck-us.net>
To: Ming Lei <ming.lei@redhat.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>,
	linux-scsi@vger.kernel.org,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-block@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Bart Van Assche <bvanassche@acm.org>,
	"Ewan D . Milne" <emilne@redhat.com>,
	Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH V4 3/3] scsi: core: avoid to pre-allocate big chunk for sg list
Date: Mon, 3 Jun 2019 20:49:10 -0700	[thread overview]
Message-ID: <cdf94e43-79a7-078e-676d-dfc736eec286@roeck-us.net> (raw)
In-Reply-To: <20190604010002.GA24432@ming.t460p>

On 6/3/19 6:00 PM, Ming Lei wrote:
> On Mon, Jun 03, 2019 at 01:44:22PM -0700, Guenter Roeck wrote:
>> On Sun, Apr 28, 2019 at 03:39:32PM +0800, Ming Lei wrote:
>>> Now scsi_mq_setup_tags() pre-allocates a big buffer for IO sg list,
>>> and the buffer size is scsi_mq_sgl_size() which depends on smaller
>>> value between shost->sg_tablesize and SG_CHUNK_SIZE.
>>>
>>> Modern HBA's DMA is often capable of deadling with very big segment
>>> number, so scsi_mq_sgl_size() is often big. Suppose the max sg number
>>> of SG_CHUNK_SIZE is taken, scsi_mq_sgl_size() will be 4KB.
>>>
>>> Then if one HBA has lots of queues, and each hw queue's depth is
>>> high, pre-allocation for sg list can consume huge memory.
>>> For example of lpfc, nr_hw_queues can be 70, each queue's depth
>>> can be 3781, so the pre-allocation for data sg list is 70*3781*2k
>>> =517MB for single HBA.
>>>
>>> There is Red Hat internal report that scsi_debug based tests can't
>>> be run any more since legacy io path is killed because too big
>>> pre-allocation.
>>>
>>> So switch to runtime allocation for sg list, meantime pre-allocate 2
>>> inline sg entries. This way has been applied to NVMe PCI for a while,
>>> so it should be fine for SCSI too. Also runtime sg entries allocation
>>> has verified and run always in the original legacy io path.
>>>
>>> Not see performance effect in my big BS test on scsi_debug.
>>>
>>
>> This patch causes a variety of boot failures in -next. Typical failure
>> pattern is scsi hangs or failure to find a root file system. For example,
>> on alpha, trying to boot from usb:
> 
> I guess it is because alpha doesn't support sg chaining, and
> CONFIG_ARCH_NO_SG_CHAIN is enabled. ARCHs not supporting sg chaining
> can only be arm, alpha and parisc.
> 

I don't think it is that simple. I do see the problem on x86 (32 and 64 bit)
sparc, ppc, and m68k as well, and possibly others (I didn't check all because
-next is in terrible shape right now). Error log is always a bit different
but similar.

On sparc:

scsi host0: Data transfer overflow.
scsi host0: cur_residue[0] tot_residue[-181604017] len[8192]
scsi host0: DMA length is zero!
scsi host0: cur adr[f000f000] len[00000000]
scsi host0: Data transfer overflow.
scsi host0: cur_residue[0] tot_residue[-181604017] len[8192]
scsi host0: DMA length is zero!

On ppc:

scsi host0: DMA length is zero!
scsi host0: cur adr[0fd21000] len[00000000]
scsi host0: Aborting command [(ptrval):28]
scsi host0: Current command [(ptrval):28]
scsi host0:  Active command [(ptrval):28]

On x86, x86_64 (after reverting a different crash-causing patch):

[   20.226809] scsi host0: DMA length is zero!
[   20.227459] scsi host0: cur adr[00000000] len[00000000]
[   50.588814] scsi host0: Aborting command [(____ptrval____):28]
[   50.589210] scsi host0: Current command [(____ptrval____):28]
[   50.589447] scsi host0:  Active command [(____ptrval____):28]
[   50.589674] scsi host0: Dumping command log

On m68k there is a crash.

Unable to handle kernel NULL pointer dereference at virtual address (ptrval)
Oops: 00000000
Modules linked in:
PC: [<00203a9e>] esp_maybe_execute_command+0x31e/0x46c
SR: 2704  SP: (ptrval)  a2: 07c1ea20
d0: 00000002    d1: 00000400    d2: 00000000    d3: 00002000
d4: 00000000    d5: 00000000    a0: 07db753c    a1: 00000000
Process kworker/0:0H (pid: 4, task=(ptrval))
Frame format=7 eff addr=00000020 ssw=0505 faddr=00000020
wb 1 stat/addr/data: 0000 00000000 00000000
wb 2 stat/addr/data: 0000 00000000 00000000
wb 3 stat/addr/data: 0000 00000020 00000000
push data: 00000000 00000000 00000000 00000000
Stack from 07c2be08:
...
Call Trace: [<00002000>] _start+0x0/0x8
  [<001f65ca>] scsi_mq_done+0x0/0x2c
  [<001887fe>] blk_mq_get_driver_tag+0x0/0xba
  [<00188800>] blk_mq_get_driver_tag+0x2/0xba
  [<00025aec>] create_worker+0x0/0x14e
  [<00203f64>] esp_queuecommand+0x9c/0xa2
  [<00203f3a>] esp_queuecommand+0x72/0xa2
  [<001f7fd4>] scsi_queue_rq+0x54e/0x5ba
  [<00188b4a>] blk_mq_dispatch_rq_list+0x292/0x38a
  [<00188656>] blk_mq_dequeue_from_ctx+0x0/0x1a8
  [<001888b8>] blk_mq_dispatch_rq_list+0x0/0x38a
  [<00025aec>] create_worker+0x0/0x14e
...

All those problems are fixed by reverting this patch.

Guenter