Re: [PATCH v2] scsi_debug: fix sleep in invalid context
> "Doug" == Douglas Gilbert writes: Doug> In this post: Doug> http://www.spinics.net/lists/linux-scsi/msg97124.html the author Doug> shows some kernel infrastructure complaining about a sleep in an Doug> invalid context. Remove offending call to vmalloc(). Instead of Doug> using kzalloc() which reviewers didn't like, use a bucket system Doug> (64 bytes on the stack) and potentially multiple calls to Doug> sg_pcopy_from_buffer() to construct the 'data-in' buffer for the Doug> SCSI REPORT LUNS command. Applied to 4.8/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi_debug: fix sleep in invalid context
On 05/31/2016 02:15 PM, 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. Remove offending call to vmalloc(). Instead of using kzalloc() which reviewers didn't like, use a bucket system (64 bytes on the stack) and potentially multiple calls to sg_pcopy_from_buffer() to construct the 'data-in' buffer for the SCSI REPORT LUNS command. Reviewed-by: Bart Van Assche -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi_debug: fix sleep in invalid context
Oh well, not a fan og the casting, but as this is an important fix let's get it in for now: Acked-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi_debug: fix sleep in invalid context
On 2016-06-08 04:57 PM, Douglas Gilbert wrote: On 2016-06-07 09:55 PM, Christoph Hellwig wrote: +static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr, + int arr_len, unsigned int off_dst) +{ +int act_len, n; +struct scsi_data_buffer *sdb = scsi_in(scp); +off_t skip = off_dst; Why off_t which is a signed value instead of the unsigned in passed in? Because off_t is the type of the last argument to sg_pcopy_from_buffer() which is where the off_dst value goes. So there is potentially a size of integer change plus signedness, IMO best expressed by defining a new auto variable. I notice some projects have a uoff_t typedef for offsets that make no sense when negative (like this case), but not the Linux kernel. +#define RL_BUCKET_ELEMS 8 + /* Even though each pseudo target has a REPORT LUNS "well known logical unit" * (W-LUN), the normal Linux scanning logic does not associate it with a * device (e.g. /dev/sg7). The following magic will make that association: @@ -3285,12 +3315,14 @@ static int resp_report_luns(struct scsi_cmnd *scp, unsigned char select_report; u64 lun; struct scsi_lun *lun_p; -u8 *arr; +u8 arr[RL_BUCKET_ELEMS * sizeof(struct scsi_lun)]; just use an on-stack array of type struct scsi_lun here, e.g.: struct scsi_lun arr[RL_BUCKET_ELEMS]; Which you can then use directly instead of lun_p later, but which can also be passed p_fill_from_dev_buffer as that takes a void pointer. Can do. When I looked at doing this, it's not that simple. The first 8 byte "slot" is not a LUN, it's the response header, which just happens to be 8 bytes long. That is the reason for the comment above that loop: /* loops rely on sizeof response header same as sizeof lun (both 8) */ So IMO: 'struct scsi_lun arr[RL_BUCKET_ELEMS];' will work but is deceptive. IMO the v2 patch should stand, unless someone has an alternate patch. The original patch is also fine. This is a bug fix, not a new feature. Doug Gilbert -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi_debug: fix sleep in invalid context
On 2016-06-07 09:55 PM, Christoph Hellwig wrote: +static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr, + int arr_len, unsigned int off_dst) +{ + int act_len, n; + struct scsi_data_buffer *sdb = scsi_in(scp); + off_t skip = off_dst; Why off_t which is a signed value instead of the unsigned in passed in? Because off_t is the type of the last argument to sg_pcopy_from_buffer() which is where the off_dst value goes. So there is potentially a size of integer change plus signedness, IMO best expressed by defining a new auto variable. I notice some projects have a uoff_t typedef for offsets that make no sense when negative (like this case), but not the Linux kernel. +#define RL_BUCKET_ELEMS 8 + /* Even though each pseudo target has a REPORT LUNS "well known logical unit" * (W-LUN), the normal Linux scanning logic does not associate it with a * device (e.g. /dev/sg7). The following magic will make that association: @@ -3285,12 +3315,14 @@ static int resp_report_luns(struct scsi_cmnd *scp, unsigned char select_report; u64 lun; struct scsi_lun *lun_p; - u8 *arr; + u8 arr[RL_BUCKET_ELEMS * sizeof(struct scsi_lun)]; just use an on-stack array of type struct scsi_lun here, e.g.: struct scsi_lun arr[RL_BUCKET_ELEMS]; Which you can then use directly instead of lun_p later, but which can also be passed p_fill_from_dev_buffer as that takes a void pointer. Can do. Doug Gilbert -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi_debug: fix sleep in invalid context
> +static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr, > + int arr_len, unsigned int off_dst) > +{ > + int act_len, n; > + struct scsi_data_buffer *sdb = scsi_in(scp); > + off_t skip = off_dst; Why off_t which is a signed value instead of the unsigned in passed in? > +#define RL_BUCKET_ELEMS 8 > + > /* Even though each pseudo target has a REPORT LUNS "well known logical unit" > * (W-LUN), the normal Linux scanning logic does not associate it with a > * device (e.g. /dev/sg7). The following magic will make that association: > @@ -3285,12 +3315,14 @@ static int resp_report_luns(struct scsi_cmnd *scp, > unsigned char select_report; > u64 lun; > struct scsi_lun *lun_p; > - u8 *arr; > + u8 arr[RL_BUCKET_ELEMS * sizeof(struct scsi_lun)]; just use an on-stack array of type struct scsi_lun here, e.g.: struct scsi_lun arr[RL_BUCKET_ELEMS]; Which you can then use directly instead of lun_p later, but which can also be passed p_fill_from_dev_buffer as that takes a void pointer. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi_debug: fix sleep in invalid context
> "Doug" == Douglas Gilbert writes: Doug> In this post: Doug> http://www.spinics.net/lists/linux-scsi/msg97124.html the author Doug> shows some kernel infrastructure complaining about a sleep in an Doug> invalid context. Remove offending call to vmalloc(). Instead of Doug> using kzalloc() which reviewers didn't like, use a bucket system Doug> (64 bytes on the stack) and potentially multiple calls to Doug> sg_pcopy_from_buffer() to construct the 'data-in' buffer for the Doug> SCSI REPORT LUNS command. It would be great to get this reviewed... -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] scsi_debug: fix sleep in invalid context
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. Remove offending call to vmalloc(). Instead of using kzalloc() which reviewers didn't like, use a bucket system (64 bytes on the stack) and potentially multiple calls to sg_pcopy_from_buffer() to construct the 'data-in' buffer for the SCSI REPORT LUNS command. Signed-off-by: Douglas Gilbert --- drivers/scsi/scsi_debug.c | 93 +-- 1 file changed, 66 insertions(+), 27 deletions(-) diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 0f9ba41..6a219a0 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -890,7 +890,7 @@ static int make_ua(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) return 0; } -/* Returns 0 if ok else (DID_ERROR << 16). Sets scp->resid . */ +/* Build SCSI "data-in" buffer. Returns 0 if ok else (DID_ERROR << 16). */ static int fill_from_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr, int arr_len) { @@ -909,7 +909,35 @@ static int fill_from_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr, return 0; } -/* Returns number of bytes fetched into 'arr' or -1 if error. */ +/* Partial build of SCSI "data-in" buffer. Returns 0 if ok else + * (DID_ERROR << 16). Can write to offset in data-in buffer. If multiple + * calls, not required to write in ascending offset order. Assumes resid + * set to scsi_bufflen() prior to any calls. + */ +static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr, + int arr_len, unsigned int off_dst) +{ + int act_len, n; + struct scsi_data_buffer *sdb = scsi_in(scp); + off_t skip = off_dst; + + if (sdb->length <= off_dst) + return 0; + if (!(scsi_bidi_cmnd(scp) || scp->sc_data_direction == DMA_FROM_DEVICE)) + return DID_ERROR << 16; + + act_len = sg_pcopy_from_buffer(sdb->table.sgl, sdb->table.nents, + arr, arr_len, skip); + pr_debug("%s: off_dst=%u, scsi_bufflen=%u, act_len=%u, resid=%d\n", +__func__, off_dst, scsi_bufflen(scp), act_len, sdb->resid); + n = (int)scsi_bufflen(scp) - ((int)off_dst + act_len); + sdb->resid = min(sdb->resid, n); + return 0; +} + +/* Fetches from SCSI "data-out" buffer. Returns number of bytes fetched into + * 'arr' or -1 if error. + */ static int fetch_to_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr, int arr_len) { @@ -3269,6 +3297,8 @@ static int resp_get_lba_status(struct scsi_cmnd *scp, return fill_from_dev_buffer(scp, arr, SDEBUG_GET_LBA_STATUS_LEN); } +#define RL_BUCKET_ELEMS 8 + /* Even though each pseudo target has a REPORT LUNS "well known logical unit" * (W-LUN), the normal Linux scanning logic does not associate it with a * device (e.g. /dev/sg7). The following magic will make that association: @@ -3285,12 +3315,14 @@ static int resp_report_luns(struct scsi_cmnd *scp, unsigned char select_report; u64 lun; struct scsi_lun *lun_p; - u8 *arr; + u8 arr[RL_BUCKET_ELEMS * sizeof(struct scsi_lun)]; unsigned int lun_cnt; /* normal LUN count (max: 256) */ unsigned int wlun_cnt; /* report luns W-LUN count */ unsigned int tlun_cnt; /* total LUN count */ unsigned int rlen; /* response length (in bytes) */ - int i, res; + int k, j, n, res; + unsigned int off_rsp = 0; + const int sz_lun = sizeof(struct scsi_lun); clear_luns_changed_on_target(devip); @@ -3329,33 +3361,40 @@ static int resp_report_luns(struct scsi_cmnd *scp, --lun_cnt; tlun_cnt = lun_cnt + wlun_cnt; - - rlen = (tlun_cnt * sizeof(struct scsi_lun)) + 8; - arr = vmalloc(rlen); - if (!arr) { - mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC, - INSUFF_RES_ASCQ); - return check_condition_result; - } - memset(arr, 0, rlen); + rlen = tlun_cnt * sz_lun; /* excluding 8 byte header */ + scsi_set_resid(scp, scsi_bufflen(scp)); pr_debug("select_report %d luns = %d wluns = %d no_lun0 %d\n", select_report, lun_cnt, wlun_cnt, sdebug_no_lun_0); - /* luns start at byte 8 in response following the header */ - lun_p = (struct scsi_lun *)&arr[8]; - - /* LUNs use single level peripheral device addressing method */ + /* loops rely on sizeof response header same as sizeof lun (both 8) */ lun = sdebug_no_lun_0 ? 1 : 0; - for (i = 0; i < lun_cnt; i++) - int_to_scsilun(lun++, lun_p++); - - if (wlun_cnt) - int_to_scsilun(SCSI_W_LUN_REPORT_LUNS, lun_p++); - - put_unaligned_be32(rlen - 8, &arr[0]); - -