On 2025/6/12 23:03, Darrick J. Wong wrote:

On Thu, Jun 12, 2025 at 07:20:45PM +0800, Zhang Yi wrote:

quoted

On 2025/6/12 12:47, Christoph Hellwig wrote:

quoted

On Wed, Jun 11, 2025 at 03:31:21PM +0800, Zhang Yi wrote:

quoted

quoted

quoted

+/* 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.