[PATCH 0/3] scsi: disable discard when set target full provisioning

[PATCH 0/3] scsi: disable discard when set target full provisioning

[Haoqian He]

Hi all,

this series has a few fixes for the scsi discard mode changes.
The first disable the disacrd when set the target lun full
provisioning, the reset have some cleanups.

The series based on ("scsi: sd: Keep the discard mode stable"):
https://lore.kernel.org/all/20240619071412.140100-1-fengli@smartx.com

Haoqian He (3):
  scsi: sd: disable discard when set target full provisioning
  scsi: sd: remove scsi_disk field lbpvpd
  scsi: sd: remove some redundant initialization code

 drivers/scsi/sd.c | 34 ++++++++++++++++++----------------
 drivers/scsi/sd.h |  1 -
 2 files changed, 18 insertions(+), 17 deletions(-)

-- 
2.44.0

[PATCH 1/3] scsi: sd: disable discard when set target full provisioning

[Haoqian He]

When the target lun is set to full provisioning, the kernel
cannot perceive this change, so the kernel still thinks the
device supports the discard feature.

Discard will be disabled only after encountering a discard
IO error (a fully provisioned logical unit does not support
logical block provisioning management, so subsequent discard
IO will fail) or reconnection.

To fix this issue, we can disable device discard feature as
soon as possible during the iSCSI initiator rescanning session.

Specifically, we can reset lbpme bit 0 during the SCSI probe
if found the target lun does not support lbpm, then adjust the
discard mode to SD_LBP_DISABLE.

With this patch, the kernel can sync whether the target lun
supports logical block provisioning management after the iSCSI
initiator rescanning session, without IO error or reconnection.

Signed-off-by: Haoqian He <haoqian.he@smartx.com>
Signed-off-by: Li Feng <fengli@smartx.com>
---
 drivers/scsi/sd.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 548c74ecc836..44a19945b5b6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2709,6 +2709,9 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 
 		if (buffer[14] & 0x40) /* LBPRZ */
 			sdkp->lbprz = 1;
+	} else {
+		sdkp->lbpme = 0;
+		sdkp->lbprz = 0;
 	}
 
 	sdkp->capacity = lba + 1;
@@ -3303,12 +3306,9 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
 
 static unsigned int sd_discard_mode(struct scsi_disk *sdkp)
 {
-	if (!sdkp->lbpvpd) {
-		/* LBP VPD page not provided */
-		if (sdkp->max_unmap_blocks)
-			return SD_LBP_UNMAP;
-		return SD_LBP_WS16;
-	}
+	if (!sdkp->lbpvpd)
+		/* Disable discard if LBP VPD page not provided */
+		return SD_LBP_DISABLE;
 
 	/* LBP VPD page tells us what to use */
 	if (sdkp->lbpu && sdkp->max_unmap_blocks)
@@ -3343,8 +3343,12 @@ static void sd_read_block_limits(struct scsi_disk *sdkp,
 
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&vpd->data[36]);
 
-		if (!sdkp->lbpme)
+		if (!sdkp->lbpme) {
+			sdkp->max_unmap_blocks = 0;
+			sdkp->unmap_granularity = 0;
+			sdkp->unmap_alignment = 0;
 			goto config_atomic;
+		}
 
 		lba_count = get_unaligned_be32(&vpd->data[20]);
 		desc_count = get_unaligned_be32(&vpd->data[24]);
@@ -3425,8 +3429,13 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 {
 	struct scsi_vpd *vpd;
 
-	if (sdkp->lbpme == 0)
+	if (!sdkp->lbpme) {
+		sdkp->lbpvpd    = 0;
+		sdkp->lbpu      = 0;
+		sdkp->lbpws     = 0;
+		sdkp->lbpws10   = 0;
 		return;
+	}
 
 	rcu_read_lock();
 	vpd = rcu_dereference(sdkp->device->vpd_pgb2);
-- 
2.44.0

Re: [PATCH 1/3] scsi: sd: disable discard when set target full provisioning

[Martin K. Petersen]

Haoqian,

> +	if (!sdkp->lbpvpd)
> +		/* Disable discard if LBP VPD page not provided */
> +		return SD_LBP_DISABLE;

That is not a valid assumption. Many devices which support thin
provisioning either predate the LBP VPD being defined or have decided
not to support that page. The current heuristics are very deliberate.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 2/3] scsi: sd: remove scsi_disk field lbpvpd

[Haoqian He]

The lbpme bit in scsi_disk can be used directly to indicate
if the logical unit supports logical block provisioning
management. The lbpvpd bit is no longer needed, so remove
this field from scsi_disk.

Signed-off-by: Haoqian He <haoqian.he@smartx.com>
Signed-off-by: Li Feng <fengli@smartx.com>
---
 drivers/scsi/sd.c | 8 ++++----
 drivers/scsi/sd.h | 1 -
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 44a19945b5b6..b49bab1d8610 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3306,8 +3306,10 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
 
 static unsigned int sd_discard_mode(struct scsi_disk *sdkp)
 {
-	if (!sdkp->lbpvpd)
-		/* Disable discard if LBP VPD page not provided */
+	if (!sdkp->lbpme)
+		/* LBPME was not set means the logical unit
+		 * is fully provisioned, so disable discard.
+		 */
 		return SD_LBP_DISABLE;
 
 	/* LBP VPD page tells us what to use */
@@ -3430,7 +3432,6 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 	struct scsi_vpd *vpd;
 
 	if (!sdkp->lbpme) {
-		sdkp->lbpvpd    = 0;
 		sdkp->lbpu      = 0;
 		sdkp->lbpws     = 0;
 		sdkp->lbpws10   = 0;
@@ -3445,7 +3446,6 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 		return;
 	}
 
-	sdkp->lbpvpd	= 1;
 	sdkp->lbpu	= (vpd->data[5] >> 7) & 1; /* UNMAP */
 	sdkp->lbpws	= (vpd->data[5] >> 6) & 1; /* WRITE SAME(16) w/ UNMAP */
 	sdkp->lbpws10	= (vpd->data[5] >> 5) & 1; /* WRITE SAME(10) w/ UNMAP */
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 36382eca941c..ff9ff2655c25 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -146,7 +146,6 @@ struct scsi_disk {
 	unsigned	lbpu : 1;
 	unsigned	lbpws : 1;
 	unsigned	lbpws10 : 1;
-	unsigned	lbpvpd : 1;
 	unsigned	ws10 : 1;
 	unsigned	ws16 : 1;
 	unsigned	rc_basis: 2;
-- 
2.44.0

Re: [PATCH 2/3] scsi: sd: remove scsi_disk field lbpvpd

[Damien Le Moal]

On 7/2/24 12:01, Haoqian He wrote:
> The lbpme bit in scsi_disk can be used directly to indicate
> if the logical unit supports logical block provisioning
> management. The lbpvpd bit is no longer needed, so remove
> this field from scsi_disk.
> 
> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
>  drivers/scsi/sd.c | 8 ++++----
>  drivers/scsi/sd.h | 1 -
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 44a19945b5b6..b49bab1d8610 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3306,8 +3306,10 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
>  
>  static unsigned int sd_discard_mode(struct scsi_disk *sdkp)
>  {
> -	if (!sdkp->lbpvpd)
> -		/* Disable discard if LBP VPD page not provided */
> +	if (!sdkp->lbpme)
> +		/* LBPME was not set means the logical unit
> +		 * is fully provisioned, so disable discard.
> +		 */

Incorrect multi-line comment format. Please start the comment with a "/*" line
and no text. It may also be a good idea to add curly brackets for this if as it
is multi line (but single statement). Or move the comment before the if.

>  		return SD_LBP_DISABLE;
>  
>  	/* LBP VPD page tells us what to use */
> @@ -3430,7 +3432,6 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
>  	struct scsi_vpd *vpd;
>  
>  	if (!sdkp->lbpme) {
> -		sdkp->lbpvpd    = 0;
>  		sdkp->lbpu      = 0;
>  		sdkp->lbpws     = 0;
>  		sdkp->lbpws10   = 0;
> @@ -3445,7 +3446,6 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
>  		return;
>  	}
>  
> -	sdkp->lbpvpd	= 1;
>  	sdkp->lbpu	= (vpd->data[5] >> 7) & 1; /* UNMAP */
>  	sdkp->lbpws	= (vpd->data[5] >> 6) & 1; /* WRITE SAME(16) w/ UNMAP */
>  	sdkp->lbpws10	= (vpd->data[5] >> 5) & 1; /* WRITE SAME(10) w/ UNMAP */
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 36382eca941c..ff9ff2655c25 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -146,7 +146,6 @@ struct scsi_disk {
>  	unsigned	lbpu : 1;
>  	unsigned	lbpws : 1;
>  	unsigned	lbpws10 : 1;
> -	unsigned	lbpvpd : 1;
>  	unsigned	ws10 : 1;
>  	unsigned	ws16 : 1;
>  	unsigned	rc_basis: 2;

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/3] scsi: sd: remove some redundant initialization code

[Haoqian He]

Since the memory allocated by kzalloc for sdkp has been
initialized to 0, the code that initializes some sdkp
fields to 0 is no longer needed.

Signed-off-by: Haoqian He <haoqian.he@smartx.com>
Signed-off-by: Li Feng <fengli@smartx.com>
---
 drivers/scsi/sd.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b49bab1d8610..c7268780c642 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3957,7 +3957,6 @@ static int sd_probe(struct device *dev)
 	sdkp->disk = gd;
 	sdkp->index = index;
 	sdkp->max_retries = SD_MAX_RETRIES;
-	atomic_set(&sdkp->openers, 0);
 	atomic_set(&sdkp->device->ioerr_cnt, 0);
 
 	if (!sdp->request_queue->rq_timeout) {
@@ -3990,13 +3989,7 @@ static int sd_probe(struct device *dev)
 
 	/* defaults, until the device tells us otherwise */
 	sdp->sector_size = 512;
-	sdkp->capacity = 0;
 	sdkp->media_present = 1;
-	sdkp->write_prot = 0;
-	sdkp->cache_override = 0;
-	sdkp->WCE = 0;
-	sdkp->RCD = 0;
-	sdkp->ATO = 0;
 	sdkp->first_scan = 1;
 	sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
 
-- 
2.44.0

Re: [PATCH 3/3] scsi: sd: remove some redundant initialization code

[Damien Le Moal]

On 7/2/24 12:01, Haoqian He wrote:
> Since the memory allocated by kzalloc for sdkp has been
> initialized to 0, the code that initializes some sdkp
> fields to 0 is no longer needed.
> 
> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
> Signed-off-by: Li Feng <fengli@smartx.com>

Looks OK to me.

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] scsi: sd: remove some redundant initialization code

[Haoqian He]

> 2024年7月2日 11:33，Damien Le Moal <dlemoal@kernel.org> 写道：
> 
> On 7/2/24 12:01, Haoqian He wrote:
>> Since the memory allocated by kzalloc for sdkp has been
>> initialized to 0, the code that initializes some sdkp
>> fields to 0 is no longer needed.
>> 
>> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
>> Signed-off-by: Li Feng <fengli@smartx.com>
> 
> Looks OK to me.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

Hi Martin,

According to the SBC-3 SPEC:
"The device server in a logical unit that supports logical block provisioning
management shall set the LBPME bit to one in the parameter data returned for
a READ CAPACITY (16) command."

So we can use lbpme bit instead of lbpvpd to indicate if the device is thin
provisioned, which was implemented in patch 2 ("scsi: sd: remove scsi_disk
field lbpvpd").

Thanks,
Haoqian