On Tue, Jun 07, 2022 at 06:40:14PM +0900, Damien Le Moal wrote:
> On 6/7/22 18:37, Niklas Cassel wrote:
> > On Tue, Jun 07, 2022 at 06:25:45PM +0900, Damien Le Moal wrote:
> > > On 6/7/22 18:16, Niklas Cassel wrote:
> > > > On Tue, Jun 07, 2022 at 10:49:42AM +0900, Damien Le Moal wrote:
> > > > > When a write command to a sequential write required or sequential write
> > > > > preferred zone result in the zone write pointer reaching the end of the
> > > > > zone, the zone condition must be set to full AND the number of
> > > > > implicitly or explicitly open zones updated to have a correct accounting
> > > > > for zone resources. However, the function zbc_inc_wp() only sets the
> > > > > zone condition to full without updating the open zone counters,
> > > > > resulting in a zone state machine breakage.
> > > > >
> > > > > Factor out the correct code from zbc_finish_zone() to transition a zone
> > > > > to the full condition and introduce the helper zbc_set_zone_full(). Use
> > > > > this helper in zbc_finish_zone() and zbc_inc_wp() to correctly
> > > > > transition zones to the full condition.
> > > > >
> > > > > Fixes: 0d1cf9378bd4 ("scsi: scsi_debug: Add ZBC zone commands")
> > > > > Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/scsi/scsi_debug.c | 27 +++++++++++++++++----------
> > > > > 1 file changed, 17 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > > > > index 1f423f723d06..6c2bb02a42d8 100644
> > > > > --- a/drivers/scsi/scsi_debug.c
> > > > > +++ b/drivers/scsi/scsi_debug.c
> > > > > @@ -2826,6 +2826,19 @@ static void zbc_open_zone(struct sdebug_dev_info *devip,
> > > > > }
> > > > > }
> > > > >
> > > > > +static inline void zbc_set_zone_full(struct sdebug_dev_info *devip,
> > > > > + struct sdeb_zone_state *zsp)
> > > > > +{
> > > > > + enum sdebug_z_cond zc = zsp->z_cond;
> > > > > +
> > > > > + if (zc == ZC2_IMPLICIT_OPEN || zc == ZC3_EXPLICIT_OPEN)
> > > > > + zbc_close_zone(devip, zsp);
> > > > > + if (zsp->z_cond == ZC4_CLOSED)
> > > > > + devip->nr_closed--;
> > > > > + zsp->z_wp = zsp->z_start + zsp->z_size;
> > > > > + zsp->z_cond = ZC5_FULL;
> > > > > +}
> > > > > +
> > > > > static void zbc_inc_wp(struct sdebug_dev_info *devip,
> > > > > unsigned long long lba, unsigned int num)
> > > > > {
> > > > > @@ -2838,7 +2851,7 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
> > > > > if (zsp->z_type == ZBC_ZTYPE_SWR) {
> > > > > zsp->z_wp += num;
> > > > > if (zsp->z_wp >= zend)
> > > > > - zsp->z_cond = ZC5_FULL;
> > > > > + zbc_set_zone_full(devip, zsp);
> > > > > return;
> > > > > }
> > > > >
> > > > > @@ -2857,7 +2870,7 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
> > > > > n = num;
> > > > > }
> > > > > if (zsp->z_wp >= zend)
> > > > > - zsp->z_cond = ZC5_FULL;
> > > > > + zbc_set_zone_full(devip, zsp);
> > > >
> > > > Hello Damien,
> > > >
> > > > In the equivalent function (null_zone_write()) in null_blk,
> > > > we instead do this:
> > > >
> > > > if (zone->wp == zone->start + zone->capacity) {
> > > > null_lock_zone_res(dev);
> > > > if (zone->cond == BLK_ZONE_COND_EXP_OPEN)
> > > > dev->nr_zones_exp_open--;
> > > > else if (zone->cond == BLK_ZONE_COND_IMP_OPEN)
> > > > dev->nr_zones_imp_open--;
> > > > zone->cond = BLK_ZONE_COND_FULL;
> > > > null_unlock_zone_res(dev);
> > > > }
> > > >
> > > > Isn't it more clear to do the same here?
> > > > i.e. set the state to FULL, like before, and simply decrease the
> > > > imp/exp open counters.
> > > >
> > > > zbc_set_zone_full() does some things that are not applicable in
> > > > the write path, specifically this:
> > > > > + if (zc == ZC2_IMPLICIT_OPEN || zc == ZC3_EXPLICIT_OPEN)
> > > > > + zbc_close_zone(devip, zsp);
> > > > > + if (zsp->z_cond == ZC4_CLOSED)
> > > > > + devip->nr_closed--;
> > > >
> > > > e.g. with this new helper, if we are in e.g. IMP OPEN, we will now
> > > > set the zone state first to CLOSED, increase the nr_closed counter,
> > > > decrease the nr_closed counter, and then set the zone state to FULL.
> > >
> > > Yes. I am aware of this. It is indeed a bit inefficient, but this makes for
> > > a simple bug fix by covering all call sites (finish and write). If you look
> > > at zbc_rwp_zone() for zone reset, something similar end up being done, the
> > > closed condition is used as an intermediate one. So that one should be
> > > cleaned up too.
> > >
> > > We should improve this, but I think this should be done in a followup
> > > patch(es) and I prefer to keep this bug fix patch small.
> > > Unless you insist :)
> >
> > I just saw that zbc_rwp_zone() does the same after sending my email.
> >
> > I also saw that zbc_close_zone() does a:
> >
> > if (!zbc_zone_is_seq(zsp))
> > return;
> >
> > (Although I don't see a similar check in zbc_finish_zone())
> >
> > So one has to ensure that both SWR and SWP are still handled correctly
> > when doing this cleanup, so considering that this fix solves the problem,
> > it is probably better to leave the cleanup to remove the extra (and at
> > least in my opinion, confusing) state transition in a follow up series.
> >
> > Therefore:
> > Reviewed-by: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> Thanks. Could do a fix like this which avoids the closed intermediate state:
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 1f423f723d06..6ff4e03ad521 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -2826,6 +2826,27 @@ static void zbc_open_zone(struct sdebug_dev_info
> *devip,
> }
> }
>
> +static void zbc_set_zone_full(struct sdebug_dev_info *devip,
> + struct sdeb_zone_state *zsp)
> +{
> + switch (zsp->z_cond) {
> + case ZC2_IMPLICIT_OPEN:
> + devip->nr_imp_open--;
> + break;
> + case ZC3_EXPLICIT_OPEN:
> + devip->nr_exp_open--;
> + break;
> + case ZC4_CLOSED:
> + devip->nr_closed--;
> + break;
> + default:
> + break;
> + }
> +
> + zsp->z_wp = zsp->z_start + zsp->z_size;
> + zsp->z_cond = ZC5_FULL;
> +}
> +
> static void zbc_inc_wp(struct sdebug_dev_info *devip,
> unsigned long long lba, unsigned int num)
> {
> @@ -2838,7 +2859,7 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
> if (zsp->z_type == ZBC_ZTYPE_SWR) {
> zsp->z_wp += num;
> if (zsp->z_wp >= zend)
> - zsp->z_cond = ZC5_FULL;
> + zbc_set_zone_full(devip, zsp);
> return;
> }
>
> @@ -2857,7 +2878,7 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
> n = num;
> }
> if (zsp->z_wp >= zend)
> - zsp->z_cond = ZC5_FULL;
> + zbc_set_zone_full(devip, zsp);
>
> num -= n;
> lba += n;
> @@ -4731,14 +4752,8 @@ static void zbc_finish_zone(struct sdebug_dev_info
> *devip,
> enum sdebug_z_cond zc = zsp->z_cond;
>
> if (zc == ZC4_CLOSED || zc == ZC2_IMPLICIT_OPEN ||
> - zc == ZC3_EXPLICIT_OPEN || (empty && zc == ZC1_EMPTY)) {
> - if (zc == ZC2_IMPLICIT_OPEN || zc == ZC3_EXPLICIT_OPEN)
> - zbc_close_zone(devip, zsp);
> - if (zsp->z_cond == ZC4_CLOSED)
> - devip->nr_closed--;
> - zsp->z_wp = zsp->z_start + zsp->z_size;
> - zsp->z_cond = ZC5_FULL;
> - }
> + zc == ZC3_EXPLICIT_OPEN || (empty && zc == ZC1_EMPTY))
> + zbc_set_zone_full(devip, zsp);
> }
>
> static void zbc_finish_all(struct sdebug_dev_info *devip)
>
> Can send a v2 with that. Tested. It works fine too.
Looks good to me.
But if you send it, I think that you should send a 2/2 that cleans up
zbc_rwp_zone() as well, so that we remove the intermediate closed state
everywhere.
The reason why I really disliked the intermediate closed state when
transitioning to FULL/EMPTY, is because it is not in the spec.
For closed and empty zones, we do have an intermediate implicit open state,
e.g. for a finish zone operation, both in the spec and in the code
(check_zbc_access_params()), so this intermediate state was represented in
the code for a reason.
Someone reading the code could thus think that the intermediate closed state
also exists for a reason (is described in the spec), but that isn't the case.
Kind regards,
Niklas
