Re: [PATCH] scsi: sd: Keep the discard mode stable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 2024年6月18日 14:41,Christoph Hellwig <hch@xxxxxxxxxxxxx> 写道:
> 
> 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






[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux