[PATCH] nvme-loop: set blocking flag

[PATCH] nvme-loop: set blocking flag

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Commit 505363957fad ("nvmet: fix nvme status code when namespace is
disabled") introduced a mutex lock in the io path for target. The loop
target connects this to the blk-mq submission side, so it requires the
blocking flag set so that queue_rq doesn't happen inside an rcu context.

Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index e32790d8fc260..77dd809fe4507 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -479,7 +479,7 @@ static void nvme_loop_reset_ctrl_work(struct work_struct *work)
 static const struct nvme_ctrl_ops nvme_loop_ctrl_ops = {
 	.name			= "loop",
 	.module			= THIS_MODULE,
-	.flags			= NVME_F_FABRICS,
+	.flags			= NVME_F_FABRICS | NVME_F_BLOCKING,
 	.reg_read32		= nvmf_reg_read32,
 	.reg_read64		= nvmf_reg_read64,
 	.reg_write32		= nvmf_reg_write32,
-- 
2.43.5

Re: [PATCH] nvme-loop: set blocking flag

[Bart Van Assche]

On 10/17/24 10:20 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Commit 505363957fad ("nvmet: fix nvme status code when namespace is
> disabled") introduced a mutex lock in the io path for target. The loop
> target connects this to the blk-mq submission side, so it requires the
> blocking flag set so that queue_rq doesn't happen inside an rcu context.
> 
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/loop.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index e32790d8fc260..77dd809fe4507 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -479,7 +479,7 @@ static void nvme_loop_reset_ctrl_work(struct work_struct *work)
>   static const struct nvme_ctrl_ops nvme_loop_ctrl_ops = {
>   	.name			= "loop",
>   	.module			= THIS_MODULE,
> -	.flags			= NVME_F_FABRICS,
> +	.flags			= NVME_F_FABRICS | NVME_F_BLOCKING,
>   	.reg_read32		= nvmf_reg_read32,
>   	.reg_read64		= nvmf_reg_read64,
>   	.reg_write32		= nvmf_reg_write32,

Shouldn't this patch have Fixes: and Cc: stable tags?

Thanks,

Bart.

Re: [PATCH] nvme-loop: set blocking flag

[Jens Axboe]

On 10/17/24 11:20 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Commit 505363957fad ("nvmet: fix nvme status code when namespace is
> disabled") introduced a mutex lock in the io path for target. The loop
> target connects this to the blk-mq submission side, so it requires the
> blocking flag set so that queue_rq doesn't happen inside an rcu context.

Looks fine to me, but might be worth looking at doing that a bit
differently so that the target loop driver can remain non-blocking
as it should be considerably more efficient.

-- 
Jens Axboe

Re: [PATCH] nvme-loop: set blocking flag

[Keith Busch]

On Thu, Oct 17, 2024 at 11:39:24AM -0600, Jens Axboe wrote:
> On 10/17/24 11:20 AM, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Commit 505363957fad ("nvmet: fix nvme status code when namespace is
> > disabled") introduced a mutex lock in the io path for target. The loop
> > target connects this to the blk-mq submission side, so it requires the
> > blocking flag set so that queue_rq doesn't happen inside an rcu context.
> 
> Looks fine to me, but might be worth looking at doing that a bit
> differently so that the target loop driver can remain non-blocking
> as it should be considerably more efficient.

That's what I first looked at, but it's too hard. Maybe if we can make a
lookup that doesn't rely on searching the configfs group, but it's
definitely not this nice one-liner patch. :)

No rush on this today. I'll think more on it.

Re: [PATCH] nvme-loop: set blocking flag

[Christoph Hellwig]

On Thu, Oct 17, 2024 at 12:04:21PM -0600, Keith Busch wrote:
> > Looks fine to me, but might be worth looking at doing that a bit
> > differently so that the target loop driver can remain non-blocking
> > as it should be considerably more efficient.
> 
> That's what I first looked at, but it's too hard. Maybe if we can make a
> lookup that doesn't rely on searching the configfs group, but it's
> definitely not this nice one-liner patch. :)

Searching configfs in the I/O path sounds like a train wreck no matter
what.  I feel a bit bad for not noticing that earlier.

Let me see what I can do.

Re: [PATCH] nvme-loop: set blocking flag

[Sagi Grimberg]

On 18/10/2024 8:10, Christoph Hellwig wrote:
> On Thu, Oct 17, 2024 at 12:04:21PM -0600, Keith Busch wrote:
>>> Looks fine to me, but might be worth looking at doing that a bit
>>> differently so that the target loop driver can remain non-blocking
>>> as it should be considerably more efficient.
>> That's what I first looked at, but it's too hard. Maybe if we can make a
>> lookup that doesn't rely on searching the configfs group, but it's
>> definitely not this nice one-liner patch. :)
> Searching configfs in the I/O path sounds like a train wreck no matter
> what.  I feel a bit bad for not noticing that earlier.
>
> Let me see what I can do.

Note, the search is ONLY in the error case, i.e. when nsid is not found in
subsys->namespaces.

I agree its a shame to make the loop driver blocking for this niche case.

Note that this doesn't do anything, because it is really designed to 
make the
host failover work correctly (i.e. not fail IO) when nvmet disabled the 
ns in
one server, and the namespace is available in the same nvmet subsystem 
on another server.

We could escape this entirely for loop transport because it there are no 
multi-server nvmet
subsystems for loop? There is a weird case where one can use mpath 
device where one
path is loop and another is tcp/rdma... Which is very weird but maybe 
possible with nvmet.

Re: [PATCH] nvme-loop: set blocking flag

[Christoph Hellwig]

On Mon, Oct 21, 2024 at 12:35:17AM +0300, Sagi Grimberg wrote:
>> Let me see what I can do.
>
> Note, the search is ONLY in the error case, i.e. when nsid is not found in
> subsys->namespaces.

Which kinda makes it worse as we use an unusual context just from
that rare error path.

Re: [PATCH] nvme-loop: set blocking flag

[Christoph Hellwig]

On Tue, Oct 22, 2024 at 08:28:10AM +0200, Christoph Hellwig wrote:
> On Mon, Oct 21, 2024 at 12:35:17AM +0300, Sagi Grimberg wrote:
> >> Let me see what I can do.
> >
> > Note, the search is ONLY in the error case, i.e. when nsid is not found in
> > subsys->namespaces.
> 
> Which kinda makes it worse as we use an unusual context just from
> that rare error path.

I just took a very quick look, and inserting the namespace into the
xarray when created and only removing it when it is deleted, and
instead checking an enabled flag during lookup should do the work.

Do we have a good test case for commit 505363957fad ?

> 
---end quoted text---

Re: [PATCH] nvme-loop: set blocking flag

[Shinichiro Kawasaki]

On Oct 22, 2024 / 09:02, Christoph Hellwig wrote:
> On Tue, Oct 22, 2024 at 08:28:10AM +0200, Christoph Hellwig wrote:
> > On Mon, Oct 21, 2024 at 12:35:17AM +0300, Sagi Grimberg wrote:
> > >> Let me see what I can do.
> > >
> > > Note, the search is ONLY in the error case, i.e. when nsid is not found in
> > > subsys->namespaces.
> > 
> > Which kinda makes it worse as we use an unusual context just from
> > that rare error path.
> 
> I just took a very quick look, and inserting the namespace into the
> xarray when created and only removing it when it is deleted, and
> instead checking an enabled flag during lookup should do the work.
> 
> Do we have a good test case for commit 505363957fad ?

As for the BUG caused by the commit 505363957fad, the blktests test case
nvme/052 can confirm if any patch fixes the BUG or not. It is recreate in
stable manner on my test node [1], so I'm happy to help testing.

If you mean a test case to confirm that the commit 505363957fad achieves its
purpose, I'm not sure about it. According to its commit log, I guess Jirong
might have a test environment.

[1] https://lore.kernel.org/linux-nvme/tqcy3sveity7p56v7ywp7ssyviwcb3w4623cnxj3knoobfcanq@yxgt2mjkbkam/

Re: [PATCH] nvme-loop: set blocking flag

[hch]

On Tue, Oct 22, 2024 at 08:08:31AM +0000, Shinichiro Kawasaki wrote:
> As for the BUG caused by the commit 505363957fad, the blktests test case
> nvme/052 can confirm if any patch fixes the BUG or not. It is recreate in
> stable manner on my test node [1], so I'm happy to help testing.
> 
> If you mean a test case to confirm that the commit 505363957fad achieves its
> purpose, I'm not sure about it. According to its commit log, I guess Jirong
> might have a test environment.

Yes, that's the main thing I want to be able to test.

Re: [PATCH] nvme-loop: set blocking flag

[Sagi Grimberg]

On 22/10/2024 10:02, Christoph Hellwig wrote:
> On Tue, Oct 22, 2024 at 08:28:10AM +0200, Christoph Hellwig wrote:
>> On Mon, Oct 21, 2024 at 12:35:17AM +0300, Sagi Grimberg wrote:
>>>> Let me see what I can do.
>>> Note, the search is ONLY in the error case, i.e. when nsid is not found in
>>> subsys->namespaces.
>> Which kinda makes it worse as we use an unusual context just from
>> that rare error path.
> I just took a very quick look, and inserting the namespace into the
> xarray when created and only removing it when it is deleted, and
> instead checking an enabled flag during lookup should do the work.

Agreed.

Re: [PATCH] nvme-loop: set blocking flag

[Sagi Grimberg]

On 22/10/2024 9:28, Christoph Hellwig wrote:
> On Mon, Oct 21, 2024 at 12:35:17AM +0300, Sagi Grimberg wrote:
>>> Let me see what I can do.
>> Note, the search is ONLY in the error case, i.e. when nsid is not found in
>> subsys->namespaces.
> Which kinda makes it worse as we use an unusual context just from
> that rare error path.
>

I'd say that introducing this search in the hot path not only for error 
cases
would be worse :)

Yes, nvme-loop was an oversight.

Re: [PATCH] nvme-loop: set blocking flag

[Christoph Hellwig]

On Thu, Oct 17, 2024 at 11:39:24AM -0600, Jens Axboe wrote:
> On 10/17/24 11:20 AM, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Commit 505363957fad ("nvmet: fix nvme status code when namespace is
> > disabled") introduced a mutex lock in the io path for target. The loop
> > target connects this to the blk-mq submission side, so it requires the
> > blocking flag set so that queue_rq doesn't happen inside an rcu context.
> 
> Looks fine to me, but might be worth looking at doing that a bit
> differently so that the target loop driver can remain non-blocking
> as it should be considerably more efficient.

Yes.  When designing the nvmet code back in the day (gosh do I feel old)
we tried to avoid blocking in the main handling thread in general.

So if we can get rid of the mutex that would be great.

Re: [PATCH] nvme-loop: set blocking flag

[Shinichiro Kawasaki]

On Oct 17, 2024 / 10:20, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Commit 505363957fad ("nvmet: fix nvme status code when namespace is
> disabled") introduced a mutex lock in the io path for target. The loop
> target connects this to the blk-mq submission side, so it requires the
> blocking flag set so that queue_rq doesn't happen inside an rcu context.
> 
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Thank you Keith for this quick action. I confirmed the patch avoids the BUG.
I also ran whole blktests and observed no regression. In case this patch gets
applied as it is,

Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>