Re: [PATCH 3/4] scsi: implement REQ_OP_ZONE_RESET_ALL

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Cc: "axboe@kernel.dk" <axboe@kernel.dk>,
	"jejb@linux.ibm.com" <jejb@linux.ibm.com>,
	"dennis@kernel.org" <dennis@kernel.org>,
	"hare@suse.com" <hare@suse.com>,
	"sagi@grimberg.me" <sagi@grimberg.me>,
	"dennisszhou@gmail.com" <dennisszhou@gmail.com>,
	"jthumshirn@suse.de" <jthumshirn@suse.de>,
	"osandov@fb.com" <osandov@fb.com>,
	"ming.lei@redhat.com" <ming.lei@redhat.com>,
	"tj@kernel.org" <tj@kernel.org>,
	"bvanassche@acm.org" <bvanassche@acm.org>,
	"martin.petersen@oracle.com" <martin.petersen@oracle.com>
Subject: Re: [PATCH 3/4] scsi: implement REQ_OP_ZONE_RESET_ALL
Date: Thu, 1 Aug 2019 01:00:51 +0000	[thread overview]
Message-ID: <BYAPR04MB581616F0196D93AE1C596735E7DE0@BYAPR04MB5816.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20190731210102.3472-4-chaitanya.kulkarni@wdc.com

On 2019/08/01 6:02, Chaitanya Kulkarni wrote:
> This patch implements the zone reset all operation for sd_zbc.c. We add
> a new boolean parameter for the sd_zbc_setup_reset_cmd() to indicate
> REQ_OP_ZONE_RESET_ALL command setup. Along with that we add support in
> the completion path for the zone reset all.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/scsi/sd.c     | 7 ++++++-
>  drivers/scsi/sd.h     | 5 +++--
>  drivers/scsi/sd_zbc.c | 9 +++++++--
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a3406bd62391..620a6d743952 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1292,7 +1292,9 @@ static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
>  	case REQ_OP_WRITE:
>  		return sd_setup_read_write_cmnd(cmd);
>  	case REQ_OP_ZONE_RESET:
> -		return sd_zbc_setup_reset_cmnd(cmd);
> +		return sd_zbc_setup_reset_cmnd(cmd, false);
> +	case REQ_OP_ZONE_RESET_ALL:
> +		return sd_zbc_setup_reset_cmnd(cmd, true);
>  	default:
>  		WARN_ON_ONCE(1);
>  		return BLK_STS_NOTSUPP;
> @@ -1958,6 +1960,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  	case REQ_OP_WRITE_ZEROES:
>  	case REQ_OP_WRITE_SAME:
>  	case REQ_OP_ZONE_RESET:
> +	case REQ_OP_ZONE_RESET_ALL:
>  		if (!result) {
>  			good_bytes = blk_rq_bytes(req);
>  			scsi_set_resid(SCpnt, 0);
> @@ -2948,6 +2951,8 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>  			 */
>  			q->limits.zoned = BLK_ZONED_NONE;
>  	}
> +	if (q->limits.zoned != BLK_ZONED_NONE)
> +		blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);

Move this into sd_zbc.c in sd_zbc_read_zones(). That avoids the "if" and keeps
things for ZBC in one place.

>  	if (blk_queue_is_zoned(q) && sdkp->first_scan)
>  		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
>  		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 38c50946fc42..1eab779f812b 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -209,7 +209,7 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
>  
>  extern int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
>  extern void sd_zbc_print_zones(struct scsi_disk *sdkp);
> -extern blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd);
> +extern blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd, bool all);
>  extern void sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
>  			    struct scsi_sense_hdr *sshdr);
>  extern int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
> @@ -225,7 +225,8 @@ static inline int sd_zbc_read_zones(struct scsi_disk *sdkp,
>  
>  static inline void sd_zbc_print_zones(struct scsi_disk *sdkp) {}
>  
> -static inline blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
> +static inline blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd,
> +						   bool all)
>  {
>  	return BLK_STS_TARGET;
>  }
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index db16c19e05c4..538216b9e1f4 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -209,10 +209,11 @@ static inline sector_t sd_zbc_zone_sectors(struct scsi_disk *sdkp)
>  /**
>   * sd_zbc_setup_reset_cmnd - Prepare a RESET WRITE POINTER scsi command.
>   * @cmd: the command to setup
> + * @all: flag to prepare a RESET ALL WRITE POINTER scsi command.

There is no "RESET ALL WRITE POINTER" command. It is the same
RESET WRITE POINTER, but with the ALL bit set. So maybe change the description
of the argument to something like:

@all: Reset all zones control

>   *
>   * Called from sd_init_command() for a REQ_OP_ZONE_RESET request.
>   */
> -blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
> +blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd, bool all)
>  {
>  	struct request *rq = cmd->request;
>  	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
> @@ -234,7 +235,10 @@ blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
>  	memset(cmd->cmnd, 0, cmd->cmd_len);
>  	cmd->cmnd[0] = ZBC_OUT;
>  	cmd->cmnd[1] = ZO_RESET_WRITE_POINTER;
> -	put_unaligned_be64(block, &cmd->cmnd[2]);
> +	if (all)
> +		cmd->cmnd[14] = 0x1;
> +	else
> +		put_unaligned_be64(block, &cmd->cmnd[2]);
>  
>  	rq->timeout = SD_TIMEOUT;
>  	cmd->sc_data_direction = DMA_NONE;
> @@ -261,6 +265,7 @@ void sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
>  
>  	switch (req_op(rq)) {
>  	case REQ_OP_ZONE_RESET:
> +	case REQ_OP_ZONE_RESET_ALL:
>  
>  		if (result &&
>  		    sshdr->sense_key == ILLEGAL_REQUEST &&
> 

-- 
Damien Le Moal
Western Digital Research