Re: [PATCH v4 12/22] sg: sense buffer rework

[Douglas Gilbert <dgilbert@interlog.com>]

On 2019-09-09 11:01 a.m., Hannes Reinecke wrote:
> On 8/29/19 4:26 AM, Douglas Gilbert wrote:
>> The biggest single item in the sg_request object is the sense
>> buffer array which is SCSI_SENSE_BUFFERSIZE bytes long. That
>> constant started out at 18 bytes 20 years ago and is 96 bytes
>> now and might grow in the future. On the other hand the sense
>> buffer is only used by a small number of SCSI commands: those
>> that fail and those that want to return more information
>> other than a SCSI status of GOOD.
>>
>> Set up a small mempool called "sg_sense" that is only used as
>> required and released back to the mempool as soon as practical.
>>
>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
>> ---
>>   drivers/scsi/sg.c | 86 ++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 73 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 2e476e74130b..ebb9d4f53177 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -72,6 +72,10 @@ static char *sg_version_date = "20190606";
>>    */
>>   #define SG_MAX_CDB_SIZE 252
>>   
>> +static struct kmem_cache *sg_sense_cache;
>> +#define SG_MEMPOOL_MIN_NR 4
>> +static mempool_t *sg_sense_pool;
>> +
>>   #define uptr64(val) ((void __user *)(uintptr_t)(val))
>>   #define cuptr64(val) ((const void __user *)(uintptr_t)(val))
>>   
>> @@ -176,7 +180,6 @@ struct sg_request {	/* active SCSI command or inactive on free list (fl) */
>>   	spinlock_t req_lck;
>>   	struct sg_scatter_hold sgat_h;	/* hold buffer, perhaps scatter list */
>>   	struct sg_slice_hdr3 s_hdr3;  /* subset of sg_io_hdr */
>> -	u8 sense_b[SCSI_SENSE_BUFFERSIZE];
>>   	u32 duration;		/* cmd duration in milliseconds */
>>   	u32 rq_flags;		/* hold user supplied flags */
>>   	u32 rq_info;		/* info supplied by v3 and v4 interfaces */
>> @@ -188,6 +191,7 @@ struct sg_request {	/* active SCSI command or inactive on free list (fl) */
>>   	u8 cmd_opcode;		/* first byte of SCSI cdb */
>>   	u64 start_ns;		/* starting point of command duration calc */
>>   	unsigned long frq_bm[1];	/* see SG_FRQ_* defines above */
>> +	u8 *sense_bp;		/* mempool alloc-ed sense buffer, as needed */
>>   	struct sg_fd *parentfp;	/* pointer to owning fd, even when on fl */
>>   	struct request *rq;	/* released in sg_rq_end_io(), bio kept */
>>   	struct bio *bio;	/* kept until this req -->SG_RS_INACTIVE */
>> @@ -845,18 +849,21 @@ sg_copy_sense(struct sg_request *srp)
>>   	    (driver_byte(srp->rq_result) & DRIVER_SENSE)) {
>>   		int sb_len = min_t(int, SCSI_SENSE_BUFFERSIZE, srp->sense_len);
>>   		int mx_sb_len;
>> +		u8 *sbp = srp->sense_bp;
>>   		void __user *up;
>>   
>> +		srp->sense_bp = NULL;
>>   		up = (void __user *)srp->s_hdr3.sbp;
>>   		mx_sb_len = srp->s_hdr3.mx_sb_len;
>> -		if (up && mx_sb_len > 0) {
>> +		if (up && mx_sb_len > 0 && sbp) {
>>   			sb_len = min_t(int, sb_len, mx_sb_len);
>>   			/* Additional sense length field */
>> -			sb_len_wr = 8 + (int)srp->sense_b[7];
>> +			sb_len_wr = 8 + (int)sbp[7];
>>   			sb_len_wr = min_t(int, sb_len, sb_len_wr);
>> -			if (copy_to_user(up, srp->sense_b, sb_len_wr))
>> +			if (copy_to_user(up, sbp, sb_len_wr))
>>   				sb_len_wr = -EFAULT;
>>   		}
>> +		mempool_free(sbp, sg_sense_pool);
>>   	}
>>   	return sb_len_wr;
>>   }
>> @@ -963,8 +970,14 @@ sg_rd_v1v2(void __user *buf, int count, struct sg_fd *sfp,
>>   	h2p->driver_status = driver_byte(rq_result);
>>   	if ((CHECK_CONDITION & status_byte(rq_result)) ||
>>   	    (DRIVER_SENSE & driver_byte(rq_result))) {
>> -		memcpy(h2p->sense_buffer, srp->sense_b,
>> -		       sizeof(h2p->sense_buffer));
>> +		if (srp->sense_bp) {
>> +			u8 *sbp = srp->sense_bp;
>> +
>> +			srp->sense_bp = NULL;
>> +			memcpy(h2p->sense_buffer, sbp,
>> +			       sizeof(h2p->sense_buffer));
>> +			mempool_free(sbp, sg_sense_pool);
>> +		}
>>   	}
>>   	switch (host_byte(rq_result)) {
>>   	/*
>> @@ -999,17 +1012,21 @@ sg_rd_v1v2(void __user *buf, int count, struct sg_fd *sfp,
>>   
>>   	/* Now copy the result back to the user buffer.  */
>>   	if (count >= SZ_SG_HEADER) {
>> -		if (copy_to_user(buf, h2p, SZ_SG_HEADER))
>> -			return -EFAULT;
>> +		if (copy_to_user(buf, h2p, SZ_SG_HEADER)) {
>> +			res = -EFAULT;
>> +			goto fini;
>> +		}
>>   		buf += SZ_SG_HEADER;
>>   		if (count > h2p->reply_len)
>>   			count = h2p->reply_len;
>>   		if (count > SZ_SG_HEADER) {
>> -			if (sg_rd_append(srp, buf, count - SZ_SG_HEADER))
>> -				return -EFAULT;
>> +			res = sg_rd_append(srp, buf, count - SZ_SG_HEADER);
>> +			if (res)
>> +				goto fini;
>>   		}
>>   	} else
>>   		res = (h2p->result == 0) ? 0 : -EIO;
>> +fini:
>>   	atomic_set(&srp->rq_st, SG_RS_DONE_RD);
>>   	sg_finish_scsi_blk_rq(srp);
>>   	sg_deact_request(sfp, srp);
>> @@ -1971,8 +1988,17 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
>>   	srp->duration = sg_calc_rq_dur(srp);
>>   	if (unlikely((srp->rq_result & SG_ML_RESULT_MSK) && slen > 0))
>>   		sg_check_sense(sdp, srp, slen);
>> -	if (slen > 0)
>> -		memcpy(srp->sense_b, scsi_rp->sense, slen);
>> +	if (slen > 0) {
>> +		if (scsi_rp->sense) {
>> +			srp->sense_bp = mempool_alloc(sg_sense_pool,
>> +						      GFP_ATOMIC);
>> +			if (srp->sense_bp)
>> +				memcpy(srp->sense_bp, scsi_rp->sense, slen);
>> +		} else {
>> +			pr_warn("%s: scsi_request::sense==NULL\n", __func__);
>> +			slen = 0;
>> +		}
>> +	}
>>   	srp->sense_len = slen;
>>   	if (unlikely(test_bit(SG_FRQ_IS_ORPHAN, srp->frq_bm))) {
>>   		spin_lock(&srp->req_lck);
> That looks ... odd.
> What happens if 'mempool_alloc()' fails?
> Shouldn't we set 'slen' to '0' here, too?

Indeed. Fixed in v5 with a pr_warn() added, like the other failed branch.
Also make the pr_warn messages more precise.

Doug Gilbert