Re: [PATCH v3 12/20] sg: sense buffer rework

[Douglas Gilbert <dgilbert@interlog.com>]

On 2019-08-12 10:37 a.m., Christoph Hellwig wrote:
> On Wed, Aug 07, 2019 at 01:42:44PM +0200, 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>
>>
>> -
>>
>> The scsi_lib.c file in the scsi mid-level maintains two sense
>> buffer caches but declares them and their access functions
>> static so they can't use by the sg driver. Perhaps the fastest
>> option would be to transfer ownership of a (non-empty) sense
>> buffer from the scsi_lib.c file to the sg driver. This technique
>> may be useful to ther ULDs.
> 
> Why do you need your own storage for the sense buffer?  As soon
> as you have allocate the request/scsi_request you can use its
> sense buffer.  There shouldn't really be a need to keep a copy
> around.

Different lifetimes between the corresponding struct request and
struct scsi_request objects, on one hand, and a sg_request object on
the other. The former are freed in sg_rq_end_io() (i.e. the callback
from the mid-level) while sg_request object must keep them until the
user space completion (e.g. calls to read() or ioctl(SG_IORECEIVE)).

This comment was left by you (?) or Jens in sg_rq_end_io():
         /*
          * Free the mid-level resources apart from the bio (if any). The bio's
          * blk_rq_unmap_user() can be called later from user context.
          */

I certainly didn't put it there.

Anyway Hannes didn't like a kmalloc(GFP_ATOMIC) in the callback
and suggested a mempool. Given that the sense buffer is not
required very often, I felt pre-allocating space in a
sg_request object was wasteful.  Got any better ideas?

One idea I floated was making these guys in scsi_lib:
   static struct kmem_cache *scsi_sense_cache;
   static struct kmem_cache *scsi_sense_isadma_cache;

... accessible to ULDs (because the st driver may benefit also) to
transfer ownership and free it when it was no longer needed. This
would allow the sg driver to transfer ownership of the sense
buffer in scsi_request before that scsi_request was "freed". Then
the sg_request could free up that sense_buffer when it was no
longer needed. No-one responded to that idea.


Doug Gilbert