Hello,
(cc'ing Luis, Christoph and Jens and quoting whole body)
On Fri, Jan 06, 2023 at 05:58:55PM +0300, Dan Carpenter wrote:
> Hello Tejun Heo,
>
> The patch 2cf855837b89: "memcontrol: schedule throttling if we are
> congested" from Jul 3, 2018, leads to the following Smatch static
> checker warning:
>
> block/blk-cgroup.c:1863 blkcg_schedule_throttle() warn: sleeping in atomic context
>
> The call tree looks like:
>
> ioc_rqos_merge() <- disables preempt
> __cgroup_throttle_swaprate() <- disables preempt
> -> blkcg_schedule_throttle()
>
> Here is one of the callers:
> mm/swapfile.c
> 3657 spin_lock(&swap_avail_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Takes spin lock.
>
> 3658 plist_for_each_entry_safe(si, next, &swap_avail_heads[nid],
> 3659 avail_lists[nid]) {
> 3660 if (si->bdev) {
> 3661 blkcg_schedule_throttle(si->bdev->bd_disk, true);
> ^^^^^^^^^^^^^^^^^^^^^^^
> Calls blkcg_schedule_throttle().
>
> 3662 break;
> 3663 }
> 3664 }
>
> block/blk-cgroup.c
> 1851 void blkcg_schedule_throttle(struct gendisk *disk, bool use_memdelay)
> 1852 {
> 1853 struct request_queue *q = disk->queue;
> 1854
> 1855 if (unlikely(current->flags & PF_KTHREAD))
> 1856 return;
> 1857
> 1858 if (current->throttle_queue != q) {
> 1859 if (!blk_get_queue(q))
> 1860 return;
> 1861
> 1862 if (current->throttle_queue)
> 1863 blk_put_queue(current->throttle_queue);
> ^^^^^^^^^^^^^^
> Sleeps.
>
> 1864 current->throttle_queue = q;
> 1865 }
> 1866
> 1867 if (use_memdelay)
> 1868 current->use_memdelay = use_memdelay;
> 1869 set_notify_resume(current);
> 1870 }
In general, it's quite unusual for a put operation to require a sleepable
context and I could be missing sth but the actual put / release paths don't
seem to actually need might_sleep(). It seems sprious.
The might_sleep() in put was added by Christoph's 63f93fd6fa57 ("block: mark
blk_put_queue as potentially blocking") which promoted it from release to
put cuz the caller usually can't tell whether its put is the last put.
And that put in release was added by Luis in e8c7d14ac6c3 ("block: revert
back to synchronous request_queue removal") while making the release path
synchronous, the rationale being that releasing asynchronously makes dynamic
device removal / readdition behaviors unpredictable and it also seems to
note that might_sleep() is no longer needed but still kept, which seems a
bit odd to me.
Here's my take on it:
* Let's please not require a sleepable context in a put operation. It's
unusual, inconvenient and error-prone, and likely to cause its users to
implement multiple copies of async mechanisms around it.
* A better way to deal with removal / readdition race is flushing release
operaitons either at the end of removal or before trying to add something
(you can get fancy w/ flushing only if there's name collision too), not
making a put path synchronously call release which needs to sleep.
* If might_sleep() is currently not needed, let's please drop it. It just
makes people scratch their head when reading the code.
Thanks.
--
tejun
