diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 76c47fe9b8d6..6eaccd18d8b4 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -143,7 +143,6 @@ queue_ra_store(struct gendisk *disk, const char *page, size_t count) { unsigned long ra_kb; ssize_t ret;
- unsigned int memflags; struct request_queue *q = disk->queue;
ret = queue_var_store(&ra_kb, page, count); @@ -154,10 +153,8 @@ queue_ra_store(struct gendisk *disk, const char *page, size_t count) * calculated from the queue limits by queue_limits_commit_update. */ mutex_lock(&q->limits_lock);
- memflags = blk_mq_freeze_queue(q);
- disk->bdi->ra_pages = ra_kb >> (PAGE_SHIFT - 10);
- data_race(disk->bdi->ra_pages = ra_kb >> (PAGE_SHIFT - 10)); mutex_unlock(&q->limits_lock);
- blk_mq_unfreeze_queue(q, memflags);
return ret; }
I think we don't need data_race() here as disk->bdi->ra_pages is already protected by ->limits_lock. Furthermore, while you're at it, I’d suggest protecting the set/get access of ->ra_pages using ->limits_lock when it’s invoked from the ioctl context (BLKRASET/BLKRAGET).
@@ -472,7 +460,7 @@ static ssize_t queue_io_timeout_show(struct gendisk *disk, char *page) static ssize_t queue_io_timeout_store(struct gendisk *disk, const char *page, size_t count) {
- unsigned int val, memflags;
- unsigned int val; int err; struct request_queue *q = disk->queue;
@@ -480,9 +468,7 @@ static ssize_t queue_io_timeout_store(struct gendisk *disk, const char *page, if (err || val == 0) return -EINVAL;
- memflags = blk_mq_freeze_queue(q);
- blk_queue_rq_timeout(q, msecs_to_jiffies(val));
- blk_mq_unfreeze_queue(q, memflags);
- data_race((blk_queue_rq_timeout(q, msecs_to_jiffies(val)), 0));
return count; }
The use of data_race() above seems redundant, since the update to q->rq_timeout is already marked with WRITE_ONCE(). However, the read access to q->rq_timeout in a few places within the I/O hotpath is not marked and instead accessed directly using plain C-language loads.
When I ran this proposed change with KCSAN enabled on my system, I observed the following warning ( when one thread was running I/O and another thread was concurrently updating q->rq_timeout):
BUG: KCSAN: data-race in blk_add_timer+0x74/0x1f0
race at unknown origin, with read to 0xc000000143c9c568 of 4 bytes by task 3725 on cpu 16: blk_add_timer+0x74/0x1f0 blk_mq_start_request+0xe0/0x558 nvme_prep_rq.part.0+0x300/0x194c [nvme] nvme_queue_rqs+0x1f0/0x4e4 [nvme] blk_mq_dispatch_queue_requests+0x510/0x8e0 blk_mq_flush_plug_list+0xc0/0x3d8 __blk_flush_plug+0x29c/0x368 __submit_bio+0x320/0x3bc submit_bio_noacct_nocheck+0x604/0x94c submit_bio_noacct+0x3b4/0xef8 submit_bio+0x7c/0x310 submit_bio_wait+0xd4/0x19c __blkdev_direct_IO_simple+0x290/0x484 blkdev_direct_IO+0x658/0x1000 blkdev_write_iter+0x530/0x67c vfs_write+0x698/0x894 sys_pwrite64+0x140/0x17c system_call_exception+0x1ac/0x520 system_call_vectored_common+0x15c/0x2ec
value changed: 0x00000bb8 -> 0x000005dc
Reported by Kernel Concurrency Sanitizer on: CPU: 16 UID: 0 PID: 3725 Comm: fio Kdump: loaded Not tainted 6.18.0-rc3+ #47 VOLUNTARY Hardware name: IBM,9105-22A Power11 (architected) 0x820200 0xf000007 of:IBM,FW1120.00 (RB1120_115) hv:phyp pSeries ==================================================================
Based on the gdb trace:
(gdb) info line *(blk_add_timer+0x74) Line 138 of "block/blk-timeout.c" starts at address 0xc000000000d5637c <blk_add_timer+108> and ends at 0xc000000000d5638c <blk_add_timer+124>.
This corresponds to:
128 void blk_add_timer(struct request *req) 129 { 130 struct request_queue *q = req->q; 131 unsigned long expiry; 132 133 /* 134 * Some LLDs, like scsi, peek at the timeout to prevent a 135 * command from being retried forever. 136 */ 137 if (!req->timeout) 138 req->timeout = q->rq_timeout;
As seen above, the read access to q->rq_timeout is unmarked. To avoid the reported data race, we should replace this plain access with READ_ONCE(q->rq_timeout). This is one instance in the I/O hotpath where q->rq_timeout is read without annotation, and there are likely a few more similar cases that should be updated in the same way.
Thanks, --Nilay