Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: t4ah0JnLxME02i3VUWS/Aygd2gVgcERWxTE1Ts6G0SXT2oEYWEFzL6YZORN0CWVk7IQ7lAoifG
 EkYoE782+Q3y2D3TWHObdGK4Umig1vdOcqWmOyfvP5kVqoKM/xKZRrDfmcr8X4h2xWVDgJMZZk
 qCrcBQnRFH9mZJhti82P/r1GbuhRWt7cmIsDbt+LHm45FwWMegXo+f0DD+J5+xo6GecTpL2hce
 VJr1O4tNil+xGfrAFgZw2r57BD3xzZx8+mDk74V419HGgs3vhBrSpqSzSLe9VcDy6+nGobwkRB
 3EzMWxj6vN0VAUuahL+xNFWY
IronPort-SDR: 85g9/+IvmZeey4X0sNaW7euWiWvdFWX9vWt8CVJ973lzPeX3jtA5EwbXuLaqHWQ1HqIKy9+NGe
 OatBZgw64pP2mFBBeCcyyHpytIh5IcNa8HEexdzNWzseB4M/tv7T0QgTVY6T7z6EPWEzJFtVhe
 wa9T5d13wWENi5BYd0GAh7hOXaRUFP++UtriKScFivZkTUBs3zPBM6kyWJoaYQ9So391XRFGuU
 qOyJKqceGKuanPeAy9iT3zev4isdpdlN+N37F+JahyoWjJj3HGHhRWG+Zfks8qP9zlksbt4ioK
 8s8=
WDCIronportException: Internal
Message-ID: <3f519a72-bb43-b1f0-c85d-a2ea4596f2f2@opensource.wdc.com>
Date:   Mon, 13 Jun 2022 16:03:59 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH RFC v2 04/18] scsi: core: Add support to send reserved
 commands
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, axboe@kernel.dk,
        jejb@linux.ibm.com, martin.petersen@oracle.com, brking@us.ibm.com,
        hare@suse.de, hch@lst.de
Cc:     linux-block@vger.kernel.org, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
        chenxiang66@hisilicon.com
References: <1654770559-101375-1-git-send-email-john.garry@huawei.com>
 <1654770559-101375-5-git-send-email-john.garry@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <1654770559-101375-5-git-send-email-john.garry@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/9/22 19:29, John Garry wrote:
> Add a method to queue reserved commands.
> 
> TODO:
> - fix timeout handler to call into reserved commands
> - We don't clear host_scribble for libata qc, but we should be able to drop
>   this if we store libata qc in the scsi cmnd priv_data
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hosts.c     |  6 ++++++
>  drivers/scsi/scsi_lib.c  | 32 ++++++++++++++++++++++++++++++++
>  include/scsi/scsi_cmnd.h |  5 +++++
>  include/scsi/scsi_host.h |  1 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 27296addaf63..5c9b05a8fec8 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -221,6 +221,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  		goto fail;
>  	}
>  
> +	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
> +		shost_printk(KERN_ERR, shost,
> +			"nr_reserved_cmds set but no method to queue\n");
> +		goto fail;

This would be a driver implementation bug. So what about a WARN() here ?

> +	}
> +
>  	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
>  	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>  				   shost->can_queue);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f6e53c6d913c..8c8b4c6767d9 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq)
>  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  	enum scsi_disposition disposition;
>  
> +	if (scsi_is_reserved_cmd(cmd)) {
> +		struct scsi_device *sdev = cmd->device;
> +
> +		scsi_mq_uninit_cmd(cmd);
> +		scsi_device_unbusy(sdev, cmd);
> +		__blk_mq_end_request(rq, 0);
> +
> +		return;
> +	}
> +
>  	INIT_LIST_HEAD(&cmd->eh_entry);
>  
>  	atomic_inc(&cmd->device->iodone_cnt);
> @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	WARN_ON_ONCE(cmd->budget_token < 0);
>  
> +	if (scsi_is_reserved_cmd(cmd)) {
> +		unsigned char *host_scribble = cmd->host_scribble;
> +
> +		if (!(req->rq_flags & RQF_DONTPREP)) {
> +			ret = scsi_prepare_cmd(req);
> +			if (ret != BLK_STS_OK) {
> +

Stray blank line.

> +				goto out_dec_host_busy;
> +			}

No need for the curly brackets here.

> +
> +			req->rq_flags |= RQF_DONTPREP;
> +		} else {
> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
> +		}
> +
> +		cmd->host_scribble = host_scribble;
> +
> +		blk_mq_start_request(req);
> +
> +		return shost->hostt->reserved_queuecommand(shost, cmd);
> +	}
> +
>  	/*
>  	 * If the device is not in running state we will reject some or all
>  	 * commands.
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 1e80e70dfa92..e47df5836070 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -152,6 +152,11 @@ static inline void *scsi_cmd_priv(struct scsi_cmnd *cmd)
>  	return cmd + 1;
>  }
>  
> +static inline bool scsi_is_reserved_cmd(struct scsi_cmnd *cmd)
> +{
> +	return blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd));
> +}
> +
>  void scsi_done(struct scsi_cmnd *cmd);
>  void scsi_done_direct(struct scsi_cmnd *cmd);
>  
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 149dcbd4125e..88c8504395c8 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -73,6 +73,7 @@ struct scsi_host_template {
>  	 * STATUS: REQUIRED
>  	 */
>  	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
> +	int (* reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
>  
>  	/*
>  	 * The commit_rqs function is used to trigger a hardware


-- 
Damien Le Moal
Western Digital Research