Re: [PATCH v5 07/10] scsi: sd_zbc: emulate ZONE_APPEND commands

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 10/04/2020 09:23, Christoph Hellwig wrote:
>> +	spin_lock_bh(&sdkp->zones_wp_ofst_lock);
>> +
>> +	wp_ofst = sdkp->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;
>> +	}
> 
> Maybe I'm a little too clever for my own sake, but what about something
> like:
> 
> 	spin_lock_bh(&sdkp->zones_wp_ofst_lock);
> 	switch (wp_ofst) {
> 	case SD_ZBC_INVALID_WP_OFST:
> 		if (scsi_device_get(sdkp->device)) {
> 			ret = BLK_STS_IOERR;
> 			break;
> 		}
> 		sdkp->zones_wp_ofst[zno] = SD_ZBC_UPDATING_WP_OFST;
> 		schedule_work(&sdkp->zone_wp_ofst_work);
> 		/*FALLTHRU*/
> 	case SD_ZBC_UPDATING_WP_OFST:
> 		ret = BLK_STS_DEV_RESOURCE;
> 		break;
> 	default:
> 		wp_ofst = sectors_to_logical(sdkp->device, wp_ofst);
> 		if (wp_ofst + nr_blocks > sdkp->zone_blocks) {
> 			ret = BLK_STS_IOERR;
> 			break;
> 		}
> 
> 		*lba += wp_ofst;
> 	}
> 	spin_unlock_bh(&sdkp->zones_wp_ofst_lock);
> 	if (ret)
> 		blk_req_zone_write_unlock(rq);
> 	return ret;
> }

This indeed looks cleaner, I'll throw it into testing.

> 
>>   	int result = cmd->result;
>> @@ -294,7 +543,18 @@ void sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
>>   		 * so be quiet about the error.
>>   		 */
>>   		rq->rq_flags |= RQF_QUIET;
>> +		goto unlock_zone;
>>   	}
>> +
>> +	if (sd_zbc_need_zone_wp_update(rq))
>> +		good_bytes = sd_zbc_zone_wp_update(cmd, good_bytes);
>> +
>> +
>> +unlock_zone:
> 
> why not use a good old "else if" here?
> 

Done