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
