Re: [PATCH v8 07/12] zloop: add storage element emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] zloop: zloop_update_seq_zone() fails zone reports and reads
  when !zloop_zone_is_ok()
- [Medium] zloop: inconsistent zone conditions and element accounting in
  restore and remove paths
- [Medium] zloop: missing deletion check after msleep() in element
  remove and restore
- [Low] zloop: 1-zone device with multiple storage elements in
  zloop_ctl_add()
--

commit 20458ce8408d80d52134d64889b748a6d50aa2b4
Author: Damien Le Moal <dlemoal@kernel.org>

zloop: add storage element emulation

This commit adds emulation of SMR storage element depopulation and
restoration to the zloop driver via the stor_elements option and
blk_se_ops callbacks.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index e1ad7513fea9b..164f21f15158f 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c

[ ... ]

> @@ -289,22 +315,64 @@ static bool zloop_do_open_zone(struct zloop_device *zlo,
>  	}
>  }
>  

[ ... ]

> +static bool zloop_zone_is_ok(struct zloop_device *zlo, struct zloop_zone *zone)
> +{
> +	bool ret;
> +
> +	spin_lock(&zone->wp_lock);
> +
> +	ret = zloop_zone_element_is_ok(zlo, zone, true);
> +	if (ret)
> +		ret = zone->cond != BLK_ZONE_COND_OFFLINE &&
> +			zone->cond != BLK_ZONE_COND_READONLY;
> +
> +	spin_unlock(&zone->wp_lock);
> +
> +	return ret;
>  }
>  
>  static int zloop_update_seq_zone(struct zloop_device *zlo, unsigned int zone_no)
> @@ -316,6 +384,15 @@ static int zloop_update_seq_zone(struct zloop_device *zlo, unsigned int zone_no)
>  
>  	lockdep_assert_held(&zone->lock);
>  
> +	/*
> +	 * If the zone is served by an element that is not in a good state, fail
> +	 * the operation.
> +	 */
> +	if (!zloop_zone_is_ok(zlo, zone)) {
> +		set_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags);
> +		return -EIO;
> +	}
> +
>  	ret = vfs_getattr(&zone->file->f_path, &stat, STATX_SIZE, 0);

[Severity: Medium]
Can checking zloop_zone_is_ok() in zloop_update_seq_zone() permanently
break zone reporting and reject valid reads on read-only zones?

If vfs_truncate() fails in zloop_restore_zone(), ZLOOP_ZONE_SEQ_ERROR is
set while the zone stays in BLK_ZONE_COND_OFFLINE or
BLK_ZONE_COND_READONLY and the element enters BLK_SE_STS_RESTORE_ERROR.
When bdev_restore_storage_elements() or BLKREPORTZONE then calls
zloop_report_zones(), test_and_clear_bit(ZLOOP_ZONE_SEQ_ERROR, ...)
triggers zloop_update_seq_zone(), which sees !zloop_zone_is_ok(), re-sets
ZLOOP_ZONE_SEQ_ERROR, and returns -EIO on every subsequent zone report:

drivers/block/zloop.c:zloop_report_zones() {
    ...
		if (test_and_clear_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags)) {
			ret = zloop_update_seq_zone(zlo, zone_no);
			if (ret) {
				mutex_unlock(&zone->lock);
				return ret;
			}
		}
    ...
}

In addition, because zloop_zone_is_ok() checks the write element
(for_write = true) and rejects BLK_ZONE_COND_READONLY, a REQ_OP_READ to a
zone with ZLOOP_ZONE_SEQ_ERROR set whose write element is removed or
being removed in ZLOOP_STOR_ELEMENTS_PAIRS mode passes zloop_prep_rq()
(which checks the read element), but then fails with -EIO in
zloop_rw() -> zloop_update_seq_zone().

[ ... ]

> @@ -1012,6 +1167,331 @@ static int zloop_report_zones(struct gendisk *disk, sector_t sector,
>  	return nr_zones;
>  }
>  

[ ... ]

> +static int zloop_execute_remove_element(struct zloop_device *zlo)
> +{
> +	struct gendisk *disk = zlo->disk;
> +	struct blk_se *se, *paired_se = NULL;
> +	enum blk_zone_cond cond;
> +	struct zloop_zone *zone;
> +	unsigned int memflags, i;
> +	int ret = 0;
> +
> +	/* Wait for all IOs to complete. */
> +	memflags = blk_mq_freeze_queue(disk->queue);
> +	blk_mq_unfreeze_queue(disk->queue, memflags);
> +
> +	/*
> +	 * If the element to remove is a read element, zones must go offline and
> +	 * the associated write element is also removed.
> +	 */
> +	se = &zlo->elements[zlo->remove_element_id - 1];
> +	switch (se->type) {
> +	case BLK_SE_TYPE_RDWR:
> +		cond = BLK_ZONE_COND_OFFLINE;
> +		break;
> +	case BLK_SE_TYPE_READ:
> +		cond = BLK_ZONE_COND_OFFLINE;
> +		paired_se = &zlo->elements[se->paired_id - 1];
> +		break;
> +	case BLK_SE_TYPE_WRITE:
> +		cond = BLK_ZONE_COND_READONLY;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Change the condition of the zones owned by the (pair of) elements
> +	 * being removed and mark the elements removed.
> +	 */
> +	for (i = 0, zone = zlo->zones; i < zlo->nr_zones; i++, zone++) {
> +		if (zone->wr_se_id != se->id && zone->rd_se_id != se->id)
> +			continue;
> +		mutex_lock(&zone->lock);
> +		spin_lock(&zone->wp_lock);
> +		zloop_set_zone_cond(zlo, zone, cond);
> +		clear_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags);
> +		spin_unlock(&zone->wp_lock);
> +		mutex_unlock(&zone->lock);
> +	}
> +
> +	WRITE_ONCE(se->status, BLK_SE_STS_REMOVED);
> +	zlo->nr_removed_elements++;
> +	if (paired_se && READ_ONCE(paired_se->status) != BLK_SE_STS_REMOVED) {
> +		WRITE_ONCE(paired_se->status, BLK_SE_STS_REMOVED);
> +		zlo->nr_removed_elements++;
> +	}
> +
> +	return ret;
> +}
> +
> +static int zloop_remove_element(struct gendisk *disk, unsigned int element_id)
> +{
> +	struct zloop_device *zlo = disk->private_data;
> +	struct blk_se *se, *paired_se = NULL;
> +	unsigned int nr_remove = 1;
> +	int ret = 0;
> +
> +	if (test_bit(GD_DEAD, &disk->state) ||
> +	    data_race(READ_ONCE(zlo->state)) == Zlo_deleting)
> +		return -ENODEV;
> +
> +	if (zlo->stor_elements == ZLOOP_STOR_ELEMENTS_NONE)
> +		return -EOPNOTSUPP;
> +
> +	if (!element_id || element_id > zlo->nr_elements)
> +		return -EINVAL;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* If a remove or restore operation is already in progress, bail out. */
> +	if (zlo->remove_element_id || READ_ONCE(zlo->restore_in_progress)) {
> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	/*
> +	 * Get the element to remove. If it is already removed, we have nothing
> +	 * to do.
> +	 */
> +	se = &zlo->elements[element_id - 1];
> +	if (READ_ONCE(se->status) == BLK_SE_STS_REMOVED)
> +		goto unlock;
> +
> +	/*
> +	 * If the element to remove is a read element, the associated write
> +	 * element must also be removed.
> +	 */
> +	if (se->type == BLK_SE_TYPE_READ) {
> +		paired_se = &zlo->elements[se->paired_id - 1];
> +		if (READ_ONCE(paired_se->status) != BLK_SE_STS_REMOVED)
> +			nr_remove = 2;
> +	}
> +
> +	/*
> +	 * Make sure we do not allow removing all storage elements as that does
> +	 * not make any sense. This is consistent with the device advertized
> +	 * limit of SCSI and ATA devices supporting the storage element
> +	 * depopulation feature.
> +	 */
> +	if (zlo->nr_removed_elements + nr_remove >= zlo->nr_elements) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	/*
> +	 * Change the elements to be removed to the remove in progress state so
> +	 * that concurrent report elements can see the operation on-going.
> +	 */
> +	WRITE_ONCE(se->status, BLK_SE_STS_REMOVE_IN_PROGRESS);
> +	if (paired_se && READ_ONCE(paired_se->status) != BLK_SE_STS_REMOVED)
> +		WRITE_ONCE(paired_se->status, BLK_SE_STS_REMOVE_IN_PROGRESS);
> +	zlo->remove_element_id = element_id;
> +
> +	mutex_unlock(&zlo->stor_elements_lock);
> +
> +	/*
> +	 * Wait a little so that this looks like a real device taking time to
> +	 * operate.
> +	 */
> +	msleep(2000);
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* Remove the element(s). */
> +	ret = zloop_execute_remove_element(zlo);
> +	zlo->remove_element_id = 0;
> +
> +unlock:
> +	mutex_unlock(&zlo->stor_elements_lock);
> +
> +	return ret;
> +}
> +
> +static int zloop_restore_zone(struct zloop_device *zlo,
> +			      struct zloop_zone *zone)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&zone->lock);
> +
> +	if (!test_bit(ZLOOP_ZONE_CONV, &zone->flags)) {
> +		ret = vfs_truncate(&zone->file->f_path, 0);
> +		if (ret) {
> +			set_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags);
> +			ret = -EIO;
> +			goto unlock;
> +		}
> +	}
> +
> +	spin_lock(&zone->wp_lock);
> +	if (test_bit(ZLOOP_ZONE_CONV, &zone->flags)) {
> +		zloop_set_zone_cond(zlo, zone, BLK_ZONE_COND_NOT_WP);
> +	} else {
> +		zloop_set_zone_empty(zlo, zone);
> +		clear_bit(ZLOOP_ZONE_SEQ_ERROR, &zone->flags);
> +	}
> +	spin_unlock(&zone->wp_lock);
> +
> +unlock:
> +	mutex_unlock(&zone->lock);
> +
> +	return ret;
> +}
> +
> +static int zloop_execute_restore_elements(struct zloop_device *zlo)
> +{
> +	struct gendisk *disk = zlo->disk;
> +	struct zloop_zone *zone;
> +	struct blk_se *se;
> +	enum blk_se_status sts = BLK_SE_STS_OK;
> +	unsigned int memflags, i;
> +	int ret = 0;
> +
> +	/* Wait for all IOs to complete. */
> +	memflags = blk_mq_freeze_queue(disk->queue);
> +	blk_mq_unfreeze_queue(disk->queue, memflags);
> +
> +	/*
> +	 * Truncate all files for sequential zones and restore the condition of
> +	 * all zones. If this fails, we will propagate the failure as a
> +	 * restoration error.
> +	 */
> +	for (i = 0, zone = zlo->zones; i < zlo->nr_zones; i++, zone++) {
> +		ret = zloop_restore_zone(zlo, zone);
> +		if (ret) {
> +			pr_err("zloop%d: restore zone %u failed %d\n",
> +			       zlo->id, i, ret);
> +			sts = BLK_SE_STS_RESTORE_ERROR;
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * Update the status of all elements being restored based on the zone
> +	 * restoration result.
> +	 */
> +	for (i = 0, se = zlo->elements; i < zlo->nr_elements; i++, se++) {
> +		if (READ_ONCE(se->status) == BLK_SE_STS_RESTORE_IN_PROGRESS)
> +			WRITE_ONCE(se->status, sts);
> +	}
> +
> +	/*
> +	 * Clear the counter of removed elements so that even if we had an error
> +	 * and set the elements health to BLK_SE_STS_RESTORE_ERROR, then can be
> +	 * removed again.
> +	 */
> +	zlo->nr_removed_elements = 0;
> +
> +	return ret;
> +}

[Severity: Medium]
Can zloop_execute_restore_elements() and zloop_remove_element() leave
zone conditions out of sync with element states or allow depopulating all
remaining usable elements?

If zloop_restore_zone() fails on sequential zone i in
zloop_execute_restore_elements(), the loop breaks and sets all restoring
elements to BLK_SE_STS_RESTORE_ERROR.  However, zones before i (including
conventional zones) belonging to those failed elements were already set
to BLK_ZONE_COND_NOT_WP or BLK_ZONE_COND_EMPTY, so they report as online
and writable even though zloop_prep_rq() rejects I/O to them.

Resetting zlo->nr_removed_elements to 0 on restore error also lets
zloop_remove_element() pass the zlo->nr_removed_elements + nr_remove >=
zlo->nr_elements check while those elements are still in
BLK_SE_STS_RESTORE_ERROR, allowing the caller to remove all remaining
BLK_SE_STS_OK elements.

In ZLOOP_STOR_ELEMENTS_PAIRS mode, removing a BLK_SE_TYPE_WRITE element
whose paired read element is in BLK_SE_STS_RESTORE_ERROR unconditionally
sets the zones to BLK_ZONE_COND_READONLY even though reads cannot be
served.  In addition, comparing zlo->nr_removed_elements + nr_remove
against zlo->nr_elements in paired mode allows removing every
BLK_SE_TYPE_WRITE element on the device, leaving zero writable zones.

> +
> +static int zloop_restore_elements(struct gendisk *disk)
> +{
> +	struct zloop_device *zlo = disk->private_data;
> +	struct blk_se *se;
> +	unsigned int i, n = 0;
> +	int ret = 0;
> +
> +	if (test_bit(GD_DEAD, &disk->state) ||
> +	    data_race(READ_ONCE(zlo->state)) == Zlo_deleting)
> +		return -ENODEV;

[ ... ]

> +	WRITE_ONCE(zlo->restore_in_progress, true);
> +	mutex_unlock(&zlo->stor_elements_lock);
> +
> +	/*
> +	 * Wait a little so that this looks like a real device taking time to
> +	 * operate.
> +	 */
> +	msleep(5000);
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* Restore the elements. */
> +	ret = zloop_execute_restore_elements(zlo);
> +
> +	WRITE_ONCE(zlo->restore_in_progress, false);
> +
> +unlock:
> +	mutex_unlock(&zlo->stor_elements_lock);
> +
> +	return ret;
> +}

[Severity: Medium]
What happens if the zloop device is removed via /dev/zloop-control while
zloop_remove_element() or zloop_restore_elements() is sleeping in
msleep()?

CPU0                              CPU1
zloop_restore_elements()
  /* checks GD_DEAD / Zlo_deleting */
  mutex_unlock(&zlo->stor_elements_lock)
  msleep(5000)
                                  zloop_ctl_remove()
                                    WRITE_ONCE(zlo->state, Zlo_deleting)
                                    del_gendisk(zlo->disk)
                                    zloop_forget_cache(zlo)
  mutex_lock(&zlo->stor_elements_lock)
  zloop_execute_restore_elements()
    blk_mq_freeze_queue(disk->queue)
    zloop_restore_zone()
      vfs_truncate(..., 0)        <-- truncates files on deleted device

Neither zloop_remove_element() nor zloop_restore_elements() re-checks
GD_DEAD or Zlo_deleting after reacquiring zlo->stor_elements_lock.  If
zloop_ctl_remove() runs during the sleep, the waking thread freezes a
queue already torn down by del_gendisk() and truncates all sequential
zone backing files to 0, destroying persisted zone data or corrupting a
newly re-added device reusing the same ID.

[ ... ]

> @@ -1188,6 +1695,69 @@ static int zloop_init_zone(struct zloop_device *zlo, struct zloop_options *opts,
>  	return ret;
>  }
>  
> +#define ZLOOP_MIN_STOR_ELEMENTS			2
> +#define ZLOOP_MAX_STOR_ELEMENTS			32
> +#define ZLOOP_MIN_ZONES_PER_STOR_ELEMENTS	32
> +
> +static int zloop_create_storage_elements(struct zloop_device *zlo)
> +{
> +	struct blk_se *se, *paired_se;
> +	unsigned int i, nr_elems, nr_elements;
> +
> +	/*
> +	 * Calculate the number of storage elements we are going to emulate.
> +	 * To achieve a somewhat realistic emulation, we want at least 2 storage
> +	 * elements, and no more than 32, targeting at least 32 zones per
> +	 * element.
> +	 */
> +	if (zlo->nr_zones <= 64)
> +		nr_elements = ZLOOP_MIN_STOR_ELEMENTS;
> +	else
> +		nr_elements =
> +			min(ZLOOP_MAX_STOR_ELEMENTS,
> +			    zlo->nr_zones / ZLOOP_MIN_ZONES_PER_STOR_ELEMENTS);

[Severity: Low]
Can zloop_create_storage_elements() create storage elements with
nr_zones == 0 when a device has only 1 zone?

In zloop_parse_options(), opts->capacity <= opts->zone_size passes when
capacity_mb is strictly between zone_size_mb and 2 * zone_size_mb (for
example capacity_mb=257, zone_size_mb=256), and in zloop_ctl_add(),
nr_zones = opts->capacity >> ilog2(opts->zone_size) truncates to 1, which
passes opts->nr_conv_zones >= nr_zones when conv_zones=0.
zloop_create_storage_elements() then creates 2 elements (or 2 pairs),
leaving the second element (or pair) with nr_zones == 0 and allowing
zloop_remove_element() on element 1 to offline the only zone on the
device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109095144.819709-1-dlemoal@kernel.org?part=7