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

From: Ming Lei <ming.lei@redhat.com>
To: Hannes Reinecke <hare@suse.de>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	James Bottomley <james.bottomley@hansenpartnership.com>,
	John Garry <john.garry@huawei.com>,
	Bart van Assche <bvanassche@acm.org>,
	linux-scsi@vger.kernel.org, Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH RFC v3 06/41] virtio_scsi: use reserved commands for TMF
Date: Mon, 4 May 2020 18:35:38 +0800	[thread overview]
Message-ID: <20200504103538.GD1139563@T590> (raw)
In-Reply-To: <2957db37-3aca-ece5-23b1-0bceba45e10b@suse.de>

On Mon, May 04, 2020 at 12:21:57PM +0200, Hannes Reinecke wrote:
> On 5/4/20 11:25 AM, Ming Lei wrote:
> > 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)
> > 
> > virtscsi uses dedicated ->ctrl_vq to send TMF, and ->ctrl_vq isn't
> > nothing to do with vqs(->vqs[]) for sending IO request.
> > 
> Indeed, you are right.
> We should be handling this by adding a virtual LUN corresponding the the
> ctrl_vq and allocate commands from there.

I am not sure why we need this kind of change, and looks the current
code just works fine.

Thanks,
Ming