* Re: [PATCH v2] drivers/nvme/host: Fix namespace duplication check rule
2022-09-01 0:49 ` [PATCH v2] drivers/nvme/host: Fix namespace duplication check rule Sungup Moon
@ 2022-09-01 10:49 ` Sagi Grimberg
2022-09-05 5:49 ` hch
1 sibling, 0 replies; 3+ messages in thread
From: Sagi Grimberg @ 2022-09-01 10:49 UTC (permalink / raw)
To: sungup.moon, kbusch@kernel.org, axboe@fb.com, hch@lst.de
Cc: linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org
On 9/1/22 03:49, Sungup Moon wrote:
> Some NVMe device, use EUI64 and NGUID, has fixed value EUI64 on a
> sub-system because of the bit size of ID. Current kernel check the
> all IDs should have unique value in a sub-system and globally.
> However, if an namespace has duplicate IDs (not all) in a sub-system,
> current kernel raise "duplicate IDs in subsystem for nsid" error. But
> NVMe Specification defines the namespace unique identifier like this:
>
> When creating a namespace, the controller shall indicate a globally
> unique value in one or more of the following:
> a) the EUI64 field;
> b) the NGUID field; or
> c) a Namespace Identification Descriptor with the Namespace Identifier
> Type field set to 3h
> (reference: 7.11 Unique Identifier; NVM Express 1.4c spec)
>
> So, I suggest the modified nvme_subsys_check_duplicate_ids function
> checking uniqueness from all IDS to one more IDs.
>
> I missed the initializing of "duplicated" variable, so I reissue the
> version2 patch.
>
> Signed-off-by: Sungup Moon <sungup.moon@samsung.com>
> ---
> drivers/nvme/host/core.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index af367b22871b..8c2faa2881a4 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3902,24 +3902,35 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_ctrl *ctrl,
> return NULL;
> }
>
> +#define IDS_EQUAL(A, B) (memcmp(&(A), &(B), sizeof(A)) == 0)
No need for this macro IMO.
> +
> static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
> struct nvme_ns_ids *ids)
> {
> bool has_uuid = !uuid_is_null(&ids->uuid);
> bool has_nguid = memchr_inv(ids->nguid, 0, sizeof(ids->nguid));
> bool has_eui64 = memchr_inv(ids->eui64, 0, sizeof(ids->eui64));
> + bool duplicated;
> struct nvme_ns_head *h;
>
> lockdep_assert_held(&subsys->lock);
>
> list_for_each_entry(h, &subsys->nsheads, entry) {
> - if (has_uuid && uuid_equal(&ids->uuid, &h->ids.uuid))
> - return -EINVAL;
> - if (has_nguid &&
> - memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0)
> - return -EINVAL;
> - if (has_eui64 &&
> - memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0)
> + duplicated = has_uuid || has_nguid || has_eui64;
> +
> + if (has_uuid)
> + duplicated = duplicated &&
> + uuid_equal(&ids->uuid, &h->ids.uuid);
> +
> + if (has_nguid)
> + duplicated = duplicated &&
> + IDS_EQUAL(ids->nguid, h->ids.nguid);
> +
> + if (has_eui64)
> + duplicated = duplicated &&
> + IDS_EQUAL(ids->eui64, h->ids.eui64);
> +
> + if (duplicated)
> return -EINVAL;
That is very confusing.
So a ns can have an identifier that does not have to be unique? or that
every identifier that exist should be unique but at least one identifier
should exist?
The former seems weird...
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] drivers/nvme/host: Fix namespace duplication check rule
2022-09-01 0:49 ` [PATCH v2] drivers/nvme/host: Fix namespace duplication check rule Sungup Moon
2022-09-01 10:49 ` Sagi Grimberg
@ 2022-09-05 5:49 ` hch
1 sibling, 0 replies; 3+ messages in thread
From: hch @ 2022-09-05 5:49 UTC (permalink / raw)
To: Sungup Moon
Cc: kbusch@kernel.org, axboe@fb.com, hch@lst.de, sagi@grimberg.me,
linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org
On Thu, Sep 01, 2022 at 09:49:41AM +0900, Sungup Moon wrote:
> Some NVMe device, use EUI64 and NGUID, has fixed value EUI64 on a
> sub-system because of the bit size of ID.
Huh? Fixed value of a EUI64 is brokem, so we must warn and ignore
those. The whole point of the EUI64/NGUID/UUID is that they are
uniqueu per namespace.
^ permalink raw reply [flat|nested] 3+ messages in thread