[PATCH] scsi_debug: fix sleep in invalid context

[PATCH] scsi_debug: fix sleep in invalid context

[Douglas Gilbert]

In this post: http://www.spinics.net/lists/linux-scsi/msg97124.html
the author shows some kernel infrastructure complaining about a
sleep in an invalid context. For calls to fetch memory when
processing SCSI commands, reviewers often propose non GFP_ATOMIC
variants; reality dictates otherwise. Fix memory allocation for
response to REPORT LUNS command.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_debug.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0f9ba41..b85c5dc 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3331,13 +3331,12 @@ static int resp_report_luns(struct scsi_cmnd *scp,
 	tlun_cnt = lun_cnt + wlun_cnt;
 
 	rlen = (tlun_cnt * sizeof(struct scsi_lun)) + 8;
-	arr = vmalloc(rlen);
+	arr = kzalloc(rlen, GFP_ATOMIC);
 	if (!arr) {
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
 				INSUFF_RES_ASCQ);
 		return check_condition_result;
 	}
-	memset(arr, 0, rlen);
 	pr_debug("select_report %d luns = %d wluns = %d no_lun0 %d\n",
 		 select_report, lun_cnt, wlun_cnt, sdebug_no_lun_0);
 
@@ -3355,7 +3354,7 @@ static int resp_report_luns(struct scsi_cmnd *scp,
 	put_unaligned_be32(rlen - 8, &arr[0]);
 
 	res = fill_from_dev_buffer(scp, arr, rlen);
-	vfree(arr);
+	kfree(arr);
 	return res;
 }
 
-- 
2.7.4

Re: [PATCH] scsi_debug: fix sleep in invalid context

[James Bottomley]

On Mon, 2016-05-30 at 14:19 -0400, Douglas Gilbert wrote:
> In this post: http://www.spinics.net/lists/linux-scsi/msg97124.html
> the author shows some kernel infrastructure complaining about a
> sleep in an invalid context. For calls to fetch memory when
> processing SCSI commands, reviewers often propose non GFP_ATOMIC
> variants; reality dictates otherwise. Fix memory allocation for
> response to REPORT LUNS command.
> 
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
> ---
>  drivers/scsi/scsi_debug.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 0f9ba41..b85c5dc 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3331,13 +3331,12 @@ static int resp_report_luns(struct scsi_cmnd
> *scp,
>  	tlun_cnt = lun_cnt + wlun_cnt;
>  
>  	rlen = (tlun_cnt * sizeof(struct scsi_lun)) + 8;
> -	arr = vmalloc(rlen);
> +	arr = kzalloc(rlen, GFP_ATOMIC);
>  	if (!arr) {
>  		mk_sense_buffer(scp, ILLEGAL_REQUEST,
> INSUFF_RES_ASC,
>  				INSUFF_RES_ASCQ);
>  		return check_condition_result;
>  	}
> -	memset(arr, 0, rlen);
>  	pr_debug("select_report %d luns = %d wluns = %d no_lun0
> %d\n",
>  		 select_report, lun_cnt, wlun_cnt, sdebug_no_lun_0);
>  
> @@ -3355,7 +3354,7 @@ static int resp_report_luns(struct scsi_cmnd
> *scp,
>  	put_unaligned_be32(rlen - 8, &arr[0]);
>  
>  	res = fill_from_dev_buffer(scp, arr, rlen);
> -	vfree(arr);
> +	kfree(arr);

This might fix the immediate warning, but won't it demand huge
contiguous memory chunks in high lun configurations and thus fail
randomly?  Report luns is important to us because if that fails the
target won't attach.

What about vmalloc'ing enough space at configuration time, when you do
have process context, and simply reusing the already allocated buffer
in this routine?  If you want to be clever, you could do a single
vmalloc for the biggest LUN size you have and reuse that buffer for
every report lun command with suitable locking ... we tend to fire off
report luns sequentially at start of day, so it's not like they have
huge performance or concurrency requirements.

James

Re: [PATCH] scsi_debug: fix sleep in invalid context

[Christoph Hellwig]

On Mon, May 30, 2016 at 12:02:45PM -0700, James Bottomley wrote:
> This might fix the immediate warning, but won't it demand huge
> contiguous memory chunks in high lun configurations and thus fail
> randomly?  Report luns is important to us because if that fails the
> target won't attach.
> 
> What about vmalloc'ing enough space at configuration time, when you do
> have process context, and simply reusing the already allocated buffer
> in this routine?  If you want to be clever, you could do a single
> vmalloc for the biggest LUN size you have and reuse that buffer for
> every report lun command with suitable locking ... we tend to fire off
> report luns sequentially at start of day, so it's not like they have
> huge performance or concurrency requirements.

There is no need for the allocation at all.  Instead of the big
array and a single call to fill_from_dev_buffer we can simply
have a single scsi_lun structure on stack that gets reused for
every lun and individual calls to sg_copy_from_buffer for each
one instead of a single big one.

Re: [PATCH] scsi_debug: fix sleep in invalid context

[Douglas Gilbert]

On 2016-05-30 03:09 PM, Christoph Hellwig wrote:
> On Mon, May 30, 2016 at 12:02:45PM -0700, James Bottomley wrote:
>> This might fix the immediate warning, but won't it demand huge
>> contiguous memory chunks in high lun configurations and thus fail
>> randomly?  Report luns is important to us because if that fails the
>> target won't attach.
>>
>> What about vmalloc'ing enough space at configuration time, when you do
>> have process context, and simply reusing the already allocated buffer
>> in this routine?  If you want to be clever, you could do a single
>> vmalloc for the biggest LUN size you have and reuse that buffer for
>> every report lun command with suitable locking ... we tend to fire off
>> report luns sequentially at start of day, so it's not like they have
>> huge performance or concurrency requirements.
>
> There is no need for the allocation at all.  Instead of the big
> array and a single call to fill_from_dev_buffer we can simply
> have a single scsi_lun structure on stack that gets reused for
> every lun and individual calls to sg_copy_from_buffer for each
> one instead of a single big one.

Christoph,
Interesting idea but a time waster due to:
    s/sg_copy_from_buffer/sg_pcopy_from_buffer/

Otherwise the start of the sgl is overwritten by each subsequent
call. The "p" variant has a trailing 'off_t skip' argument and
currently there are no helper functions in scsi_cmnd.h for the
"pcopy" variants. Also handling the resid is fiddly, not helped by
initializing it to zero rather than scsi_bufflen().

Doug Gilbert