On 2025/6/12 23:03, Darrick J. Wong wrote:
> On Thu, Jun 12, 2025 at 07:20:45PM +0800, Zhang Yi wrote:
>> On 2025/6/12 12:47, Christoph Hellwig wrote:
>>> On Wed, Jun 11, 2025 at 03:31:21PM +0800, Zhang Yi wrote:
>>>>>> +/* supports unmap write zeroes command */
>>>>>> +#define BLK_FEAT_WRITE_ZEROES_UNMAP ((__force blk_features_t)(1u <<
>>>>>> 17))
>>>>>
>>>>>
>>>>> Should this be exposed through sysfs as a read-only value?
>>>>
>>>> Uh, are you suggesting adding another sysfs interface to expose
>>>> this feature?
>>>
>>> That was the idea. Or do we have another way to report this capability?
>>>
>>
>> Exposing this feature looks useful, but I think adding a new interface
>> might be somewhat redundant, and it's also difficult to name the new
>> interface. What about extend this interface to include 3 types? When
>> read, it exposes the following:
>>
>> - none : the device doesn't support BLK_FEAT_WRITE_ZEROES_UNMAP.
>> - enabled : the device supports BLK_FEAT_WRITE_ZEROES_UNMAP, but the
>> BLK_FLAG_WRITE_ZEROES_UNMAP_DISABLED is not set.
>> - disabled : the device supports BLK_FEAT_WRITE_ZEROES_UNMAP, and the
>> BLK_FLAG_WRITE_ZEROES_UNMAP_DISABLED is set.
>>
>> Users can write '0' and '1' to disable and enable this operation if it
>> is not 'none', thoughts?
>
> Perhaps it should reuse the enumeration pattern elsewhere in sysfs?
> For example,
>
> # cat /sys/block/sda/queue/scheduler
> none [mq-deadline]
> # echo none > /sys/block/sda/queue/scheduler
> # cat /sys/block/sda/queue/scheduler
> [none] mq-deadline
>
> (Annoying that this seems to be opencoded wherever it appears...)
>
Yeah, this solution looks good to me. However, we currently have only
two selections (none and unmap). What if we keep it as is and simply
hide this interface if BLK_FEAT_WRITE_ZEROES_UNMAP is not set, making
it visible only when the device supports this feature? Something like
below:
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e918b2c93aed..204ee4d5f63f 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -747,6 +747,9 @@ static umode_t queue_attr_visible(struct kobject *kobj,
struct attribute *attr,
attr == &queue_max_active_zones_entry.attr) &&
!blk_queue_is_zoned(q))
return 0;
+ if (attr == &queue_write_zeroes_unmap_entry.attr &&
+ !(q->limits.features & BLK_FEAT_WRITE_ZEROES_UNMAP))
+ return 0;
return attr->mode;
}
Thanks,
Yi.
