On Fri, Jan 06, 2023 at 11:49:33AM -0700, Jens Axboe wrote:
> 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
The rationale was that we reverted exepected userspace expection for
something that was sync to async so broke userspace expectations and
we can't do that.
> > 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().
As soon as commit 63f93fd6fa5717 ("block: mark blk_put_queue as
potentially blocking") on v6.2-rc1 it was upgraded to might_sleep()
directly on blk_put_queue(), I can't find a rationale after that
to justify the removal. But since it is not clear if we keep it,
we should document that rationale.
Luis
