On 1/6/23 10:33 AM, Tejun Heo wrote:
> 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.
I looked over the call path, and I don't think anything in there sleeps.
So should be fine to remove the might_sleep().
--
Jens Axboe
