[PATCH] lightnvm: move device L2P detection to core

[PATCH] lightnvm: move device L2P detection to core

[Matias Bjørling]

A 1.2 device is able to manage the logical to physical mapping
table internally or leave it to the host.

A target only supports one of those approaches, and therefore must
check on initialization. Move this check to core to avoid each target
implement the check.

Signed-off-by: Matias Bjørling <mb@lightnvm.io>
---
 drivers/lightnvm/core.c      | 5 +++++
 drivers/lightnvm/pblk-init.c | 7 -------
 include/linux/lightnvm.h     | 6 ++++++
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 68553c7ae937..964352720a03 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -355,6 +355,11 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 		return -EINVAL;
 	}
 
+	if ((tt->flags & NVM_TGT_F_HOST_L2P) != (dev->geo.dom & NVM_RSP_L2P)) {
+		pr_err("nvm: device is incompatible with target L2P type.\n");
+		return -EINVAL;
+	}
+
 	if (nvm_target_exists(create->tgtname)) {
 		pr_err("nvm: target name already exists (%s)\n",
 							create->tgtname);
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index e9e2fedff387..cc8a59e3c240 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -1202,13 +1202,6 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
 	pblk->state = PBLK_STATE_RUNNING;
 	pblk->gc.gc_enabled = 0;
 
-	if (geo->version == NVM_OCSSD_SPEC_12 && geo->dom & NVM_RSP_L2P) {
-		pblk_err(pblk, "host-side L2P table not supported. (%x)\n",
-							geo->dom);
-		kfree(pblk);
-		return ERR_PTR(-EINVAL);
-	}
-
 	spin_lock_init(&pblk->resubmit_lock);
 	spin_lock_init(&pblk->trans_lock);
 	spin_lock_init(&pblk->lock);
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 8acc2fe277d6..f4a84694e5e2 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -495,9 +495,15 @@ typedef void (nvm_tgt_exit_fn)(void *, bool);
 typedef int (nvm_tgt_sysfs_init_fn)(struct gendisk *);
 typedef void (nvm_tgt_sysfs_exit_fn)(struct gendisk *);
 
+enum {
+	NVM_TGT_F_DEV_L2P = 0,
+	NVM_TGT_F_HOST_L2P = 1 << 0,
+};
+
 struct nvm_tgt_type {
 	const char *name;
 	unsigned int version[3];
+	int flags;
 
 	/* target entry points */
 	nvm_tgt_make_rq_fn *make_rq;
-- 
2.11.0

Re: [PATCH] lightnvm: move device L2P detection to core

[Javier Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> A 1.2 device is able to manage the logical to physical mapping
> table internally or leave it to the host.
> 
> A target only supports one of those approaches, and therefore must
> check on initialization. Move this check to core to avoid each target
> implement the check.
> 
> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
> ---


I see where you want to go with these changes, but the way targets are
layered on top of the LightNVM subsystem does not align with it.
LightNVM can support different OCSSD versions and capabilities, but that
does not mean that a target (e.g., pblk) does. The way I see it, core
should only check for (i) the drive to expose itself in a known revision
and (ii) the reported structures to be consistent. However, specific
functionality is not for core to check upo.

In this particular case, the NVM_TGT_F_HOST_L2P check should be pblk
specific, as it is indeed a 1.2 capability not supported by pblk.
However, a different target supporting this 1.2 feature can be
implemented on top.

As mentioned before, I started looking at moving checks to core too and
my initial thought was to add a .capabilities entry to nvm_tgt_type{} so
that these can be checked prior target initialization. Alternatively, we
can move all these checks to pblk-init.c. At this point, it is safe to
push 1.2 / 2.0 specific functionality to core as registered targets
would have validated the spec and the actual supported capabilities.

Thoughts?

Javier

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] lightnvm: move device L2P detection to core

[Matias Bjørling]

On 08/03/2018 02:16 PM, Javier Gonzalez wrote:
>> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> A 1.2 device is able to manage the logical to physical mapping
>> table internally or leave it to the host.
>>
>> A target only supports one of those approaches, and therefore must
>> check on initialization. Move this check to core to avoid each target
>> implement the check.
>>
>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>> ---
> 
> 
> I see where you want to go with these changes, but the way targets are
> layered on top of the LightNVM subsystem does not align with it.
> LightNVM can support different OCSSD versions and capabilities, but that
> does not mean that a target (e.g., pblk) does. The way I see it, core
> should only check for (i) the drive to expose itself in a known revision
> and (ii) the reported structures to be consistent. However, specific
> functionality is not for core to check upo.

Why try to initialize a target, if we already know that it is incompatible?

Re: [PATCH] lightnvm: move device L2P detection to core

[Javier Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

> On 3 Aug 2018, at 14.37, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 08/03/2018 02:16 PM, Javier Gonzalez wrote:
>>> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
>>> 
>>> A 1.2 device is able to manage the logical to physical mapping
>>> table internally or leave it to the host.
>>> 
>>> A target only supports one of those approaches, and therefore must
>>> check on initialization. Move this check to core to avoid each target
>>> implement the check.
>>> 
>>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>>> ---
>> I see where you want to go with these changes, but the way targets are
>> layered on top of the LightNVM subsystem does not align with it.
>> LightNVM can support different OCSSD versions and capabilities, but that
>> does not mean that a target (e.g., pblk) does. The way I see it, core
>> should only check for (i) the drive to expose itself in a known revision
>> and (ii) the reported structures to be consistent. However, specific
>> functionality is not for core to check upo.
> 
> Why try to initialize a target, if we already know that it is incompatible?

Yes, that is my point. But the one who knows if the targets supports
something or not is the target, not the subsystem. Here, you are making
an assumption knowing the pblk requires the L2P on the host, but that
could change in the future...

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] lightnvm: move device L2P detection to core

[Matias Bjørling]

On 08/03/2018 02:40 PM, Javier Gonzalez wrote:
>> On 3 Aug 2018, at 14.37, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 08/03/2018 02:16 PM, Javier Gonzalez wrote:
>>>> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>
>>>> A 1.2 device is able to manage the logical to physical mapping
>>>> table internally or leave it to the host.
>>>>
>>>> A target only supports one of those approaches, and therefore must
>>>> check on initialization. Move this check to core to avoid each target
>>>> implement the check.
>>>>
>>>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>>>> ---
>>> I see where you want to go with these changes, but the way targets are
>>> layered on top of the LightNVM subsystem does not align with it.
>>> LightNVM can support different OCSSD versions and capabilities, but that
>>> does not mean that a target (e.g., pblk) does. The way I see it, core
>>> should only check for (i) the drive to expose itself in a known revision
>>> and (ii) the reported structures to be consistent. However, specific
>>> functionality is not for core to check upo.
>>
>> Why try to initialize a target, if we already know that it is incompatible?
> 
> Yes, that is my point. But the one who knows if the targets supports
> something or not is the target, not the subsystem. Here, you are making
> an assumption knowing the pblk requires the L2P on the host, but that
> could change in the future...
> 

I don't believe it can. It is not supported by the 2.0 specification. 
1.2 is legacy.

I understand this from the perspective when checking for un-even 
configurations using the geometry. But this is a spec incompatibility, 
which I don't think the target should care about.

Re: [PATCH] lightnvm: move device L2P detection to core

[Javier Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

> On 3 Aug 2018, at 14.43, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 08/03/2018 02:40 PM, Javier Gonzalez wrote:
>>> On 3 Aug 2018, at 14.37, Matias Bjørling <mb@lightnvm.io> wrote:
>>> 
>>> On 08/03/2018 02:16 PM, Javier Gonzalez wrote:
>>>>> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>> 
>>>>> A 1.2 device is able to manage the logical to physical mapping
>>>>> table internally or leave it to the host.
>>>>> 
>>>>> A target only supports one of those approaches, and therefore must
>>>>> check on initialization. Move this check to core to avoid each target
>>>>> implement the check.
>>>>> 
>>>>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>>>>> ---
>>>> I see where you want to go with these changes, but the way targets are
>>>> layered on top of the LightNVM subsystem does not align with it.
>>>> LightNVM can support different OCSSD versions and capabilities, but that
>>>> does not mean that a target (e.g., pblk) does. The way I see it, core
>>>> should only check for (i) the drive to expose itself in a known revision
>>>> and (ii) the reported structures to be consistent. However, specific
>>>> functionality is not for core to check upo.
>>> 
>>> Why try to initialize a target, if we already know that it is incompatible?
>> Yes, that is my point. But the one who knows if the targets supports
>> something or not is the target, not the subsystem. Here, you are making
>> an assumption knowing the pblk requires the L2P on the host, but that
>> could change in the future...
> 
> I don't believe it can. It is not supported by the 2.0 specification.
> 1.2 is legacy.
> 

Ja... We both know that people is using 1.2 variants out there...

> I understand this from the perspective when checking for un-even
> configurations using the geometry. But this is a spec incompatibility,
> which I don't think the target should care about.

I see the point of not having this check in pblk since we know that we
are moving towards 2.0 and leaving 1.2 as legacy/not-upstream. But does
it really make sense to fail LightNVM on a 1.2 capability that is spec.
compliant? For all we know people could have this and use it from user
space or through an internal target.

Javier

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] lightnvm: move device L2P detection to core

[Matias Bjørling]

On 08/03/2018 02:55 PM, Javier Gonzalez wrote:
>> On 3 Aug 2018, at 14.43, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 08/03/2018 02:40 PM, Javier Gonzalez wrote:
>>>> On 3 Aug 2018, at 14.37, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>
>>>> On 08/03/2018 02:16 PM, Javier Gonzalez wrote:
>>>>>> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>>>
>>>>>> A 1.2 device is able to manage the logical to physical mapping
>>>>>> table internally or leave it to the host.
>>>>>>
>>>>>> A target only supports one of those approaches, and therefore must
>>>>>> check on initialization. Move this check to core to avoid each target
>>>>>> implement the check.
>>>>>>
>>>>>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>>>>>> ---
>>>>> I see where you want to go with these changes, but the way targets are
>>>>> layered on top of the LightNVM subsystem does not align with it.
>>>>> LightNVM can support different OCSSD versions and capabilities, but that
>>>>> does not mean that a target (e.g., pblk) does. The way I see it, core
>>>>> should only check for (i) the drive to expose itself in a known revision
>>>>> and (ii) the reported structures to be consistent. However, specific
>>>>> functionality is not for core to check upo.
>>>>
>>>> Why try to initialize a target, if we already know that it is incompatible?
>>> Yes, that is my point. But the one who knows if the targets supports
>>> something or not is the target, not the subsystem. Here, you are making
>>> an assumption knowing the pblk requires the L2P on the host, but that
>>> could change in the future...
>>
>> I don't believe it can. It is not supported by the 2.0 specification.
>> 1.2 is legacy.
>>
> 
> Ja... We both know that people is using 1.2 variants out there...

Yes, I'm not saying that. I'm saying the spec 1.2 has been deprecated by 
2.0 and no longer developed, and I'm not going to move that brainfart 
into the 2.0 spec :)

> 
>> I understand this from the perspective when checking for un-even
>> configurations using the geometry. But this is a spec incompatibility,
>> which I don't think the target should care about.
> 
> I see the point of not having this check in pblk since we know that we
> are moving towards 2.0 and leaving 1.2 as legacy/not-upstream. But does
> it really make sense to fail LightNVM on a 1.2 capability that is spec.
> compliant? For all we know people could have this and use it from user
> space or through an internal target.
>

It only fails on target creation, not on disk initialization. The disk 
will be up so user-space and targets that implements. Is that a problem?

Re: [PATCH] lightnvm: move device L2P detection to core

[Javier Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

> On 3 Aug 2018, at 15.01, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 08/03/2018 02:55 PM, Javier Gonzalez wrote:
>>> On 3 Aug 2018, at 14.43, Matias Bjørling <mb@lightnvm.io> wrote:
>>> 
>>> On 08/03/2018 02:40 PM, Javier Gonzalez wrote:
>>>>> On 3 Aug 2018, at 14.37, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>> 
>>>>> On 08/03/2018 02:16 PM, Javier Gonzalez wrote:
>>>>>>> On 3 Aug 2018, at 10.54, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>>>> 
>>>>>>> A 1.2 device is able to manage the logical to physical mapping
>>>>>>> table internally or leave it to the host.
>>>>>>> 
>>>>>>> A target only supports one of those approaches, and therefore must
>>>>>>> check on initialization. Move this check to core to avoid each target
>>>>>>> implement the check.
>>>>>>> 
>>>>>>> Signed-off-by: Matias Bjørling <mb@lightnvm.io>
>>>>>>> ---
>>>>>> I see where you want to go with these changes, but the way targets are
>>>>>> layered on top of the LightNVM subsystem does not align with it.
>>>>>> LightNVM can support different OCSSD versions and capabilities, but that
>>>>>> does not mean that a target (e.g., pblk) does. The way I see it, core
>>>>>> should only check for (i) the drive to expose itself in a known revision
>>>>>> and (ii) the reported structures to be consistent. However, specific
>>>>>> functionality is not for core to check upo.
>>>>> 
>>>>> Why try to initialize a target, if we already know that it is incompatible?
>>>> Yes, that is my point. But the one who knows if the targets supports
>>>> something or not is the target, not the subsystem. Here, you are making
>>>> an assumption knowing the pblk requires the L2P on the host, but that
>>>> could change in the future...
>>> 
>>> I don't believe it can. It is not supported by the 2.0 specification.
>>> 1.2 is legacy.
>> Ja... We both know that people is using 1.2 variants out there...
> 
> Yes, I'm not saying that. I'm saying the spec 1.2 has been deprecated
> by 2.0 and no longer developed,

But we need to maintain it either way.

> and I'm not going to move that brainfart into the 2.0 spec :)

I would not ask you to. Luckily, no-one has asked for it yet...

>>> I understand this from the perspective when checking for un-even
>>> configurations using the geometry. But this is a spec incompatibility,
>>> which I don't think the target should care about.
>> I see the point of not having this check in pblk since we know that we
>> are moving towards 2.0 and leaving 1.2 as legacy/not-upstream. But does
>> it really make sense to fail LightNVM on a 1.2 capability that is spec.
>> compliant? For all we know people could have this and use it from user
>> space or through an internal target.
> 
> It only fails on target creation, not on disk initialization. The disk
> will be up so user-space and targets that implements. Is that a
> problem?

My only point is that there are different responsibilities for core and
targets and some things are supported by 1.2 and not by 2.0, but that is
ok. If someone wants to create a hybrid 1.2 device on top of LightNVN -
in my view -, they should be allowed to.

Anyway, this is more a general layering discussion than actual
opposition to the patch itself.

Javier

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]