Re: [PATCH] scsi: scsi_debug: fix zone transition to full condition

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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@opensource.wdc.com>
>>>> ---
>>>>    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@wdc.com>

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.


-- 
Damien Le Moal
Western Digital Research