On 9/12/2024 6:32 PM, Christoph Hellwig wrote:
> On Tue, Sep 10, 2024 at 08:31:59PM +0530, Kanchan Joshi wrote:
>> From: Nitesh Shetty <nj.shetty@xxxxxxxxxxx>
>>
>> The incoming hint value maybe either lifetime hint or placement hint.
>
> .. may either be .. ?
Sure.
>> Make SCSI interpret only temperature-based write lifetime hints.
>>
>> Signed-off-by: Nitesh Shetty <nj.shetty@xxxxxxxxxxx>
>> Signed-off-by: Kanchan Joshi <joshi.k@xxxxxxxxxxx>
>> ---
>> drivers/scsi/sd.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index dad3991397cf..82bd4b07314e 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1191,8 +1191,8 @@ static u8 sd_group_number(struct scsi_cmnd *cmd)
>> if (!sdkp->rscs)
>> return 0;
>>
>> - return min3((u32)rq->write_hint, (u32)sdkp->permanent_stream_count,
>> - 0x3fu);
>> + return min3((u32)WRITE_LIFETIME_HINT(rq->write_hint),
>
> No fan of the screaming WRITE_LIFETIME_HINT.
Macros tend to. Once it becomes lowercase (inline function), it will
stop screaming.
Or the fact that multiple
> things are multiplexed into the single rq->write_hint field to
> start with.
Please see the response in patch #1. My worries were:
(a) adding a new field and propagating it across the stack will cause
code duplication.
(b) to add a new field we need to carve space within inode, bio and
request.
We had a hole in request, but it is set to vanish after ongoing
integrity refactoring patch of Keith [1]. For inode also, there is no
liberty at this point [2].
I think current multiplexing approach is similar to ioprio where
multiple io priority classes/values are expressed within an int type.
And few kernel components choose to interpret certain ioprio values at will.
And all this is still in-kernel details. Which can be changed if/when
other factors start helping.
[1]
https://lore.kernel.org/linux-nvme/20240911201240.3982856-2-kbusch@xxxxxxxx/
[2]
https://lore.kernel.org/linux-nvme/20240903-erfassen-bandmitglieder-32dfaeee66b2@brauner/
