On 11/30/22 18:41, Shin'ichiro Kawasaki wrote:
[...]
> @@ -627,6 +631,12 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,
>
> null_lock_zone(dev, zone);
>
> + if (zone->cond == BLK_ZONE_COND_READONLY ||
> + zone->cond == BLK_ZONE_COND_OFFLINE) {
> + ret = BLK_STS_IOERR;
> + goto unlock;
> + }
> +
> switch (op) {
> case REQ_OP_ZONE_RESET:
> ret = null_reset_zone(dev, zone);
> @@ -648,6 +658,7 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,
> if (ret == BLK_STS_OK)
> trace_nullb_zone_op(cmd, zone_no, zone->cond);
>
> +unlock:
> null_unlock_zone(dev, zone);
>
> return ret;
> @@ -674,10 +685,103 @@ blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_op op,
> default:
> dev = cmd->nq->dev;
> zone = &dev->zones[null_zone_no(dev, sector)];
> -
I would prefer keeping this blank line after the "return BLK_STS_IOERR" below.
> + if (zone->cond == BLK_ZONE_COND_OFFLINE)
> + return BLK_STS_IOERR;
> null_lock_zone(dev, zone);
> sts = null_process_cmd(cmd, op, sector, nr_sectors);
> null_unlock_zone(dev, zone);
> return sts;
> }
> }
> +
> +/*
> + * Set specified condition COND_READONLY or COND_OFFLINE to a zone.
May be "Set a zone in the read-only or offline condition."
> + */
> +static int null_set_zone_cond(struct nullb_device *dev, struct nullb_zone *zone,
> + enum blk_zone_cond cond)
> +{
> + enum blk_zone_cond old_cond;
> + int ret;
> +
> + if (WARN_ON_ONCE(cond != BLK_ZONE_COND_READONLY &&
> + cond != BLK_ZONE_COND_OFFLINE))
> + return -EINVAL;
> +
> + null_lock_zone(dev, zone);
> +
> + /*
> + * When current zone condition is readonly or offline, handle the zone
> + * as full condition to avoid failure of zone reset or zone finish.
> + */
> + old_cond = zone->cond;
> + if (zone->cond == BLK_ZONE_COND_READONLY ||
> + zone->cond == BLK_ZONE_COND_OFFLINE)
> + zone->cond = BLK_ZONE_COND_FULL;
> +
> + /*
> + * If readonly condition is requested again to zones already in readonly
If the...
> + * condition, reset the zones to restore back normal empty condition.
> + * Do same if offline condition is requested for offline zones.
Do the same if the...
> + * Otherwise, set desired zone condition to the zones. Finish the zones
Otherwise, set the specified zone condition. Finish...
> + * beforehand to free up zone resources.
> + */
> + if (old_cond == cond) {
> + ret = null_reset_zone(dev, zone);
This will not restore conventional zones since reset will be an error for
these... So you need to do the reset "manually":
You could simply do:
/* Restore the zone to a usable condition */
if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) {
zone->cond = BLK_ZONE_COND_NOT_WP;
zone->wp = (sector_t)-1;
} else {
zone->cond = BLK_ZONE_COND_EMPTY;
zone->wp = zone->start;
}
Then the hunk above that set the zone to FULL is not needed, and the
variable "old_cond" is also not needed as the "if" above can be:
if (zone->cond == cond)
> + } else {
> + ret = null_finish_zone(dev, zone);
Do the finish only for seq zones. Otherwise, setting a conventional zone
to read only or offline will always fail.
> + if (!ret) {
> + zone->cond = cond;
> + zone->wp = (sector_t)-1;
> + }
> + }
> +
> + if (ret)
> + zone->cond = old_cond;
There should never be any failure with this. So we should not need this.
> +
> + null_unlock_zone(dev, zone);
> + return ret;
> +}
> +
> +/*
> + * Identify a zone from the sector written to configfs file. Then set zone
> + * condition to the zone.
> + */
> +ssize_t zone_cond_store(struct nullb_device *dev, const char *page,
> + size_t count, enum blk_zone_cond cond)
> +{
> + unsigned long long sector;
> + unsigned int zone_no;
> + int ret;
> +
> + if (!dev->zoned) {
> + pr_err("null_blk device is not zoned\n");
> + return -EINVAL;
> + }
> +
> + if (!dev->zones) {
> + pr_err("null_blk device is not yet powered\n");
> + return -EINVAL;
> + }
> +
> + ret = kstrtoull(page, 0, §or);
> + if (ret < 0)
> + return ret;
> +
> + zone_no = null_zone_no(dev, sector);
> +
Remove the blank line here.
> + if (zone_no >= dev->nr_zones) {
> + pr_err("Sector out of range\n");
> + return -EINVAL;
> + }
> +
> + if (dev->zones[zone_no].type == BLK_ZONE_TYPE_CONVENTIONAL) {
> + pr_err("Can not change condition of conventional zones\n");
Why ? Conventional zones can go read-only or offline too. At least on
ZBC/ZAC SMR HDDs, they can. So allowing this to happen with null_blk is
desired too.
> + return -EINVAL;
> + }
> +
> + ret = null_set_zone_cond(dev, &dev->zones[zone_no], cond);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
--
Damien Le Moal
Western Digital Research
