Re: [PATCH 06/17] scsi: add support for per-host cmd pools

[Mike Christie <michaelc@cs.wisc.edu>]

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

On 02/07/2014 06:46 AM, Christoph Hellwig wrote:
> On Fri, Feb 07, 2014 at 03:35:00AM -0600, Mike Christie wrote:
>> It seems there is a issue with using the cmd_size to indicate the driver
>> has its own cmd pool and also using that for scsi mq enabled drivers to
>> indicate that we want the LLD's struct allocated by blk/scsi mq.
>>
>> If a driver sets cmd_size for only the scsi/blk mq purpose, this patch
>> wants the driver to also setup a cmd pool which I do not think is used
>> when doing scsi/blk mq.
> 
> I don't quite understand what you mean.  cmd_size means that each
> scsi_cmnd passed to the driver will have additional per-driver data.
> 
> For the old path it's implemented by creating a slab cache and storing
> it in the host template to easily find it, for blk-mq it is used to
> increase the allocation in the block core as scsi doesn't do it's own
> allocations in this case.
> 

Ah ok. There is a bug then. I thought you were doing something crazy and
trying to do both at the same time for the same host, and that is why in
the other thread I was asking for a way to figure out where per-driver
data is.

The problem is that if a driver is using scsi-mq and sets cmd_size,
scsi-ml is trying to also setup a shost->cmd_pool. We do:

scsi_add_host_with_dma->scsi_setup_command_freelist->scsi_get_host_cmd_pool->
scsi_find_host_cmd_pool

and that function uses cmd_size to determine if we are using the driver
allocated hostt->cmd_pool or the scsi-ml caches. In the case of where we
are setting cmd_size because we want the mq code to setup the per driver
data we do not want any cmd_pool and have not set one up. The problem is
that scsi_find_host_cmd_pool then returns NULL and
scsi_get_host_cmd_pool does scsi_alloc_host_cmd_pool.

Later when scsi_destroy_command_freelist calls scsi_put_host_cmd_pool,
scsi_find_host_cmd_pool returns NULL again and we oops when we access
the pool in there.

We need something like the attached patch which just prevents scsi-ml
from creating a host pool when mq is used. Note that when
scsi_destroy_command_freelist is called shost->cmd_pool will be NULL so
it will return immediately so no extra check is needed in there.

[-- Attachment #2: dont-setup-host-list.patch --]
[-- Type: text/plain, Size: 529 bytes --]

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f28ea07..2924252 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -213,9 +213,11 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 	}
 
-	error = scsi_setup_command_freelist(shost);
-	if (error)
-		goto fail;
+	if (!sht->use_blk_mq) {
+		error = scsi_setup_command_freelist(shost);
+		if (error)
+			goto fail;
+	}
 
 	if (!shost->shost_gendev.parent)
 		shost->shost_gendev.parent = dev ? dev : &platform_bus;