Re: [PATCH 3/4] scsi: sd_zbc: add zone open, close, and finish support

[Damien Le Moal]

On 2019/06/21 22:07, Matias Bjørling wrote:
> From: Ajay Joshi <ajay.joshi@xxxxxxx>
> 
> Implement REQ_OP_ZONE_OPEN, REQ_OP_ZONE_CLOSE and REQ_OP_ZONE_FINISH
> support to allow explicit control of zone states.
> 
> Signed-off-by: Ajay Joshi <ajay.joshi@xxxxxxx>
> ---
>  drivers/scsi/sd.c     | 15 ++++++++++++++-
>  drivers/scsi/sd.h     |  6 ++++--
>  drivers/scsi/sd_zbc.c | 18 +++++++++++++-----
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a3406bd62391..89f955a01d44 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1292,7 +1292,17 @@ 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_zone_mgmt_op_cmnd(cmd,
> +					ZO_RESET_WRITE_POINTER);
> +	case REQ_OP_ZONE_OPEN:
> +		return sd_zbc_setup_zone_mgmt_op_cmnd(cmd,
> +					ZO_OPEN_ZONE);
> +	case REQ_OP_ZONE_CLOSE:
> +		return sd_zbc_setup_zone_mgmt_op_cmnd(cmd,
> +					ZO_CLOSE_ZONE);
> +	case REQ_OP_ZONE_FINISH:
> +		return sd_zbc_setup_zone_mgmt_op_cmnd(cmd,
> +					ZO_FINISH_ZONE);
>  	default:
>  		WARN_ON_ONCE(1);
>  		return BLK_STS_NOTSUPP;
> @@ -1958,6 +1968,9 @@ 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_OPEN:
> +	case REQ_OP_ZONE_CLOSE:
> +	case REQ_OP_ZONE_FINISH:
>  		if (!result) {
>  			good_bytes = blk_rq_bytes(req);
>  			scsi_set_resid(SCpnt, 0);
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 5796ace76225..9a20633caefa 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -209,7 +209,8 @@ 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_zone_mgmt_op_cmnd(struct scsi_cmnd *cmd,
> +						   unsigned char op);

In ZBC specs, open, close, finish and reset command are all ZBC_OUT (94h)
commands with a different servie action. So may be renaming this function to
sd_zbc_setup_zbc_out_cmnd() is better.

>  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,
> @@ -226,7 +227,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_zone_mgmt_op_cmnd(struct scsi_cmnd *cmd,
> +							  unsigned char op)
>  {
>  	return BLK_STS_TARGET;
>  }
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 7334024b64f1..41020db5353a 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -168,12 +168,17 @@ 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
> + * sd_zbc_setup_zone_mgmt_op_cmnd - Prepare a zone ZBC_OUT command. The
> + *                                  operations can be RESET WRITE POINTER,
> + *                                  OPEN, CLOSE or FINISH.
> + * @cmd: The command to setup
> + * @op: Operation to be performed
>   *
> - * Called from sd_init_command() for a REQ_OP_ZONE_RESET request.
> + * Called from sd_init_command() for REQ_OP_ZONE_RESET, REQ_OP_ZONE_OPEN,
> + * REQ_OP_ZONE_CLOSE or REQ_OP_ZONE_FINISH requests.
>   */
> -blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
> +blk_status_t sd_zbc_setup_zone_mgmt_op_cmnd(struct scsi_cmnd *cmd,
> +					    unsigned char op)
>  {
>  	struct request *rq = cmd->request;
>  	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
> @@ -194,7 +199,7 @@ blk_status_t sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
>  	cmd->cmd_len = 16;
>  	memset(cmd->cmnd, 0, cmd->cmd_len);
>  	cmd->cmnd[0] = ZBC_OUT;
> -	cmd->cmnd[1] = ZO_RESET_WRITE_POINTER;
> +	cmd->cmnd[1] = op;
>  	put_unaligned_be64(block, &cmd->cmnd[2]);
>  
>  	rq->timeout = SD_TIMEOUT;
> @@ -222,6 +227,9 @@ 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_OPEN:
> +	case REQ_OP_ZONE_CLOSE:
> +	case REQ_OP_ZONE_FINISH:
>  
>  		if (result &&
>  		    sshdr->sense_key == ILLEGAL_REQUEST &&

The comment after this code references only the reset operation. So it needs to
be updated. The same comment applies to all operations as they all have the same
error return behavior.

-- 
Damien Le Moal
Western Digital Research