From: Christoph Hellwig <hch@lst.de>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
Hillf Danton <hdanton@sina.com>,
Pavel Tatashin <pasha.tatashin@soleen.com>,
Tyler Hicks <tyhicks@linux.microsoft.com>,
linux-block <linux-block@vger.kernel.org>
Subject: Re: [PATCH] loop: replace loop_ctl_mutex with loop_idr_spinlock
Date: Sat, 28 Aug 2021 09:18:32 +0200 [thread overview]
Message-ID: <20210828071832.GA31755@lst.de> (raw)
In-Reply-To: <73c53177-be1b-cff1-a09e-ef7979a95200@i-love.sakura.ne.jp>
On Sat, Aug 28, 2021 at 10:10:36AM +0900, Tetsuo Handa wrote:
> That is, it seems that unregister_transfer_cb() is there in case forced module
> unload of cryptoloop module was requested. And in that case, there is no point
> with crashing the kernel via panic_on_warn == 1 && WARN_ON_ONCE(). Simple printk()
> will be sufficient.
If we have that case for forced module unload a WARN_ON is the right thing.
That being said we can simply do the cmpxchg based protection for that
case as well if you want to keep it. That will lead to a spurious
loop remove failure with -EBUSY when a concurrent force module removal
for cryptoloop is happening, but if you do something like that you get
to keep the pieces.
> In order to annotate that extra operations that might sleep should not be
> added inside this section. Use of sleepable locks tends to get extra
> operations (e.g. wait for a different mutex / completion) and makes it unclear
> what the lock is protecting. I can imagine a future that someone adds an
> unwanted dependency inside this section if we use mutex here.
>
> Technically, we can add preempt_diable() after mutex_lock() and
> preempt_enable() before mutex_unlock() in order to annotate that
> extra operations that might sleep should be avoided.
> But idr_alloc(GFP_ATOMIC)/idr_find()/idr_for_each_entry() etc. will be
> fast enough.
Well, split that into a cleanup patch if you think it is worth the
effort, with a good changelog. Not really part of the bug fix.
next prev parent reply other threads:[~2021-08-28 7:18 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-26 16:03 [PATCH] loop: replace loop_ctl_mutex with loop_idr_spinlock Tetsuo Handa
2021-08-27 18:43 ` Christoph Hellwig
2021-08-28 1:10 ` Tetsuo Handa
2021-08-28 2:22 ` Tetsuo Handa
2021-08-28 7:18 ` Christoph Hellwig [this message]
2021-08-28 13:50 ` Tetsuo Handa
2021-08-29 1:22 ` [PATCH v2] loop: reduce the loop_ctl_mutex scope Tetsuo Handa
2021-08-29 13:47 ` [PATCH v3] " Tetsuo Handa
2021-08-30 7:13 ` Christoph Hellwig
2021-09-01 5:36 ` Christoph Hellwig
2021-09-01 5:47 ` Tetsuo Handa
2021-09-01 6:10 ` Christoph Hellwig
2021-09-02 0:07 ` [PATCH v3 (repost)] " Tetsuo Handa
2021-09-04 4:16 ` [PATCH v3] " Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210828071832.GA31755@lst.de \
--to=hch@lst.de \
--cc=axboe@kernel.dk \
--cc=hdanton@sina.com \
--cc=linux-block@vger.kernel.org \
--cc=pasha.tatashin@soleen.com \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=tyhicks@linux.microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).