Re: [PATCH v3 06/10] scsi: sd_zbc: emulate ZONE_APPEND commands

[Christoph Hellwig <hch@infradead.org>]

> Since zone reset and finish operations can be issued concurrently with
> writes and zone append requests, ensure a coherent update of the zone
> write pointer offsets by also write locking the target zones for these
> zone management requests.

While they can be issued concurrently you can't expect sane behavior
in that case.  So I'm not sure why we need the zone write lock in this
case.

> +++ b/drivers/scsi/sd.c
> @@ -1215,6 +1215,12 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
>  	else
>  		protect = 0;
>  
> +	if (req_op(rq) == REQ_OP_ZONE_APPEND) {
> +		ret = sd_zbc_prepare_zone_append(cmd, &lba, nr_blocks);
> +		if (ret)
> +			return ret;
> +	}

I'd move this up a few lines to keep all the PI related code together.

> +#define SD_ZBC_INVALID_WP_OFST	~(0u)
> +#define SD_ZBC_UPDATING_WP_OFST	(SD_ZBC_INVALID_WP_OFST - 1)

Given that this goes into the seq_zones_wp_ofst shouldn't the block
layer define these values?

> +struct sd_zbc_zone_work {
> +	struct work_struct work;
> +	struct scsi_disk *sdkp;
> +	unsigned int zno;
> +	char buf[SD_BUF_SIZE];
> +};

Wouldn't it make sense to have one work_struct per scsi device and batch
updates?  That is also query a decenent sized buffer with a bunch of
zones and update them all at once?  Also given that the other write
pointer caching code is in the block layer, why is this in SCSI?

> +	spin_lock_bh(&sdkp->zone_wp_ofst_lock);
> +
> +	wp_ofst = rq->q->seq_zones_wp_ofst[zno];
> +
> +	if (wp_ofst == SD_ZBC_UPDATING_WP_OFST) {
> +		/* Write pointer offset update in progress: ask for a requeue */
> +		ret = BLK_STS_RESOURCE;
> +		goto err;
> +	}
> +
> +	if (wp_ofst == SD_ZBC_INVALID_WP_OFST) {
> +		/* Invalid write pointer offset: trigger an update from disk */
> +		ret = sd_zbc_update_wp_ofst(sdkp, zno);
> +		goto err;
> +	}
> +
> +	wp_ofst = sectors_to_logical(sdkp->device, wp_ofst);
> +	if (wp_ofst + nr_blocks > sdkp->zone_blocks) {
> +		ret = BLK_STS_IOERR;
> +		goto err;
> +	}
> +
> +	/* Set the LBA for the write command used to emulate zone append */
> +	*lba += wp_ofst;
> +
> +	spin_unlock_bh(&sdkp->zone_wp_ofst_lock);

This seems like a really good use case for cmpxchg.  But I guess
premature optimization is the root of all evil, so let's keep this in
mind for later.

> +	/*
> +	 * For zone append, the zone was locked in sd_zbc_prepare_zone_append().
> +	 * For zone reset and zone finish, the zone was locked in
> +	 * sd_zbc_setup_zone_mgmt_cmnd().
> +	 * For regular writes, the zone is unlocked by the block layer elevator.
> +	 */
> +	return req_op(rq) == REQ_OP_ZONE_APPEND ||
> +		req_op(rq) == REQ_OP_ZONE_RESET ||
> +		req_op(rq) == REQ_OP_ZONE_FINISH;
> +}
> +
> +static bool sd_zbc_need_zone_wp_update(struct request *rq)
> +{
> +	if (req_op(rq) == REQ_OP_WRITE ||
> +	    req_op(rq) == REQ_OP_WRITE_ZEROES ||
> +	    req_op(rq) == REQ_OP_WRITE_SAME)
> +		return blk_rq_zone_is_seq(rq);
> +
> +	if (req_op(rq) == REQ_OP_ZONE_RESET_ALL)
> +		return true;
> +
> +	return sd_zbc_zone_needs_write_unlock(rq);

To me all this would look cleaner with a switch statement:

static bool sd_zbc_need_zone_wp_update(struct request *rq)

	switch (req_op(rq)) {
	case REQ_OP_ZONE_APPEND:
	case REQ_OP_ZONE_FINISH:
	case REQ_OP_ZONE_RESET:
	case REQ_OP_ZONE_RESET_ALL:
		return true;
	case REQ_OP_WRITE:
	case REQ_OP_WRITE_ZEROES:
	case REQ_OP_WRITE_SAME:
		return blk_rq_zone_is_seq(rq);
	default:
		return false;
	}
}

> +	if (!sd_zbc_need_zone_wp_update(rq))
> +		goto unlock_zone;

Split the wp update into a little helper?

> +void sd_zbc_init_disk(struct scsi_disk *sdkp)
> +{
> +	if (!sd_is_zoned(sdkp))
> +		return;
> +
> +	spin_lock_init(&sdkp->zone_wp_ofst_lock);

Shouldn't this lock also go into the block code where the cached
write pointer lives?