scsi_debug: illegal blocking memory allocation

Hi Doug,

resp_inquiry() does a GFP_KERNEL memory allocation, but it's not allowed to from the queuecommand context. There's no good way to return this error, so I used DID_ERROR which is used from similar paths. That doesn't seem quite right though, it would be better to return an error indicating a later retry would be more appropriate.

Signed-off-by Jens Axboe <[email protected]>

drivers/scsi/scsi_debug.c +0 -0

index 30ee3d7..0c80ed3 100644

Quoted 16 lines · Jens Axboe

Hi Doug,

resp_inquiry() does a GFP_KERNEL memory allocation, but it's not allowed to from the queuecommand context. There's no good way to return this error, so I used DID_ERROR which is used from similar paths. That doesn't seem quite right though, it would be better to return an error indicating a later retry would be more appropriate.

Signed-off-by Jens Axboe <[email protected]>

drivers/scsi/scsi_debug.c +0 -0

index 30ee3d7..0c80ed3 100644

drivers/scsi/scsi_debug.c static int resp_inquiry(struct scsi_cmnd * scp, int target, · line 962

Quoted 10 lines

drivers/scsi/scsi_debug.c +3 -1

@@ -954,7 +954,9 @@ static int resp_inquiry(struct scsi_cmnd * scp, int target,
 	int alloc_len, n, ret;
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
-	arr = kzalloc(SDEBUG_MAX_INQ_ARR_SZ, GFP_KERNEL);
+	arr = kzalloc(SDEBUG_MAX_INQ_ARR_SZ, GFP_ATOMIC);
+	if (!arr)
+		return DID_ERROR << 16;
 	if (devip->wlun)
 		pq_pdt = 0x1e;	/* present, wlun */
 	else if (scsi_debug_no_lun_0 && (0 == devip->lun))

Jens,
I had to read that twice. I'm always happy to convert a GFP_KERNEL to a GFP_ATOMIC (as I'm sure it started as a GFP_ATOMIC). There are a couple more that may be in queuecommand context.

Taking up your point about retries and seeing that the scsi_debug driver has a SAS flavour, I'm inclined towards a "aborted command, initiator response timeout" [Bh,4Bh,6] CHECK CONDITION. There is a group of transport injected error messages in SAS (see sas2r07.pdf section 10.2.3) that pop up from time to time. Due to conjestion in connection-switched SAS expanders these error messages should be interpreted as "try again" depending on the topology. The patch below adds a "aborted_command" bit in opts that will cause every nth command to be aborted (with an ack/nak timeout).

Note that SAS has an optional "transport layer retries" state machine to lessen the incidence of "aborted commands". Evidently SAS tape drives use the facility.

I guess it's fully up to you how you want to solve it. The scheme seems a little elaborate, but these error conditions are unlikely to ever been seen in the wild, so no objections from me.

Quoted 3 lines · Jens Axboe

I guess it's fully up to you how you want to solve it. The scheme seems a little elaborate, but these error conditions are unlikely to ever been seen in the wild, so no objections from me.

Actually, there's already a DID_ code that does what you want. Instead of DID_ERROR, which will retry immediately, there's DID_REQUEUE which will halt the device queue and wait for a returning command to retry.

Any returning command, or a returning command from this device?

We may
be out of memory from a different device, and this may be the only command being sent to the scsi_debug driver. Wouldn't we end up halting the device queue indefinitely then?

Quoted 3 lines · Jens Axboe

I guess it's fully up to you how you want to solve it. The scheme seems a little elaborate, but these error conditions are unlikely to ever been seen in the wild, so no objections from me.

Actually, there's already a DID_ code that does what you want. Instead of DID_ERROR, which will retry immediately, there's DID_REQUEUE which will halt the device queue and wait for a returning command to retry.

Quoted 7 lines · James Bottomley

Quoted 3 lines · Jens Axboe

I guess it's fully up to you how you want to solve it. The scheme seems a little elaborate, but these error conditions are unlikely to ever been seen in the wild, so no objections from me.

Actually, there's already a DID_ code that does what you want. Instead of DID_ERROR, which will retry immediately, there's DID_REQUEUE which will halt the device queue and wait for a returning command to retry.

As long as it keeps firing the queue at some intervals even without any commands pending at all, then that'll work just fine. I like that approach a lot better than coding the error into some sense value that is (at best) some vague approximation of what has happened (calling memory shortage a transport error is a bit of a stretch).

Quoted 9 lines · Jens Axboe

Quoted 7 lines · James Bottomley

Quoted 3 lines · Jens Axboe

I guess it's fully up to you how you want to solve it. The scheme seems a little elaborate, but these error conditions are unlikely to ever been seen in the wild, so no objections from me.

Actually, there's already a DID_ code that does what you want. Instead of DID_ERROR, which will retry immediately, there's DID_REQUEUE which will halt the device queue and wait for a returning command to retry.

Quoted 5 lines

As long as it keeps firing the queue at some intervals even without any commands pending at all, then that'll work just fine. I like that approach a lot better than coding the error into some sense value that is (at best) some vague approximation of what has happened (calling memory shortage a transport error is a bit of a stretch).

True, but both happen. The scsi_debug driver is a virtual host, virtual target and a lu (ram disk). The failure that you pointed out stopped a response being built. In the real world that would in the target or lu.