Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Fri, 6 Jan 2023 07:33:20 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Dan Carpenter <error27@gmail.com>, Christoph Hellwig <hch@lst.de>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Jens Axboe <axboe@kernel.dk>
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [bug report] memcontrol: schedule throttling if we are congested
Message-ID: <Y7hbYPSdLqW++y/p@slm.duckdns.org>
References: <Y7g3L6fntnTtOm63@kili>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y7g3L6fntnTtOm63@kili>
Precedence: bulk

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