Re: [PATCH v3 07/18] scsi: sd: detect support for command duration limits

Damien Le Moal
2023-01-28T00:51:57Z

On 1/27/23 22:00, Hannes Reinecke wrote:
> On 1/24/23 20:02, Niklas Cassel wrote:
>> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>
>> Detect if a disk supports command duration limits. Support for
>> the READ 16, WRITE 16, READ 32 and WRITE 32 commands is tested using
>> the function scsi_report_opcode(). For a disk supporting command
>> duration limits, the mode page indicating the command duration limits
>> descriptors that apply to the command is indicated using the rwcdlp
>> and cdlp bits.
>>
>> Support duration limits is advertizes through sysfs using the new
>> "duration_limits" sysfs sub-directory of the generic device directory,
>> that is, /sys/block/sdX/device/duration_limits. Within this new
>> directory, the limit descriptors that apply to read and write operations
>> are exposed within the read and write directories, with descriptor
>> attributes grouped together in directories. The overall sysfs structure
>> created is:
>>
>> /sys/block/sde/device/duration_limits/
>> ├── perf_vs_duration_guideline
>> ├── read
>> │   ├── 1
>> │   │   ├── duration_guideline
>> │   │   ├── duration_guideline_policy
>> │   │   ├── max_active_time
>> │   │   ├── max_active_time_policy
>> │   │   ├── max_inactive_time
>> │   │   └── max_inactive_time_policy
>> │   ├── 2
>> │   │   ├── duration_guideline
>> ...
>> │   └── page
>> └── write
>> ├── 1
>> │   ├── duration_guideline
>> │   ├── duration_guideline_policy
>> ...
>>
>> For each of the read and write descriptor directories, the page
>> attribute file indicate the command duration limit page providing the
>> descriptors. The possible values for the page attribute are "A", "B",
>> "T2A" and "T2B".
>>
>> The new "duration_limits" attributes directory is added only for disks
>> that supports command duration limits.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> ---
>> drivers/scsi/Makefile | 2 +-
>> drivers/scsi/sd.c | 2 +
>> drivers/scsi/sd.h | 61 ++++
>> drivers/scsi/sd_cdl.c | 764 ++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 828 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/scsi/sd_cdl.c
>>
> I'm not particularly happy with having sysfs reflect user settings, but
> every other place I can think of is even more convoluted.
> So there.
>
>> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
>> index f055bfd54a68..0e48cb6d21d6 100644
>> --- a/drivers/scsi/Makefile
>> +++ b/drivers/scsi/Makefile
>> @@ -170,7 +170,7 @@ scsi_mod-$(CONFIG_BLK_DEV_BSG) += scsi_bsg.o
>>
>> hv_storvsc-y := storvsc_drv.o
>>
>> -sd_mod-objs := sd.o
>> +sd_mod-objs := sd.o sd_cdl.o
>> sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
>> sd_mod-$(CONFIG_BLK_DEV_ZONED) += sd_zbc.o
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 45945bfeee92..7879a5470773 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -3326,6 +3326,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>> sd_read_write_same(sdkp, buffer);
>> sd_read_security(sdkp, buffer);
>> sd_config_protection(sdkp);
>> + sd_read_cdl(sdkp, buffer);
>> }
>>
>> /*
>> @@ -3646,6 +3647,7 @@ static void scsi_disk_release(struct device *dev)
>>
>> ida_free(&sd_index_ida, sdkp->index);
>> sd_zbc_free_zone_info(sdkp);
>> + sd_cdl_release(sdkp);
>> put_device(&sdkp->device->sdev_gendev);
>> free_opal_dev(sdkp->opal_dev);
>>
> Hmm. Calling this during revalidate() makes sense, but how can we ensure
> that we call revalidate() when the user issues a MODE_SELECT command?

Given that CDLs can be changed with a passthrough command, I do not think we can
do anything about that, unfortunately. But I think the same is true of many
things like that. E.g. "let's turn onf/off the write cache without the kernel
noticing"... But given that on a normal system only privileged applications can
do passthrough, if that happens, then the system has been hacked or the user is
shooting himself in the foot.

cdl-tools project (cdladm utility) uses passtrhough but triggers a revalidate
after changing CDLs to make sure sysfs stays in sync.

As Christoph suggested, we could change all this to an ioctl(GET_CDL) for
applications... But sysfs is so much simpler in my opinion, not to mention that
it allows access to the information for any application written in a language
that does not have ioctl() or an equivalent.

cdl-tools has a test suite all written in bash scripts thanks to the sysfs
interface :)

>
> Other than that:
>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
>
> Cheers,
>
> Hannes

--
Damien Le Moal
Western Digital Research