| Message ID | 20221130094121.2321485-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: support readonly and offline zone conditions | expand |
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, 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?
On 11/30/22 20:15, Shinichiro Kawasaki wrote: [...] >>> + 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? Indeed, it looks like it. You are correct. So no read-only or offline for conventional zones it is.
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 1f154f92f4c2..7d28e3aa406c 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -523,6 +523,24 @@ static ssize_t nullb_device_badblocks_store(struct config_item *item, } CONFIGFS_ATTR(nullb_device_, badblocks); +static ssize_t nullb_device_zone_readonly_store(struct config_item *item, + const char *page, size_t count) +{ + struct nullb_device *dev = to_nullb_device(item); + + return zone_cond_store(dev, page, count, BLK_ZONE_COND_READONLY); +} +CONFIGFS_ATTR_WO(nullb_device_, zone_readonly); + +static ssize_t nullb_device_zone_offline_store(struct config_item *item, + const char *page, size_t count) +{ + struct nullb_device *dev = to_nullb_device(item); + + return zone_cond_store(dev, page, count, BLK_ZONE_COND_OFFLINE); +} +CONFIGFS_ATTR_WO(nullb_device_, zone_offline); + static struct configfs_attribute *nullb_device_attrs[] = { &nullb_device_attr_size, &nullb_device_attr_completion_nsec, @@ -549,6 +567,8 @@ static struct configfs_attribute *nullb_device_attrs[] = { &nullb_device_attr_zone_nr_conv, &nullb_device_attr_zone_max_open, &nullb_device_attr_zone_max_active, + &nullb_device_attr_zone_readonly, + &nullb_device_attr_zone_offline, &nullb_device_attr_virt_boundary, &nullb_device_attr_no_sched, &nullb_device_attr_shared_tag_bitmap, @@ -614,7 +634,7 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page) "poll_queues,power,queue_mode,shared_tag_bitmap,size," "submit_queues,use_per_node_hctx,virt_boundary,zoned," "zone_capacity,zone_max_active,zone_max_open," - "zone_nr_conv,zone_size\n"); + "zone_nr_conv,zone_offline,zone_readonly,zone_size\n"); } CONFIGFS_ATTR_RO(memb_group_, features); diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h index 94ff68052b1e..eb5972c50be8 100644 --- a/drivers/block/null_blk/null_blk.h +++ b/drivers/block/null_blk/null_blk.h @@ -151,6 +151,8 @@ blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_op op, sector_t sector, sector_t nr_sectors); size_t null_zone_valid_read_len(struct nullb *nullb, sector_t sector, unsigned int len); +ssize_t zone_cond_store(struct nullb_device *dev, const char *page, + size_t count, enum blk_zone_cond cond); #else static inline int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) @@ -174,6 +176,12 @@ static inline size_t null_zone_valid_read_len(struct nullb *nullb, { return len; } +static inline ssize_t zone_cond_store(struct nullb_device *dev, + const char *page, size_t count, + enum blk_zone_cond cond) +{ + return -EOPNOTSUPP; +} #define null_report_zones NULL #endif /* CONFIG_BLK_DEV_ZONED */ #endif /* __NULL_BLK_H */ diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c index 55a69e48ef8b..fac68e797803 100644 --- a/drivers/block/null_blk/zoned.c +++ b/drivers/block/null_blk/zoned.c @@ -384,8 +384,10 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, null_lock_zone(dev, zone); - if (zone->cond == BLK_ZONE_COND_FULL) { - /* Cannot write to a full zone */ + if (zone->cond == BLK_ZONE_COND_FULL || + zone->cond == BLK_ZONE_COND_READONLY || + zone->cond == BLK_ZONE_COND_OFFLINE) { + /* Cannot write to the zone */ ret = BLK_STS_IOERR; goto unlock; } @@ -613,7 +615,9 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op, for (i = dev->zone_nr_conv; i < dev->nr_zones; i++) { zone = &dev->zones[i]; null_lock_zone(dev, zone); - if (zone->cond != BLK_ZONE_COND_EMPTY) { + if (zone->cond != BLK_ZONE_COND_EMPTY && + zone->cond != BLK_ZONE_COND_READONLY && + zone->cond != BLK_ZONE_COND_OFFLINE) { null_reset_zone(dev, zone); trace_nullb_zone_op(cmd, i, zone->cond); } @@ -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)]; - + 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. + */ +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 + * condition, reset the zones to restore back normal empty condition. + * Do same if offline condition is requested for offline zones. + * Otherwise, set desired zone condition to the zones. Finish the zones + * beforehand to free up zone resources. + */ + if (old_cond == cond) { + ret = null_reset_zone(dev, zone); + } else { + ret = null_finish_zone(dev, zone); + if (!ret) { + zone->cond = cond; + zone->wp = (sector_t)-1; + } + } + + if (ret) + zone->cond = old_cond; + + 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); + + 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"); + return -EINVAL; + } + + ret = null_set_zone_cond(dev, &dev->zones[zone_no], cond); + if (ret) + return ret; + + return count; +}
In zoned mode, zones with write pointers can have conditions "readonly" or "offline". In readonly condition, zones can not be written. In offline condition, the zones can be neither written nor read. These conditions are intended for zones with media failures, then it is difficult to set those conditions to zones on real devices. To test handling of zones in the conditions, add a feature to null_blk to set up zones in readonly or offline condition. Add new configuration attributes "zone_readonly" and "zone_offline". Write a sector to the attribute files to specify the target zone to set the zone conditions. For example, following command lines do it: echo 0 > nullb1/zone_readonly echo 524288 > nullb1/zone_offline When the specified zones are already in readonly or offline condition, normal empty condition is restored to the zones. These condition changes can be done only after the null_blk device get powered, since status area of each zone is not yet allocated before power-on. Also improve zone condition checks to inhibit all commands for zones in offline conditions. In same manner, inhibit write and zone management commands for zones in readonly condition. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- This patch conflicts with another null_blk patch series posted recently [1]. Will rework to avoid the conflict on v2 or later. [1] https://lore.kernel.org/linux-block/20221129232813.37968-1-kch@nvidia.com/ drivers/block/null_blk/main.c | 22 +++++- drivers/block/null_blk/null_blk.h | 8 +++ drivers/block/null_blk/zoned.c | 112 ++++++++++++++++++++++++++++-- 3 files changed, 137 insertions(+), 5 deletions(-)