Re: [PATCH 5/5] lightnvm: pblk: Disable interleaved metadata in pblk

[Matias Bjørling]

On Mon, Jun 18, 2018 at 4:29 PM, Javier Gonzalez  wrote:
>> On 16 Jun 2018, at 21.38, Matias Bjørling  wrote:
>>
>> On 06/16/2018 12:27 AM, Igor Konopko wrote:
>>> Currently pblk and lightnvm does only check for size
>>> of OOB metadata and does not care wheather this meta
>>> is located in separate buffer or is interleaved with
>>> data in single buffer.
>>> In reality only the first scenario is supported, where
>>> second mode will break pblk functionality during any
>>> IO operation.
>>> The goal of this patch is to block creation of pblk
>>> devices in case of interleaved metadata
>>> Signed-off-by: Igor Konopko 
>>> ---
>>>  drivers/lightnvm/pblk-init.c | 6 ++
>>>  drivers/nvme/host/lightnvm.c | 1 +
>>>  include/linux/lightnvm.h | 1 +
>>>  3 files changed, 8 insertions(+)
>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>> index 5eb641da46ed..483a6d479e7d 100644
>>> --- a/drivers/lightnvm/pblk-init.c
>>> +++ b/drivers/lightnvm/pblk-init.c
>>> @@ -1238,6 +1238,12 @@ static void *pblk_init(struct nvm_tgt_dev *dev, 
>>> struct gendisk *tdisk,
>>>  return ERR_PTR(-EINVAL);
>>>  }
>>>  +   if (geo->ext) {
>>> +pr_err("pblk: extended (interleaved) metadata in data buffer"
>>> +" not supported\n");
>>> +return ERR_PTR(-EINVAL);
>>> +}
>>> +
>>>  pblk = kzalloc(sizeof(struct pblk), GFP_KERNEL);
>>>  if (!pblk)
>>>  return ERR_PTR(-ENOMEM);
>>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>>> index 670478abc754..872ab854ccf5 100644
>>> --- a/drivers/nvme/host/lightnvm.c
>>> +++ b/drivers/nvme/host/lightnvm.c
>>> @@ -979,6 +979,7 @@ void nvme_nvm_update_nvm_info(struct nvme_ns *ns)
>>>  geo->csecs = 1 << ns->lba_shift;
>>>  geo->sos = ns->ms;
>>> +geo->ext = ns->ext;
>>>  }
>>>int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
>>> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
>>> index 72a55d71917e..b13e64e2112f 100644
>>> --- a/include/linux/lightnvm.h
>>> +++ b/include/linux/lightnvm.h
>>> @@ -350,6 +350,7 @@ struct nvm_geo {
>>>  u32 clba;   /* sectors per chunk */
>>>  u16 csecs;  /* sector size */
>>>  u16 sos;/* out-of-band area size */
>>> +u16 ext;/* metadata in extended data buffer */
>>>  /* device write constrains */
>>>  u32 ws_min; /* minimum write size */
>>
>> I think bool type would be better here. Can it be placesd a bit down, just 
>> over the 1.2 stuff?
>>
>> Also, feel free to fix up the checkpatch stuff in patch 1 & 3 & 5.
>
> Apart from Matias' comments, it looks good to me.
>
> Traditionally, we have separated subsystem and target patches to make
> sure there is no coupling between pblk and lightnvm, but if Matias is ok
> with starting having patches covering all at once, then good for me too.
>

I often object when a patch can logically be split into two and should
be two distinct parts. In this case, it fits together.

Re: [PATCH 5/5] lightnvm: pblk: Disable interleaved metadata in pblk

[Igor Konopko]

On 18.06.2018 07:29, Javier Gonzalez wrote:

On 16 Jun 2018, at 21.38, Matias Bjørling  wrote:

On 06/16/2018 12:27 AM, Igor Konopko wrote:

Currently pblk and lightnvm does only check for size
of OOB metadata and does not care wheather this meta
is located in separate buffer or is interleaved with
data in single buffer.
In reality only the first scenario is supported, where
second mode will break pblk functionality during any
IO operation.
The goal of this patch is to block creation of pblk
devices in case of interleaved metadata
Signed-off-by: Igor Konopko 
---
  drivers/lightnvm/pblk-init.c | 6 ++
  drivers/nvme/host/lightnvm.c | 1 +
  include/linux/lightnvm.h | 1 +
  3 files changed, 8 insertions(+)
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 5eb641da46ed..483a6d479e7d 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -1238,6 +1238,12 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct 
gendisk *tdisk,
return ERR_PTR(-EINVAL);
}
  + if (geo->ext) {
+   pr_err("pblk: extended (interleaved) metadata in data buffer"
+   " not supported\n");
+   return ERR_PTR(-EINVAL);
+   }
+
pblk = kzalloc(sizeof(struct pblk), GFP_KERNEL);
if (!pblk)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 670478abc754..872ab854ccf5 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -979,6 +979,7 @@ void nvme_nvm_update_nvm_info(struct nvme_ns *ns)
geo->csecs = 1 << ns->lba_shift;
geo->sos = ns->ms;
+   geo->ext = ns->ext;
  }
int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 72a55d71917e..b13e64e2112f 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -350,6 +350,7 @@ struct nvm_geo {
u32 clba;   /* sectors per chunk */
u16 csecs;  /* sector size */
u16 sos;/* out-of-band area size */
+   u16 ext;/* metadata in extended data buffer */
/* device write constrains */
u32 ws_min; /* minimum write size */


I think bool type would be better here. Can it be placesd a bit down, just over 
the 1.2 stuff?

Also, feel free to fix up the checkpatch stuff in patch 1 & 3 & 5.


Apart from Matias' comments, it looks good to me.

Traditionally, we have separated subsystem and target patches to make
sure there is no coupling between pblk and lightnvm, but if Matias is ok
with starting having patches covering all at once, then good for me too.

Javier



Will fix above comments and resend.

Igor

Re: [PATCH 5/5] lightnvm: pblk: Disable interleaved metadata in pblk

[Javier Gonzalez]

> On 16 Jun 2018, at 21.38, Matias Bjørling  wrote:
> 
> On 06/16/2018 12:27 AM, Igor Konopko wrote:
>> Currently pblk and lightnvm does only check for size
>> of OOB metadata and does not care wheather this meta
>> is located in separate buffer or is interleaved with
>> data in single buffer.
>> In reality only the first scenario is supported, where
>> second mode will break pblk functionality during any
>> IO operation.
>> The goal of this patch is to block creation of pblk
>> devices in case of interleaved metadata
>> Signed-off-by: Igor Konopko 
>> ---
>>  drivers/lightnvm/pblk-init.c | 6 ++
>>  drivers/nvme/host/lightnvm.c | 1 +
>>  include/linux/lightnvm.h | 1 +
>>  3 files changed, 8 insertions(+)
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index 5eb641da46ed..483a6d479e7d 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -1238,6 +1238,12 @@ static void *pblk_init(struct nvm_tgt_dev *dev, 
>> struct gendisk *tdisk,
>>  return ERR_PTR(-EINVAL);
>>  }
>>  +   if (geo->ext) {
>> +pr_err("pblk: extended (interleaved) metadata in data buffer"
>> +" not supported\n");
>> +return ERR_PTR(-EINVAL);
>> +}
>> +
>>  pblk = kzalloc(sizeof(struct pblk), GFP_KERNEL);
>>  if (!pblk)
>>  return ERR_PTR(-ENOMEM);
>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>> index 670478abc754..872ab854ccf5 100644
>> --- a/drivers/nvme/host/lightnvm.c
>> +++ b/drivers/nvme/host/lightnvm.c
>> @@ -979,6 +979,7 @@ void nvme_nvm_update_nvm_info(struct nvme_ns *ns)
>>  geo->csecs = 1 << ns->lba_shift;
>>  geo->sos = ns->ms;
>> +geo->ext = ns->ext;
>>  }
>>int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
>> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
>> index 72a55d71917e..b13e64e2112f 100644
>> --- a/include/linux/lightnvm.h
>> +++ b/include/linux/lightnvm.h
>> @@ -350,6 +350,7 @@ struct nvm_geo {
>>  u32 clba;   /* sectors per chunk */
>>  u16 csecs;  /* sector size */
>>  u16 sos;/* out-of-band area size */
>> +u16 ext;/* metadata in extended data buffer */
>>  /* device write constrains */
>>  u32 ws_min; /* minimum write size */
> 
> I think bool type would be better here. Can it be placesd a bit down, just 
> over the 1.2 stuff?
> 
> Also, feel free to fix up the checkpatch stuff in patch 1 & 3 & 5.

Apart from Matias' comments, it looks good to me.

Traditionally, we have separated subsystem and target patches to make
sure there is no coupling between pblk and lightnvm, but if Matias is ok
with starting having patches covering all at once, then good for me too.

Javier



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 5/5] lightnvm: pblk: Disable interleaved metadata in pblk

[Matias Bjørling]

On 06/16/2018 12:27 AM, Igor Konopko wrote:

Currently pblk and lightnvm does only check for size
of OOB metadata and does not care wheather this meta
is located in separate buffer or is interleaved with
data in single buffer.

In reality only the first scenario is supported, where
second mode will break pblk functionality during any
IO operation.

The goal of this patch is to block creation of pblk
devices in case of interleaved metadata

Signed-off-by: Igor Konopko 
---
  drivers/lightnvm/pblk-init.c | 6 ++
  drivers/nvme/host/lightnvm.c | 1 +
  include/linux/lightnvm.h | 1 +
  3 files changed, 8 insertions(+)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 5eb641da46ed..483a6d479e7d 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -1238,6 +1238,12 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct 
gendisk *tdisk,
return ERR_PTR(-EINVAL);
}
  
+	if (geo->ext) {

+   pr_err("pblk: extended (interleaved) metadata in data buffer"
+   " not supported\n");
+   return ERR_PTR(-EINVAL);
+   }
+
pblk = kzalloc(sizeof(struct pblk), GFP_KERNEL);
if (!pblk)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 670478abc754..872ab854ccf5 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -979,6 +979,7 @@ void nvme_nvm_update_nvm_info(struct nvme_ns *ns)
  
  	geo->csecs = 1 << ns->lba_shift;

geo->sos = ns->ms;
+   geo->ext = ns->ext;
  }
  
  int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)

diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 72a55d71917e..b13e64e2112f 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -350,6 +350,7 @@ struct nvm_geo {
u32 clba;   /* sectors per chunk */
u16 csecs;  /* sector size */
u16 sos;/* out-of-band area size */
+   u16 ext;/* metadata in extended data buffer */
  
  	/* device write constrains */

u32 ws_min; /* minimum write size */



I think bool type would be better here. Can it be placesd a bit down, 
just over the 1.2 stuff?


Also, feel free to fix up the checkpatch stuff in patch 1 & 3 & 5.

[PATCH 5/5] lightnvm: pblk: Disable interleaved metadata in pblk

[Igor Konopko]

Currently pblk and lightnvm does only check for size
of OOB metadata and does not care wheather this meta
is located in separate buffer or is interleaved with
data in single buffer.

In reality only the first scenario is supported, where
second mode will break pblk functionality during any
IO operation.

The goal of this patch is to block creation of pblk
devices in case of interleaved metadata

Signed-off-by: Igor Konopko 
---
 drivers/lightnvm/pblk-init.c | 6 ++
 drivers/nvme/host/lightnvm.c | 1 +
 include/linux/lightnvm.h | 1 +
 3 files changed, 8 insertions(+)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 5eb641da46ed..483a6d479e7d 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -1238,6 +1238,12 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct 
gendisk *tdisk,
return ERR_PTR(-EINVAL);
}
 
+   if (geo->ext) {
+   pr_err("pblk: extended (interleaved) metadata in data buffer"
+   " not supported\n");
+   return ERR_PTR(-EINVAL);
+   }
+
pblk = kzalloc(sizeof(struct pblk), GFP_KERNEL);
if (!pblk)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 670478abc754..872ab854ccf5 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -979,6 +979,7 @@ void nvme_nvm_update_nvm_info(struct nvme_ns *ns)
 
geo->csecs = 1 << ns->lba_shift;
geo->sos = ns->ms;
+   geo->ext = ns->ext;
 }
 
 int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 72a55d71917e..b13e64e2112f 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -350,6 +350,7 @@ struct nvm_geo {
u32 clba;   /* sectors per chunk */
u16 csecs;  /* sector size */
u16 sos;/* out-of-band area size */
+   u16 ext;/* metadata in extended data buffer */
 
/* device write constrains */
u32 ws_min; /* minimum write size */
-- 
2.14.3