Re: [PATCH v3 2/8] zbd: fix write zone accounting

[Damien Le Moal <dlemoal@kernel.org>]

On 3/2/26 11:26 AM, Shin'ichiro Kawasaki wrote:
> Currently, zbd_convert_to_write_zones() calls io_u_quiesce() when the
> number of write target zones hits one of the limits of write zones. This
> wait by io_u_quiesce() significantly degrade the performance. While I
> tried to remove the io_u_quiesce(), I observed that the test case 58 of
> t/zbd/test-zbd-support failed with null_blk devices that have a
> max_active_zones limit set.
> 
> The failure cause is an incorrect write target zone accounting in
> zbd_convert_to_write_zones(). This function checks the current write
> target zones, and selects one of them as the next write target zone.
> After the zone selection, it locks the zone. However, when the zone is
> locked, another job such as a trim workload or a write workload with the
> zone_reset_threshold option might have already reset the zone and
> removed it from the write target zones array. This unexpected zone
> removal from the array caused an incorrect zone accounting and the test
> case failure.
> 
> To avoid the incorrect zone accounting, call zbd_write_zone_get() after
> the selected zone gets locked. If the zone is removed from the write
> target zones array, the function adds the zone back to the array.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  zbd.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index b71f842c..c511b709 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -1693,8 +1693,17 @@ retry:
>  
>  		zone_lock(td, f, z);
>  		if (zbd_zone_remainder(z) >= min_bs) {
> -			need_zone_finish = false;
> -			goto out;
> +			/*
> +			 * The zone might be already removed from
> +			 * zbdi->write_zones[] by other jobs at this moment.
> +			 * Even if the zone has remainder, call
> +			 * zbd_write_zone_get() to ensure that it is in the
> +			 * array.
> +			 */
> +			if (zbd_write_zone_get(td, f, z)) {
> +				need_zone_finish = false;
> +				goto out;
> +			}

Please change this to:

		if (zbd_zone_remainder(z) >= min_bs &&
		    zbd_write_zone_get(td, f, z)) {
			need_zone_finish = false;
			goto out;
		}

And move the comment block above the if. You could also improve the comment to
explain why we look at "zbd_zone_remainder(z) >= min_bs"

With that (and the much better commit message), feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

>  		}
>  		pthread_mutex_lock(&zbdi->mutex);
>  	}


-- 
Damien Le Moal
Western Digital Research