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

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Feb 27, 2026 / 13:53, Damien Le Moal wrote:
> 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 ?

As to the failure of the test case 58, which has one write job and one trim job,
I think your idea will work. However, I still think there is a tiny possibility
that zbd_write_zone_get() returns false due to the max_open_zone limit and what
other jobs do. As I describe below, the zone can be removed from the write
target array by the trim job. And if another write job exists and if it open
another zone in parallel, zbd_write_zone_get() may hit max_open_zones limit and
return false.

> 
> 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 ?

As to the test case 58, the condition is that there is a trim workload running
in parallel to a write workload. The trim workload can choose the zone in the
write target array, and reset the zone. After the reset, the trim worload
removes the zone from the array, then call zone_unlock(). The zone reset by the
trim zone can happen just before the zone_lock() in the hunk above. I think
write workloads with the zone_reset_threshold option can cause the same failure.

> 
> >  		}
> >  		pthread_mutex_lock(&zbdi->mutex);
> >  	}
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research