| Message ID | 20250226124006.1593985-7-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix lock order and remove redundant locking | expand |
| Context | Check | Description |
|---|---|---|
| shin/vmtest-linus-master-PR | success | PR summary |
| shin/vmtest-linus-master-VM_Test-0 | success | Logs for build-kernel |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Feb 26, 2025 at 06:09:59PM +0530, Nilay Shroff wrote: > The wbt latency and state could be updated while initializing the > elevator or exiting the elevator. It could be also updated while > configuring IO latency QoS parameters using cgroup. The elevator > code path is now protected with q->elevator_lock. So we should > protect the access to sysfs attribute wbt_lat_usec using q->elevator > _lock instead of q->sysfs_lock. White we're at it, also protect > ioc_qos_write(), which configures wbt parameters via cgroup, using > q->elevator_lock. > > Reviewed-by: Hannes Reinecke <hare@suse.de> > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
Hello,
kernel test robot noticed "sysfs:cannot_create_duplicate_filename" on:
commit: 2951441c42530952368c2bae7190ea8779b2385f ("[PATCHv5 6/7] block: protect wbt_lat_usec using q->elevator_lock")
url: https://github.com/intel-lab-lkp/linux/commits/Nilay-Shroff/block-acquire-q-limits_lock-while-reading-sysfs-attributes/20250226-204836
base: https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/all/20250226124006.1593985-7-nilay@linux.ibm.com/
patch subject: [PATCHv5 6/7] block: protect wbt_lat_usec using q->elevator_lock
in testcase: boot
config: x86_64-randconfig-074-20250228
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+----------------------------------------+------------+------------+
| | 96ec4f2440 | 2951441c42 |
+----------------------------------------+------------+------------+
| sysfs:cannot_create_duplicate_filename | 0 | 12 |
+----------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202503041413.872983bd-lkp@intel.com
[ 14.648633][ T1] Non-volatile memory driver v1.3
[ 14.649743][ T1] telclk_interrupt = 0xf non-mcpbl0010 hw.
[ 14.650659][ T1] usbcore: registered new interface driver xillyusb
[ 14.658940][ T1] zram: Added device: zram0
[ 14.659758][ T11] Floppy drive(s): fd0 is 2.88M AMI BIOS
[ 14.662273][ T1] sysfs: cannot create duplicate filename '/devices/virtual/block/nullb0/queue/wbt_lat_usec'
[ 14.663506][ T1] CPU: 0 UID: 0 PID: 1 Comm: swapper Not tainted 6.14.0-rc3-00270-g2951441c4253 #1 adcfde6e4f1dd47f52d455fb462243a4f271be2a
[ 14.664847][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 14.665952][ T1] Call Trace:
[ 14.666355][ T1] <TASK>
[ 14.666473][ T1] dump_stack_lvl (kbuild/src/consumer/lib/dump_stack.c:123 (discriminator 1))
[ 14.666473][ T1] dump_stack (kbuild/src/consumer/lib/dump_stack.c:130)
[ 14.666473][ T1] sysfs_warn_dup (kbuild/src/consumer/fs/sysfs/dir.c:32)
[ 14.666473][ T1] sysfs_add_file_mode_ns (kbuild/src/consumer/fs/sysfs/file.c:318)
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250304/202503041413.872983bd-lkp@intel.com
On 3/4/25 11:48 AM, kernel test robot wrote: > > > Hello, > > kernel test robot noticed "sysfs:cannot_create_duplicate_filename" on: > > commit: 2951441c42530952368c2bae7190ea8779b2385f ("[PATCHv5 6/7] block: protect wbt_lat_usec using q->elevator_lock") > url: https://github.com/intel-lab-lkp/linux/commits/Nilay-Shroff/block-acquire-q-limits_lock-while-reading-sysfs-attributes/20250226-204836 > base: https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git for-next > patch link: https://lore.kernel.org/all/20250226124006.1593985-7-nilay@linux.ibm.com/ > patch subject: [PATCHv5 6/7] block: protect wbt_lat_usec using q->elevator_lock > > in testcase: boot > > config: x86_64-randconfig-074-20250228 > compiler: gcc-12 > test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G > > (please refer to attached dmesg/kmsg for entire log/backtrace) > > > +----------------------------------------+------------+------------+ > | | 96ec4f2440 | 2951441c42 | > +----------------------------------------+------------+------------+ > | sysfs:cannot_create_duplicate_filename | 0 | 12 | > +----------------------------------------+------------+------------+ > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <oliver.sang@intel.com> > | Closes: https://lore.kernel.org/oe-lkp/202503041413.872983bd-lkp@intel.com > > > [ 14.648633][ T1] Non-volatile memory driver v1.3 > [ 14.649743][ T1] telclk_interrupt = 0xf non-mcpbl0010 hw. > [ 14.650659][ T1] usbcore: registered new interface driver xillyusb > [ 14.658940][ T1] zram: Added device: zram0 > [ 14.659758][ T11] Floppy drive(s): fd0 is 2.88M AMI BIOS > [ 14.662273][ T1] sysfs: cannot create duplicate filename '/devices/virtual/block/nullb0/queue/wbt_lat_usec' > [ 14.663506][ T1] CPU: 0 UID: 0 PID: 1 Comm: swapper Not tainted 6.14.0-rc3-00270-g2951441c4253 #1 adcfde6e4f1dd47f52d455fb462243a4f271be2a > [ 14.664847][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 > [ 14.665952][ T1] Call Trace: > [ 14.666355][ T1] <TASK> > [ 14.666473][ T1] dump_stack_lvl (kbuild/src/consumer/lib/dump_stack.c:123 (discriminator 1)) > [ 14.666473][ T1] dump_stack (kbuild/src/consumer/lib/dump_stack.c:130) > [ 14.666473][ T1] sysfs_warn_dup (kbuild/src/consumer/fs/sysfs/dir.c:32) > [ 14.666473][ T1] sysfs_add_file_mode_ns (kbuild/src/consumer/fs/sysfs/file.c:318) > > > The kernel config and materials to reproduce are available at: > https://download.01.org/0day-ci/archive/20250304/202503041413.872983bd-lkp@intel.com > Sorry, but there was a stale entry left over for wbt_lat_usec attribute causing the observed symptom.I will be sending next patchset with the fix. Thanks, --Nilay
diff --git a/block/blk-iocost.c b/block/blk-iocost.c index 65a1d4427ccf..c68373361301 100644 --- a/block/blk-iocost.c +++ b/block/blk-iocost.c @@ -3249,6 +3249,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input, } memflags = blk_mq_freeze_queue(disk->queue); + mutex_lock(&disk->queue->elevator_lock); blk_mq_quiesce_queue(disk->queue); spin_lock_irq(&ioc->lock); @@ -3356,6 +3357,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input, spin_unlock_irq(&ioc->lock); blk_mq_unquiesce_queue(disk->queue); + mutex_unlock(&disk->queue->elevator_lock); blk_mq_unfreeze_queue(disk->queue, memflags); ret = -EINVAL; diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index f1fa57de29ed..46033d640e30 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -557,7 +557,7 @@ static ssize_t queue_wb_lat_show(struct gendisk *disk, char *page) ssize_t ret; struct request_queue *q = disk->queue; - mutex_lock(&q->sysfs_lock); + mutex_lock(&q->elevator_lock); if (!wbt_rq_qos(q)) { ret = -EINVAL; goto out; @@ -570,7 +570,7 @@ static ssize_t queue_wb_lat_show(struct gendisk *disk, char *page) ret = sysfs_emit(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000)); out: - mutex_unlock(&q->sysfs_lock); + mutex_unlock(&q->elevator_lock); return ret; } @@ -589,8 +589,8 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page, if (val < -1) return -EINVAL; - mutex_lock(&q->sysfs_lock); memflags = blk_mq_freeze_queue(q); + mutex_lock(&q->elevator_lock); rqos = wbt_rq_qos(q); if (!rqos) { @@ -619,8 +619,8 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page, blk_mq_unquiesce_queue(q); out: + mutex_unlock(&q->elevator_lock); blk_mq_unfreeze_queue(q, memflags); - mutex_unlock(&q->sysfs_lock); return ret; } @@ -701,7 +701,9 @@ static struct attribute *blk_mq_queue_attrs[] = { */ &elv_iosched_entry.attr, &queue_requests_entry.attr, - +#ifdef CONFIG_BLK_WBT + &queue_wb_lat_entry.attr, +#endif /* * Attributes which don't require locking. */ @@ -882,10 +884,10 @@ int blk_register_queue(struct gendisk *disk) goto out_crypto_sysfs_unregister; } } + wbt_enable_default(disk); mutex_unlock(&q->elevator_lock); blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q); - wbt_enable_default(disk); /* Now everything is ready and send out KOBJ_ADD uevent */ kobject_uevent(&disk->queue_kobj, KOBJ_ADD); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 3e66ad016a23..0ee3b5c9388e 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -563,8 +563,8 @@ struct request_queue { /* * Protects against I/O scheduler switching, particularly when * updating q->elevator. Since the elevator update code path may - * also modify q->nr_requests, this lock also protects the sysfs - * attribute nr_requests. + * also modify q->nr_requests and wbt latency, this lock also + * protects the sysfs attributes nr_requests and wbt_lat_usec. * To ensure proper locking order during an elevator update, first * freeze the queue, then acquire ->elevator_lock. */