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

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

 



On 1/27/23 22:00, Hannes Reinecke wrote:
> On 1/24/23 20:02, Niklas Cassel wrote:
>> From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
>> ---
>>   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@xxxxxxx>
> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux