Date: Wed, 19 Jun 2024 14:56:18 +0800
From: Li Feng <fengli@...rtx.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
 linux-kernel <linux-kernel@...r.kernel.org>,
 "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
 "Martin K. Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCH] scsi: sd: Keep the discard mode stable



> 2024年6月18日 14:41，Christoph Hellwig <hch@...radead.org> 写道：
> 
> On Mon, Jun 17, 2024 at 05:03:03PM +0800, Li Feng wrote:
>>> But more importantly this doesn't really scale to all the variations
>>> of reported / guessed at probe time vs overriden.  I think you just
>>> need an explicit override flag that skips the discard settings.
>>> 
>> I think we only need to prevent the temporary change of discard mode 
>> from UNMAP to WS16, and this patch should be enough.
>> 
>> Maybe it is a good idea to remove the call to sd_config_discard 
>> from read_capacity_16 . Because the unmap_alignment/ unmap_granularity
>> used by sd_config_discard are assigned in sd_read_block_limits. 
>> 
>> sd_read_block_limits is enough to negotiate the discard parameter. 
>> It is redundant for read_capacity to modify the discard parameter.  In this way, 
>> when the SCSI probe sends read_capacity first and then read block limits, 
>> it avoids the change of discard from DISABLE to WS16 to UNMAP.
> 
> Note that in the linux-next tree for 6.11 we're not only applying
> the discard choice to the queue_limits structure and not commixing
> it in read_capacity_16.  So it will be overriden before it gets
> actually applied.  Can you check that your issue doesn't show up in
> linux-next?
> 

I pulled the latest linux-next and the problem still appeared.

[ 302.773386] sd 0:0:0:3: [sdc] tag#104 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
[ 302.774839] sd 0:0:0:3: [sdc] tag#104 Sense Key : Illegal Request [current]
[ 302.775638] sd 0:0:0:3: [sdc] tag#104 Add. Sense: Invalid field in cdb
[ 302.776383] sd 0:0:0:3: [sdc] tag#104 CDB: Write same(16) 93 08 00 00 00 00 00 17 58 00 00 00 08 00 00 00
[ 302.777443] critical target error, dev sdc, sector 1529856 op 0x3:(DISCARD) flags 0x4000 phys_seg 1 prio class 0

The discard mode will undergo a transition from UNMAP->WS16->UNMAP during the rescan process.

This results in an unexpected WS16 discard command.

We can see that the SCSI probe stage calls the following in sequence:
sd_read_capacity
sd_read_block_provisioning(sdkp);
sd_read_block_limits(sdkp, &lim);

We only need to negotiate the discard mode after these function calls are completed.
In this way, there will be no temporary unexpected changes to DISCARD.

Also fixed a problem where the old code could to WS16 in set read_capacity_16 when sdkp->lbpws = 0.
For my SCSI disk, lbpws is equal to 0.

How does this patch?

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e01393ed4207..4c0962ebe7d5 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2622,7 +2622,6 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
              if (buffer[14] & 0x40) /* LBPRZ */
                      sdkp->lbprz = 1;

-               sd_config_discard(sdkp, lim, SD_LBP_WS16);
      }

      sdkp->capacity = lba + 1;
@@ -3271,8 +3270,6 @@ static void sd_read_block_limits(struct scsi_disk *sdkp,
              if (vpd->data[32] & 0x80)
                      sdkp->unmap_alignment =
                              get_unaligned_be32(&vpd->data[32]) & ~(1 << 31);
-
-               sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
      }

out:
@@ -3670,6 +3667,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
                      sd_zbc_read_zones(sdkp, &lim, buffer);
                      sd_read_cpr(sdkp);
              }
+               sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));

              sd_print_capacity(sdkp, old_capacity);


If it's OK, I'll submit v2.

Thanks,
Li

Powered by blists - more mailing lists