Re: [PATCH v9 06/13] zloop: keep zone write pointer and condition consistent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] zloop: static ret in zloop_set_zone_append_sector() causes
  sticky errors
- [High] zloop: redundant zloop_do_open_zone() in zloop_seq_write_prep()
  fails I/O
- [Medium] zloop: race on zone->cond between
  zloop_set_zone_append_sector() and open
--

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

zloop: keep zone write pointer and condition consistent

This commit calls zloop_do_open_zone() in zloop_set_zone_append_sector()
and changes its return type to blk_status_t so ordered zone append commands
implicitly open the target zone when the write pointer is updated.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index 65634a39b0bfc..7aba65d43b250 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c
> @@ -885,20 +885,26 @@ static void zloop_complete_rq(struct request *rq)
>  	blk_mq_end_request(rq, sts);
>  }
>  
> -static bool zloop_set_zone_append_sector(struct request *rq)
> +static blk_status_t zloop_set_zone_append_sector(struct request *rq)
>  {
>  	struct zloop_device *zlo = rq->q->queuedata;
>  	unsigned int zone_no = rq_zone_no(rq);
>  	struct zloop_zone *zone = &zlo->zones[zone_no];
>  	sector_t zone_end = zone->start + zlo->zone_capacity;
>  	sector_t nr_sectors = blk_rq_sectors(rq);
> +	static blk_status_t ret = BLK_STS_OK;

[Severity: High]
Should ret be a non-static local variable?

Because ret is declared static and is never reset to BLK_STS_OK on the
success path, once any call fails, every subsequent ordered zone append
opens the target zone, advances zone->wp, and still returns the stale error
to zloop_queue_rq(), aborting the request and leaving zone->wp out of sync
with the backing file.  Concurrent callers on different zones also race on
reading and writing ret.

A later patch in the series, commit "zloop: add storage element emulation",
refactors zloop_set_zone_append_sector() and removes this static variable,
but this commit carries a Fixes: tag and introduces the regression for
bisection and backports.

>  
>  	spin_lock(&zone->wp_lock);
>  
>  	if (zone->cond == BLK_ZONE_COND_FULL ||
>  	    zone->wp + nr_sectors > zone_end) {
> -		spin_unlock(&zone->wp_lock);
> -		return false;
> +		ret = BLK_STS_IOERR;
> +		goto unlock;
> +	}
> +
> +	if (!zloop_do_open_zone(zlo, zone, false)) {
> +		ret = BLK_STS_ZONE_OPEN_RESOURCE;
> +		goto unlock;
>  	}

[Severity: Medium]
Can calling zloop_do_open_zone() under only zone->wp_lock race with
zloop_open_zone() on the same zone?

zloop_open_zone() calls zloop_do_open_zone() while holding only zone->lock,
without taking zone->wp_lock:

drivers/block/zloop.c:zloop_open_zone() {
	...
	mutex_lock(&zone->lock);
	...
	if (!zloop_do_open_zone(zlo, zone, true))
		ret = -EIO;
	...
}

When a zone is in BLK_ZONE_COND_EMPTY or BLK_ZONE_COND_CLOSED, an ordered
zone append and an explicit zone open can enter zloop_do_open_zone()
concurrently:

CPU0                                  CPU1
zloop_queue_rq()                      zloop_open_zone()
  zloop_set_zone_append_sector()        mutex_lock(&zone->lock)
    spin_lock(&zone->wp_lock)           zloop_do_open_zone()
    zloop_do_open_zone()                  switch (zone->cond)
      switch (zone->cond)                 zloop_open_closed_or_empty_zone()
      zloop_open_closed_or_empty_zone()     spin_lock(&zlo->open_zones_lock)
                                            zlo->nr_open_zones++;
                                            list_add_tail(&zone->open_zone_entry, ...)
                                            spin_unlock(&zlo->open_zones_lock)
        spin_lock(&zlo->open_zones_lock)
        zlo->nr_open_zones++;
        list_add_tail(&zone->open_zone_entry, ...)  <-- list corruption

Both paths add zone->open_zone_entry to zlo->open_zones_lru_list and
increment zlo->nr_open_zones, corrupting the LRU list and open zone
accounting.

>  
>  	rq->__sector = zone->wp;
> @@ -906,9 +912,10 @@ static bool zloop_set_zone_append_sector(struct request *rq)
>  	if (zone->wp >= zone_end)
>  		zloop_mark_full(zlo, zone);

[Severity: High]
Does calling zloop_do_open_zone() again in zloop_seq_write_prep() fail
ordered zone appends that fill a zone?

When an ordered zone append reaches zone_end here, zloop_mark_full() sets
zone->cond to BLK_ZONE_COND_FULL before the command work item runs.  When
the work item later executes zloop_rw() -> zloop_seq_write_prep(), it
skips the capacity check and write pointer update for ordered_zone_append,
but still calls zloop_do_open_zone() unconditionally:

drivers/block/zloop.c:zloop_seq_write_prep() {
	...
	/* Implicitly open the target zone. */
	if (!zloop_do_open_zone(zlo, zone, false)) {
		ret = -EIO;
		goto out_unlock;
	}

	if (!is_append || !zlo->ordered_zone_append) {
		zone->wp += nr_sectors;
		if (zone->wp == zone_end)
			zloop_mark_full(zlo, zone);
	}
	...
}

Because zloop_do_open_zone() returns false when zone->cond is
BLK_ZONE_COND_FULL, the zone-filling append (and any earlier in-flight
ordered append to that zone whose work item runs after zloop_mark_full())
fails with -EIO and sets ZLOOP_ZONE_SEQ_ERROR.  Skipping
zloop_do_open_zone() in zloop_seq_write_prep() when is_append and
zlo->ordered_zone_append are set would avoid reopening a zone that was
already opened and possibly marked full at submission time.

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