All of lore.kernel.org
 help / color / mirror / Atom feed
From: Keith Busch <kbusch@kernel.org>
To: "Guilherme G. Piccoli" <gpiccoli@canonical.com>
Cc: "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"axboe@fb.com" <axboe@fb.com>, "hch@lst.de" <hch@lst.de>,
	"sagi@grimberg.me" <sagi@grimberg.me>,
	"jay.vosburgh@canonical.com" <jay.vosburgh@canonical.com>
Subject: Re: [PATCH] nvme: Use first ctrl->instance id as subsystem id
Date: Wed, 14 Aug 2019 10:06:41 -0600	[thread overview]
Message-ID: <20190814160640.GA3256@localhost.localdomain> (raw)
In-Reply-To: <20190814142836.2322-1-gpiccoli@canonical.com>

On Wed, Aug 14, 2019 at 07:28:36AM -0700, Guilherme G. Piccoli wrote:
> Since after the introduction of NVMe multipath, we have a struct to
> track subsystems, and more important, we have now the nvme block device
> name bound to the subsystem id instead of ctrl->instance as before.
> This is not a big problem, users can even fallback to the old behavior
> using the module parameter "nvme_core.multipath=N" in case they don't
> have multipath and wish to have a consistent mapping between the char
> device nvmeX and the block device nvmeXnY.
> 
> That said, we noticed the nvme subsystem id is generated by its own ID
> allocator, and ctrl->instance value has itself an ID allocator too.
> The controller instance is generated during the probe, in the function
> nvme_init_ctrl(), which always executes before nvme_init_subsystem().
> That said, and since according to the spec we have a relation 1:N
> between subsystem and controllers (i.e., one subsystem may have more
> controllers but not the reciprocal), why not use the ctrl->instance id
> as the subsystem id?

The subsystem lifetime is not tied to a single controller's. Disconnect
the "first" controller in a multipathed subsystem with this patch, then
connect another controller from a different subsystem, and now you will
create naming collisions.

WARNING: multiple messages have this Message-ID (diff)
From: kbusch@kernel.org (Keith Busch)
Subject: [PATCH] nvme: Use first ctrl->instance id as subsystem id
Date: Wed, 14 Aug 2019 10:06:41 -0600	[thread overview]
Message-ID: <20190814160640.GA3256@localhost.localdomain> (raw)
In-Reply-To: <20190814142836.2322-1-gpiccoli@canonical.com>

On Wed, Aug 14, 2019@07:28:36AM -0700, Guilherme G. Piccoli wrote:
> Since after the introduction of NVMe multipath, we have a struct to
> track subsystems, and more important, we have now the nvme block device
> name bound to the subsystem id instead of ctrl->instance as before.
> This is not a big problem, users can even fallback to the old behavior
> using the module parameter "nvme_core.multipath=N" in case they don't
> have multipath and wish to have a consistent mapping between the char
> device nvmeX and the block device nvmeXnY.
> 
> That said, we noticed the nvme subsystem id is generated by its own ID
> allocator, and ctrl->instance value has itself an ID allocator too.
> The controller instance is generated during the probe, in the function
> nvme_init_ctrl(), which always executes before nvme_init_subsystem().
> That said, and since according to the spec we have a relation 1:N
> between subsystem and controllers (i.e., one subsystem may have more
> controllers but not the reciprocal), why not use the ctrl->instance id
> as the subsystem id?

The subsystem lifetime is not tied to a single controller's. Disconnect
the "first" controller in a multipathed subsystem with this patch, then
connect another controller from a different subsystem, and now you will
create naming collisions.

  reply	other threads:[~2019-08-14 16:08 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 14:28 [PATCH] nvme: Use first ctrl->instance id as subsystem id Guilherme G. Piccoli
2019-08-14 14:28 ` Guilherme G. Piccoli
2019-08-14 16:06 ` Keith Busch [this message]
2019-08-14 16:06   ` Keith Busch
2019-08-14 16:18   ` Guilherme G. Piccoli
2019-08-14 16:18     ` Guilherme G. Piccoli
2019-08-14 16:27     ` Keith Busch
2019-08-14 16:27       ` Keith Busch
2019-08-14 18:29       ` Guilherme G. Piccoli
2019-08-14 18:29         ` Guilherme G. Piccoli
2019-08-14 19:08         ` Keith Busch
2019-08-14 19:08           ` Keith Busch
2019-08-15 13:01           ` Guilherme G. Piccoli
2019-08-15 13:01             ` Guilherme G. Piccoli

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=20190814160640.GA3256@localhost.localdomain \
    --to=kbusch@kernel.org \
    --cc=axboe@fb.com \
    --cc=gpiccoli@canonical.com \
    --cc=hch@lst.de \
    --cc=jay.vosburgh@canonical.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.