Damien, thanks for the review. I will reflect most of your comments. Please find
my responses in line.
On Nov 30, 2022 / 19:18, Damien Le Moal wrote:
> 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;
I see, manual zone status change to empty status could be straight forward.
Looking in null_reset_zone(), I think additional null_handle_discard() call will
be required here, if the device is memory backed.
> }
>
> 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)
I agree that old_cond is not required to restore empty condition manually.
However, still I think we need to set the zone to FULL before calling
null_finish_zone(). Let's say that the target zone is already set read-only,
then it is made offline. In such a case, null_fininsh_zone() fails for the
read-only zone. To allow such condition change, I added the old_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.
As I will comment below, I don't think conventional zones can be read-only or
offline. So conventional zones are handled as an error in zone_cond_store().
>
> > + 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.
As far as I refer ZBC r05 Table 10 and ZAC r05 Table 5, conventional zones can
have only zone condition "NOT WRITE POINTER". I think the tables show that the
zone conditions "READ ONLY" and "OFFLINE" are valid for sequential write
required zones (and sequential write preferred zones). Do I misinterpret the
specs?
--
Shin'ichiro Kawasaki
