| Message ID | Y7iFwjN+XzWvLv3y@slm.duckdns.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [block/for-6.2-fixes] block: Drop spurious might_sleep() from blk_put_queue() | expand |
On Fri, Jan 06, 2023 at 10:34:10AM -1000, Tejun Heo wrote: > Dan reports the following smatch detected the following: > > block/blk-cgroup.c:1863 blkcg_schedule_throttle() warn: sleeping in atomic context > > caused by blkcg_schedule_throttle() calling blk_put_queue() in an > non-sleepable context. > > blk_put_queue() acquired might_sleep() in 63f93fd6fa57 ("block: mark > blk_put_queue as potentially blocking") which transferred the might_sleep() > from blk_free_queue(). > > blk_free_queue() acquired might_sleep() in e8c7d14ac6c3 ("block: revert back > to synchronous request_queue removal") while turning request_queue removal > synchronous. However, this isn't necessary as nothing in the free path > actually requires sleeping. > > It's pretty unusual to require a sleeping context in a put operation and > it's not needed in the first place. Let's drop it. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Reported-by: Dan Carpenter <error27@gmail.com> > Link: https://lkml.kernel.org/r/Y7g3L6fntnTtOm63@kili > Cc: Christoph Hellwig <hch@lst.de> > Cc: Luis Chamberlain <mcgrof@kernel.org> > Fixes: e8c7d14ac6c3 ("block: revert back to synchronous request_queue removal") # v5.9+ *tons* has changed since e8c7d14ac6c3 and so the bots might think that *if* this patch is applied upstream it is justified for older kernels and I don't think that's yet been verified and doubt it. And so I think adding a "Fixes" tag is not appropriate here. First I'd like to hear from Christoph if he agrees with this patch upstream. For stable, someone would have to do the homework. Luis
On Fri, Jan 06, 2023 at 12:45:12PM -0800, Luis Chamberlain wrote: > *tons* has changed since e8c7d14ac6c3 and so the bots might think that > *if* this patch is applied upstream it is justified for older kernels > and I don't think that's yet been verified and doubt it. Didn't look like anything relevant changed to me. Besides, all that the patch does is removing a might_sleep() which can't hurt anything.
On 1/6/23 1:45 PM, Luis Chamberlain wrote: > On Fri, Jan 06, 2023 at 10:34:10AM -1000, Tejun Heo wrote: >> Dan reports the following smatch detected the following: >> >> block/blk-cgroup.c:1863 blkcg_schedule_throttle() warn: sleeping in atomic context >> >> caused by blkcg_schedule_throttle() calling blk_put_queue() in an >> non-sleepable context. >> >> blk_put_queue() acquired might_sleep() in 63f93fd6fa57 ("block: mark >> blk_put_queue as potentially blocking") which transferred the might_sleep() >> from blk_free_queue(). >> >> blk_free_queue() acquired might_sleep() in e8c7d14ac6c3 ("block: revert back >> to synchronous request_queue removal") while turning request_queue removal >> synchronous. However, this isn't necessary as nothing in the free path >> actually requires sleeping. >> >> It's pretty unusual to require a sleeping context in a put operation and >> it's not needed in the first place. Let's drop it. >> >> Signed-off-by: Tejun Heo <tj@kernel.org> >> Reported-by: Dan Carpenter <error27@gmail.com> >> Link: https://lkml.kernel.org/r/Y7g3L6fntnTtOm63@kili >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Luis Chamberlain <mcgrof@kernel.org> >> Fixes: e8c7d14ac6c3 ("block: revert back to synchronous request_queue removal") # v5.9+ > > *tons* has changed since e8c7d14ac6c3 and so the bots might think that > *if* this patch is applied upstream it is justified for older kernels > and I don't think that's yet been verified and doubt it. > > And so I think adding a "Fixes" tag is not appropriate here. > > First I'd like to hear from Christoph if he agrees with this patch > upstream. For stable, someone would have to do the homework. Outside of the easily audited paths, the kobj release paths are the only ones of concern. And I didn't spot anything that sleeps. Looks fine to me.
On Fri, Jan 06, 2023 at 12:45:12PM -0800, Luis Chamberlain wrote: > > Signed-off-by: Tejun Heo <tj@kernel.org> > > Reported-by: Dan Carpenter <error27@gmail.com> > > Link: https://lkml.kernel.org/r/Y7g3L6fntnTtOm63@kili > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Luis Chamberlain <mcgrof@kernel.org> > > Fixes: e8c7d14ac6c3 ("block: revert back to synchronous request_queue removal") # v5.9+ > > *tons* has changed since e8c7d14ac6c3 and so the bots might think that > *if* this patch is applied upstream it is justified for older kernels > and I don't think that's yet been verified and doubt it. If you enable the correct debug option then the might_sleep() causes a stack trace. Eventually syzbot will find it. I would backport it. regards, dan carpenter
I walked through everything called from blk_free_queue and can't find
anything that sleeps either, so:
Reviewed-by: Christoph Hellwig <hch@lst.de>
I wonder if we could not also revert
d578c770c85233af592e54537f93f3831bde7e9a as I think that was added
to avoid calling blk_put_queue from atomic context.
On Fri, 06 Jan 2023 10:34:10 -1000, Tejun Heo wrote: > Dan reports the following smatch detected the following: > > block/blk-cgroup.c:1863 blkcg_schedule_throttle() warn: sleeping in atomic context > > caused by blkcg_schedule_throttle() calling blk_put_queue() in an > non-sleepable context. > > [...] Applied, thanks! [1/1] block: Drop spurious might_sleep() from blk_put_queue() commit: 49e4d04f0486117ac57a97890eb1db6d52bf82b3 Best regards,
diff --git a/block/blk-core.c b/block/blk-core.c index 9321767470dc..b5098355d8b2 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -283,12 +283,9 @@ static void blk_free_queue(struct request_queue *q) * * Decrements the refcount of the request_queue and free it when the refcount * reaches 0. - * - * Context: Can sleep. */ void blk_put_queue(struct request_queue *q) { - might_sleep(); if (refcount_dec_and_test(&q->refs)) blk_free_queue(q); }
Dan reports the following smatch detected the following: block/blk-cgroup.c:1863 blkcg_schedule_throttle() warn: sleeping in atomic context caused by blkcg_schedule_throttle() calling blk_put_queue() in an non-sleepable context. blk_put_queue() acquired might_sleep() in 63f93fd6fa57 ("block: mark blk_put_queue as potentially blocking") which transferred the might_sleep() from blk_free_queue(). blk_free_queue() acquired might_sleep() in e8c7d14ac6c3 ("block: revert back to synchronous request_queue removal") while turning request_queue removal synchronous. However, this isn't necessary as nothing in the free path actually requires sleeping. It's pretty unusual to require a sleeping context in a put operation and it's not needed in the first place. Let's drop it. Signed-off-by: Tejun Heo <tj@kernel.org> Reported-by: Dan Carpenter <error27@gmail.com> Link: https://lkml.kernel.org/r/Y7g3L6fntnTtOm63@kili Cc: Christoph Hellwig <hch@lst.de> Cc: Luis Chamberlain <mcgrof@kernel.org> Fixes: e8c7d14ac6c3 ("block: revert back to synchronous request_queue removal") # v5.9+ -- block/blk-core.c | 3 --- 1 file changed, 3 deletions(-)