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@xxxxxxxxxxxx>
-
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
