> 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
