Re: [PATCH RFC v3 06/41] virtio_scsi: use reserved commands for TMF

[Ming Lei <ming.lei@redhat.com>]

On Thu, Apr 30, 2020 at 03:18:29PM +0200, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.com>
> 
> Set two commands aside for TMF, and use reserved commands to issue
> TMFs. With that we can drop the TMF memory pool.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/virtio_scsi.c | 105 ++++++++++++++++++---------------------------
>  1 file changed, 41 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 0e0910c5b942..26054c29d897 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -35,10 +35,10 @@
>  #define VIRTIO_SCSI_MEMPOOL_SZ 64
>  #define VIRTIO_SCSI_EVENT_LEN 8
>  #define VIRTIO_SCSI_VQ_BASE 2
> +#define VIRTIO_SCSI_RESERVED_CMDS 2
>  
>  /* Command queue element */
>  struct virtio_scsi_cmd {
> -	struct scsi_cmnd *sc;
>  	struct completion *comp;
>  	union {
>  		struct virtio_scsi_cmd_req       cmd;
> @@ -86,9 +86,6 @@ struct virtio_scsi {
>  	struct virtio_scsi_vq req_vqs[];
>  };
>  
> -static struct kmem_cache *virtscsi_cmd_cache;
> -static mempool_t *virtscsi_cmd_pool;
> -
>  static inline struct Scsi_Host *virtio_scsi_host(struct virtio_device *vdev)
>  {
>  	return vdev->priv;
> @@ -108,7 +105,7 @@ static void virtscsi_compute_resid(struct scsi_cmnd *sc, u32 resid)
>  static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
>  {
>  	struct virtio_scsi_cmd *cmd = buf;
> -	struct scsi_cmnd *sc = cmd->sc;
> +	struct scsi_cmnd *sc = scsi_cmd_from_priv(cmd);
>  	struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd;
>  
>  	dev_dbg(&sc->device->sdev_gendev,
> @@ -406,7 +403,7 @@ static int __virtscsi_add_cmd(struct virtqueue *vq,
>  			    struct virtio_scsi_cmd *cmd,
>  			    size_t req_size, size_t resp_size)
>  {
> -	struct scsi_cmnd *sc = cmd->sc;
> +	struct scsi_cmnd *sc = scsi_cmd_from_priv(cmd);
>  	struct scatterlist *sgs[6], req, resp;
>  	struct sg_table *out, *in;
>  	unsigned out_num = 0, in_num = 0;
> @@ -557,8 +554,6 @@ static int virtscsi_queuecommand(struct Scsi_Host *shost,
>  	dev_dbg(&sc->device->sdev_gendev,
>  		"cmd %p CDB: %#02x\n", sc, sc->cmnd[0]);
>  
> -	cmd->sc = sc;
> -
>  	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>  
>  #ifdef CONFIG_BLK_DEV_INTEGRITY
> @@ -590,17 +585,17 @@ static int virtscsi_queuecommand(struct Scsi_Host *shost,
>  static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  {
>  	DECLARE_COMPLETION_ONSTACK(comp);
> -	int ret = FAILED;
>  
>  	cmd->comp = &comp;
> +
>  	if (virtscsi_add_cmd(&vscsi->ctrl_vq, cmd,
>  			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf, true) < 0)
> -		goto out;
> +		return FAILED;
>  
>  	wait_for_completion(&comp);
>  	if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
>  	    cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
> -		ret = SUCCESS;
> +		return SUCCESS;
>  
>  	/*
>  	 * The spec guarantees that all requests related to the TMF have
> @@ -613,33 +608,36 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  	 * REQ_ATOM_COMPLETE has been set.
>  	 */
>  	virtscsi_poll_requests(vscsi);
> -
> -out:
> -	mempool_free(cmd, virtscsi_cmd_pool);
> -	return ret;
> +	return FAILED;
>  }
>  
>  static int virtscsi_device_reset(struct scsi_cmnd *sc)
>  {
> +	struct scsi_device *sdev = sc->device;
> +	struct scsi_cmnd *reset_sc;
>  	struct virtio_scsi *vscsi = shost_priv(sc->device->host);
>  	struct virtio_scsi_cmd *cmd;
> +	int rc;
>  
> -	sdev_printk(KERN_INFO, sc->device, "device reset\n");
> -	cmd = mempool_alloc(virtscsi_cmd_pool, GFP_NOIO);
> -	if (!cmd)
> +	sdev_printk(KERN_INFO, sdev, "device reset\n");
> +	reset_sc = scsi_get_reserved_cmd(sdev, DMA_NONE);
> +	if (!reset_sc)
>  		return FAILED;
> -
> +	cmd = scsi_cmd_priv(reset_sc);
>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->req.tmf = (struct virtio_scsi_ctrl_tmf_req){
>  		.type = VIRTIO_SCSI_T_TMF,
>  		.subtype = cpu_to_virtio32(vscsi->vdev,
>  					     VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET),
>  		.lun[0] = 1,
> -		.lun[1] = sc->device->id,
> -		.lun[2] = (sc->device->lun >> 8) | 0x40,
> -		.lun[3] = sc->device->lun & 0xff,
> +		.lun[1] = sdev->id,
> +		.lun[2] = (sdev->lun >> 8) | 0x40,
> +		.lun[3] = sdev->lun & 0xff,
>  	};
> -	return virtscsi_tmf(vscsi, cmd);
> +	rc = virtscsi_tmf(vscsi, cmd);
> +	scsi_put_reserved_cmd(reset_sc);
> +
> +	return rc;
>  }
>  
>  static int virtscsi_device_alloc(struct scsi_device *sdevice)
> @@ -679,25 +677,31 @@ static int virtscsi_change_queue_depth(struct scsi_device *sdev, int qdepth)
>  
>  static int virtscsi_abort(struct scsi_cmnd *sc)
>  {
> -	struct virtio_scsi *vscsi = shost_priv(sc->device->host);
> +	struct scsi_device *sdev = sc->device;
> +	struct scsi_cmnd *reset_sc;
> +	struct virtio_scsi *vscsi = shost_priv(sdev->host);
>  	struct virtio_scsi_cmd *cmd;
> +	int rc;
>  
>  	scmd_printk(KERN_INFO, sc, "abort\n");
> -	cmd = mempool_alloc(virtscsi_cmd_pool, GFP_NOIO);
> -	if (!cmd)
> +	reset_sc = scsi_get_reserved_cmd(sdev, DMA_NONE);
> +	if (!reset_sc)
>  		return FAILED;
> +	cmd = scsi_cmd_priv(reset_sc);
>  
>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->req.tmf = (struct virtio_scsi_ctrl_tmf_req){
>  		.type = VIRTIO_SCSI_T_TMF,
>  		.subtype = VIRTIO_SCSI_T_TMF_ABORT_TASK,
>  		.lun[0] = 1,
> -		.lun[1] = sc->device->id,
> -		.lun[2] = (sc->device->lun >> 8) | 0x40,
> -		.lun[3] = sc->device->lun & 0xff,
> +		.lun[1] = sdev->id,
> +		.lun[2] = (sdev->lun >> 8) | 0x40,
> +		.lun[3] = sdev->lun & 0xff,
>  		.tag = cpu_to_virtio64(vscsi->vdev, (unsigned long)sc),
>  	};
> -	return virtscsi_tmf(vscsi, cmd);
> +	rc = virtscsi_tmf(vscsi, cmd);
> +	scsi_put_reserved_cmd(reset_sc);
> +	return rc;
>  }
>  
>  static int virtscsi_map_queues(struct Scsi_Host *shost)
> @@ -865,6 +869,11 @@ static int virtscsi_probe(struct virtio_device *vdev)
>  		goto virtscsi_init_failed;
>  
>  	shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
> +	shost->can_queue -= VIRTIO_SCSI_RESERVED_CMDS;

I guess it should have been '+=', right?

Thanks,
Ming