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

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

On 2/16/26 16:59, 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. But when the zone is
> locked, another job might have removed the zone from the write target
> zones array. This 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;
> +			}

The way I understand this is: since we do have a remainder, the zone is not
full, so zbd_write_zone_get() cannot return false. So this looks OK, but is also
very confusing. What about removing the if and instead use an assert checking
that zbd_write_zone_get() returns true ?

Also, it is not clear what the conditions are for a zone that is still not full
to be removed from the array. Can you detail that ?

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


-- 
Damien Le Moal
Western Digital Research