Linux-Block Archive on lore.kernel.org
 help / Atom feed
* [PATCH] block: don't show io_timeout if driver has no timeout handler
@ 2019-01-07 12:52 Weiping Zhang
  2019-01-07 15:43 ` Jens Axboe
  2019-01-09  0:57 ` Bart Van Assche
  0 siblings, 2 replies; 5+ messages in thread
From: Weiping Zhang @ 2019-01-07 12:52 UTC (permalink / raw)
  To: axboe; +Cc: bvanassche, hch, linux-block

If the low level driver has no timerout handler, the
/sys/block/<disk>/queue/io_timeout will not be displayed.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
 block/blk-sysfs.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 0619c8922893..e9ee81c43d36 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -726,7 +726,7 @@ static struct queue_sysfs_entry throtl_sample_time_entry = {
 };
 #endif
 
-static struct attribute *default_attrs[] = {
+static struct attribute *queue_attrs[] = {
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
 	&queue_max_hw_sectors_entry.attr,
@@ -768,6 +768,26 @@ static struct attribute *default_attrs[] = {
 	NULL,
 };
 
+static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr,
+				int n)
+{
+	struct request_queue *q =
+		container_of(kobj, struct request_queue, kobj);
+
+	if (attr == &queue_io_timeout_entry.attr) {
+		if (!q->mq_ops || !q->mq_ops->timeout)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
+static struct attribute_group queue_attr_group = {
+	.attrs = queue_attrs,
+	.is_visible = queue_attr_visible,
+};
+
+
 #define to_queue(atr) container_of((atr), struct queue_sysfs_entry, attr)
 
 static ssize_t
@@ -893,7 +913,6 @@ static const struct sysfs_ops queue_sysfs_ops = {
 
 struct kobj_type blk_queue_ktype = {
 	.sysfs_ops	= &queue_sysfs_ops,
-	.default_attrs	= default_attrs,
 	.release	= blk_release_queue,
 };
 
@@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk)
 		goto unlock;
 	}
 
+	ret = sysfs_create_group(&q->kobj, &queue_attr_group);
+	if (ret) {
+		kobject_del(&q->kobj);
+		blk_trace_remove_sysfs(dev);
+		kobject_put(&dev->kobj);
+		goto unlock;
+	}
+
 	if (queue_is_mq(q)) {
 		__blk_mq_register_dev(dev, q);
 		blk_mq_debugfs_register(q);
@@ -958,6 +985,7 @@ int blk_register_queue(struct gendisk *disk)
 		if (ret) {
 			mutex_unlock(&q->sysfs_lock);
 			kobject_uevent(&q->kobj, KOBJ_REMOVE);
+			sysfs_remove_group(&q->kobj, &queue_attr_group);
 			kobject_del(&q->kobj);
 			blk_trace_remove_sysfs(dev);
 			kobject_put(&dev->kobj);
@@ -1006,6 +1034,7 @@ void blk_unregister_queue(struct gendisk *disk)
 		blk_mq_unregister_dev(disk_to_dev(disk), q);
 	mutex_unlock(&q->sysfs_lock);
 
+	sysfs_remove_group(&q->kobj, &queue_attr_group);
 	kobject_uevent(&q->kobj, KOBJ_REMOVE);
 	kobject_del(&q->kobj);
 	blk_trace_remove_sysfs(disk_to_dev(disk));
-- 
2.14.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
  2019-01-07 12:52 [PATCH] block: don't show io_timeout if driver has no timeout handler Weiping Zhang
@ 2019-01-07 15:43 ` Jens Axboe
  2019-01-07 16:04   ` Bart Van Assche
  2019-01-09  0:57 ` Bart Van Assche
  1 sibling, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2019-01-07 15:43 UTC (permalink / raw)
  To: bvanassche, hch, linux-block

On 1/7/19 5:52 AM, Weiping Zhang wrote:
> If the low level driver has no timerout handler, the
> /sys/block/<disk>/queue/io_timeout will not be displayed.

The alternative would be to make it return an error when read/written
instead of having this separate group code.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
  2019-01-07 15:43 ` Jens Axboe
@ 2019-01-07 16:04   ` Bart Van Assche
  2019-01-07 16:06     ` Jens Axboe
  0 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2019-01-07 16:04 UTC (permalink / raw)
  To: Jens Axboe, hch, linux-block

On Mon, 2019-01-07 at 08:43 -0700, Jens Axboe wrote:
+AD4 On 1/7/19 5:52 AM, Weiping Zhang wrote:
+AD4 +AD4 If the low level driver has no timerout handler, the
+AD4 +AD4 /sys/block/+ADw-disk+AD4-/queue/io+AF8-timeout will not be displayed.
+AD4 
+AD4 The alternative would be to make it return an error when read/written
+AD4 instead of having this separate group code.

Hi Jens,

Christoph and I had asked Weiping to use a sysfs attribute group. See also
https://www.mail-archive.com/linux-block+AEA-vger.kernel.org/msg25841.html

Best regards,

Bart.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
  2019-01-07 16:04   ` Bart Van Assche
@ 2019-01-07 16:06     ` Jens Axboe
  0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2019-01-07 16:06 UTC (permalink / raw)
  To: Bart Van Assche, hch, linux-block

On 1/7/19 9:04 AM, Bart Van Assche wrote:
> On Mon, 2019-01-07 at 08:43 -0700, Jens Axboe wrote:
>> On 1/7/19 5:52 AM, Weiping Zhang wrote:
>>> If the low level driver has no timerout handler, the
>>> /sys/block/<disk>/queue/io_timeout will not be displayed.
>>
>> The alternative would be to make it return an error when read/written
>> instead of having this separate group code.
> 
> Hi Jens,
> 
> Christoph and I had asked Weiping to use a sysfs attribute group. See also
> https://www.mail-archive.com/linux-block@vger.kernel.org/msg25841.html

I think both solutions have parts that suck. If the file is only there
for some devices, that's a bit of an API wart. But at the same time, if the
file is there and not readable/writeable on those same devices, that's also
kind of ugly (as the wbt exercise has shown).

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
  2019-01-07 12:52 [PATCH] block: don't show io_timeout if driver has no timeout handler Weiping Zhang
  2019-01-07 15:43 ` Jens Axboe
@ 2019-01-09  0:57 ` Bart Van Assche
  1 sibling, 0 replies; 5+ messages in thread
From: Bart Van Assche @ 2019-01-09  0:57 UTC (permalink / raw)
  To: Weiping Zhang, axboe; +Cc: hch, linux-block

On Mon, 2019-01-07 at 20:52 +-0800, Weiping Zhang wrote:
+AD4 If the low level driver has no timerout handler, the
                                 +AF4AXgBeAF4AXgBeAF4AXg
                                 timeout?

+AD4 +-static umode+AF8-t queue+AF8-attr+AF8-visible(struct kobject +ACo-kobj, struct attribute +ACo-attr,
+AD4 +-				int n)
+AD4 +-+AHs
+AD4 +-	struct request+AF8-queue +ACo-q +AD0
+AD4 +-		container+AF8-of(kobj, struct request+AF8-queue, kobj)+ADs
+AD4 +-
+AD4 +-	if (attr +AD0APQ +ACY-queue+AF8-io+AF8-timeout+AF8-entry.attr) +AHs
+AD4 +-		if (+ACE-q-+AD4-mq+AF8-ops +AHwAfA +ACE-q-+AD4-mq+AF8-ops-+AD4-timeout)
+AD4 +-			return 0+ADs
+AD4 +-	+AH0

Are there any legacy block drivers left? Do we really need the mq+AF8-ops test?

Additionally, please combine the two nested if-statements into a single
if-statement.

+AD4 +AEAAQA -942,6 +-961,14 +AEAAQA int blk+AF8-register+AF8-queue(struct gendisk +ACo-disk)
+AD4  		goto unlock+ADs
+AD4  	+AH0
+AD4  
+AD4 +-	ret +AD0 sysfs+AF8-create+AF8-group(+ACY-q-+AD4-kobj, +ACY-queue+AF8-attr+AF8-group)+ADs
+AD4 +-	if (ret) +AHs
+AD4 +-		kobject+AF8-del(+ACY-q-+AD4-kobj)+ADs
+AD4 +-		blk+AF8-trace+AF8-remove+AF8-sysfs(dev)+ADs
+AD4 +-		kobject+AF8-put(+ACY-dev-+AD4-kobj)+ADs
+AD4 +-		goto unlock+ADs
+AD4 +-	+AH0

Are you sure the +ACI-goto unlock+ACI is OK here? Shouldn't kobject+AF8-del() be called
to undo the kobject+AF8-add() call if sysfs+AF8-create+AF8-group() fails?

+AD4  	if (queue+AF8-is+AF8-mq(q)) +AHs
+AD4  		+AF8AXw-blk+AF8-mq+AF8-register+AF8-dev(dev, q)+ADs
+AD4  		blk+AF8-mq+AF8-debugfs+AF8-register(q)+ADs
+AD4 +AEAAQA -958,6 +-985,7 +AEAAQA int blk+AF8-register+AF8-queue(struct gendisk +ACo-disk)
+AD4  		if (ret) +AHs
+AD4  			mutex+AF8-unlock(+ACY-q-+AD4-sysfs+AF8-lock)+ADs
+AD4  			kobject+AF8-uevent(+ACY-q-+AD4-kobj, KOBJ+AF8-REMOVE)+ADs
+AD4 +-			sysfs+AF8-remove+AF8-group(+ACY-q-+AD4-kobj, +ACY-queue+AF8-attr+AF8-group)+ADs
+AD4  			kobject+AF8-del(+ACY-q-+AD4-kobj)+ADs
+AD4  			blk+AF8-trace+AF8-remove+AF8-sysfs(dev)+ADs
+AD4  			kobject+AF8-put(+ACY-dev-+AD4-kobj)+ADs
+AD4 +AEAAQA -1006,6 +-1034,7 +AEAAQA void blk+AF8-unregister+AF8-queue(struct gendisk +ACo-disk)
+AD4  		blk+AF8-mq+AF8-unregister+AF8-dev(disk+AF8-to+AF8-dev(disk), q)+ADs
+AD4  	mutex+AF8-unlock(+ACY-q-+AD4-sysfs+AF8-lock)+ADs
+AD4  
+AD4 +-	sysfs+AF8-remove+AF8-group(+ACY-q-+AD4-kobj, +ACY-queue+AF8-attr+AF8-group)+ADs
+AD4  	kobject+AF8-uevent(+ACY-q-+AD4-kobj, KOBJ+AF8-REMOVE)+ADs
+AD4  	kobject+AF8-del(+ACY-q-+AD4-kobj)+ADs
+AD4  	blk+AF8-trace+AF8-remove+AF8-sysfs(disk+AF8-to+AF8-dev(disk))+ADs

Is it necessary to call sysfs+AF8-remove+AF8-group() explicitly? Isn't this something
kobject+AF8-del() does automatically?

Thanks,

Bart.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-07 12:52 [PATCH] block: don't show io_timeout if driver has no timeout handler Weiping Zhang
2019-01-07 15:43 ` Jens Axboe
2019-01-07 16:04   ` Bart Van Assche
2019-01-07 16:06     ` Jens Axboe
2019-01-09  0:57 ` Bart Van Assche

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox