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;
Message-ID: <9ac3390c-055b-546c-f1f4-68350dfe04f8@kernel.dk>
Date:   Fri, 6 Jan 2023 11:49:33 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [bug report] memcontrol: schedule throttling if we are congested
Content-Language: en-US
To:     Tejun Heo <tj@kernel.org>, Dan Carpenter <error27@gmail.com>,
        Christoph Hellwig <hch@lst.de>,
        Luis Chamberlain <mcgrof@kernel.org>
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
References: <Y7g3L6fntnTtOm63@kili> <Y7hbYPSdLqW++y/p@slm.duckdns.org>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <Y7hbYPSdLqW++y/p@slm.duckdns.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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