Re: [PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Javier Gonzalez]

> On 19 Feb 2018, at 08.31, Matias Bjørling  wrote:
> 
> On 02/16/2018 07:48 AM, Javier Gonzalez wrote:
>>> On 15 Feb 2018, at 05.11, Matias Bjørling  wrote:
>>> 
>>> The value of max_phys_sect is always static. Instead of
>>> defining it in the nvm_dev_ops structure, declare it as a global
>>> value.
>>> 
>>> Signed-off-by: Matias Bjørling 
>>> ---
>>> drivers/lightnvm/core.c  | 28 +++-
>>> drivers/lightnvm/pblk-init.c |  9 -
>>> drivers/lightnvm/pblk-recovery.c |  8 ++--
>>> drivers/nvme/host/lightnvm.c |  5 +
>>> include/linux/lightnvm.h |  5 ++---
>>> 5 files changed, 16 insertions(+), 39 deletions(-)
>> The patch looks good, but I have a question. If a target implements the
>> scalar interface, then it will not be limited to 64 lbas/ppas and it
>> will not make sense to split the bio base don this value. In fact, it
>> looks like in time, we will move to a scalar interface in the 2.0 path
>> to align with the zoned interface, so this value will be dependent on
>> whether the target is using the scalar or vector interface.
> 
> Both read/write and vector interface will coexist. I am only removing
> what is hardwired into the specification.
> 
> The read/write interface has always been able issue more than 64 LBAs,
> it is instead limited by what the hardware reports its max transfer
> size to be.
> 

Exactly. I was thinking of a similar mechanism for the vector interface
to simplify integration with the scalar interface and avoid having an
if/else for what we now call max_phys_sect.

I guess we can wait and see what the code looks like when we adapt pblk.

Reviewed-by: Javier González 

Javier


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Javier Gonzalez]

> On 19 Feb 2018, at 08.31, Matias Bjørling  wrote:
> 
> On 02/16/2018 07:48 AM, Javier Gonzalez wrote:
>>> On 15 Feb 2018, at 05.11, Matias Bjørling  wrote:
>>> 
>>> The value of max_phys_sect is always static. Instead of
>>> defining it in the nvm_dev_ops structure, declare it as a global
>>> value.
>>> 
>>> Signed-off-by: Matias Bjørling 
>>> ---
>>> drivers/lightnvm/core.c  | 28 +++-
>>> drivers/lightnvm/pblk-init.c |  9 -
>>> drivers/lightnvm/pblk-recovery.c |  8 ++--
>>> drivers/nvme/host/lightnvm.c |  5 +
>>> include/linux/lightnvm.h |  5 ++---
>>> 5 files changed, 16 insertions(+), 39 deletions(-)
>> The patch looks good, but I have a question. If a target implements the
>> scalar interface, then it will not be limited to 64 lbas/ppas and it
>> will not make sense to split the bio base don this value. In fact, it
>> looks like in time, we will move to a scalar interface in the 2.0 path
>> to align with the zoned interface, so this value will be dependent on
>> whether the target is using the scalar or vector interface.
> 
> Both read/write and vector interface will coexist. I am only removing
> what is hardwired into the specification.
> 
> The read/write interface has always been able issue more than 64 LBAs,
> it is instead limited by what the hardware reports its max transfer
> size to be.
> 

Exactly. I was thinking of a similar mechanism for the vector interface
to simplify integration with the scalar interface and avoid having an
if/else for what we now call max_phys_sect.

I guess we can wait and see what the code looks like when we adapt pblk.

Reviewed-by: Javier González 

Javier


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Matias Bjørling]

On 02/16/2018 07:48 AM, Javier Gonzalez wrote:



On 15 Feb 2018, at 05.11, Matias Bjørling  wrote:

The value of max_phys_sect is always static. Instead of
defining it in the nvm_dev_ops structure, declare it as a global
value.

Signed-off-by: Matias Bjørling 
---
drivers/lightnvm/core.c  | 28 +++-
drivers/lightnvm/pblk-init.c |  9 -
drivers/lightnvm/pblk-recovery.c |  8 ++--
drivers/nvme/host/lightnvm.c |  5 +
include/linux/lightnvm.h |  5 ++---
5 files changed, 16 insertions(+), 39 deletions(-)



The patch looks good, but I have a question. If a target implements the
scalar interface, then it will not be limited to 64 lbas/ppas and it
will not make sense to split the bio base don this value. In fact, it
looks like in time, we will move to a scalar interface in the 2.0 path
to align with the zoned interface, so this value will be dependent on
whether the target is using the scalar or vector interface.



Both read/write and vector interface will coexist. I am only removing 
what is hardwired into the specification.


The read/write interface has always been able issue more than 64 LBAs, 
it is instead limited by what the hardware reports its max transfer size 
to be.

Re: [PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Matias Bjørling]

On 02/16/2018 07:48 AM, Javier Gonzalez wrote:



On 15 Feb 2018, at 05.11, Matias Bjørling  wrote:

The value of max_phys_sect is always static. Instead of
defining it in the nvm_dev_ops structure, declare it as a global
value.

Signed-off-by: Matias Bjørling 
---
drivers/lightnvm/core.c  | 28 +++-
drivers/lightnvm/pblk-init.c |  9 -
drivers/lightnvm/pblk-recovery.c |  8 ++--
drivers/nvme/host/lightnvm.c |  5 +
include/linux/lightnvm.h |  5 ++---
5 files changed, 16 insertions(+), 39 deletions(-)



The patch looks good, but I have a question. If a target implements the
scalar interface, then it will not be limited to 64 lbas/ppas and it
will not make sense to split the bio base don this value. In fact, it
looks like in time, we will move to a scalar interface in the 2.0 path
to align with the zoned interface, so this value will be dependent on
whether the target is using the scalar or vector interface.



Both read/write and vector interface will coexist. I am only removing 
what is hardwired into the specification.


The read/write interface has always been able issue more than 64 LBAs, 
it is instead limited by what the hardware reports its max transfer size 
to be.

Re: [PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Javier Gonzalez]

> On 15 Feb 2018, at 05.11, Matias Bjørling  wrote:
> 
> The value of max_phys_sect is always static. Instead of
> defining it in the nvm_dev_ops structure, declare it as a global
> value.
> 
> Signed-off-by: Matias Bjørling 
> ---
> drivers/lightnvm/core.c  | 28 +++-
> drivers/lightnvm/pblk-init.c |  9 -
> drivers/lightnvm/pblk-recovery.c |  8 ++--
> drivers/nvme/host/lightnvm.c |  5 +
> include/linux/lightnvm.h |  5 ++---
> 5 files changed, 16 insertions(+), 39 deletions(-)
> 

The patch looks good, but I have a question. If a target implements the
scalar interface, then it will not be limited to 64 lbas/ppas and it
will not make sense to split the bio base don this value. In fact, it
looks like in time, we will move to a scalar interface in the 2.0 path
to align with the zoned interface, so this value will be dependent on
whether the target is using the scalar or vector interface.

Javier


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Javier Gonzalez]

> On 15 Feb 2018, at 05.11, Matias Bjørling  wrote:
> 
> The value of max_phys_sect is always static. Instead of
> defining it in the nvm_dev_ops structure, declare it as a global
> value.
> 
> Signed-off-by: Matias Bjørling 
> ---
> drivers/lightnvm/core.c  | 28 +++-
> drivers/lightnvm/pblk-init.c |  9 -
> drivers/lightnvm/pblk-recovery.c |  8 ++--
> drivers/nvme/host/lightnvm.c |  5 +
> include/linux/lightnvm.h |  5 ++---
> 5 files changed, 16 insertions(+), 39 deletions(-)
> 

The patch looks good, but I have a question. If a target implements the
scalar interface, then it will not be limited to 64 lbas/ppas and it
will not make sense to split the bio base don this value. In fact, it
looks like in time, we will move to a scalar interface in the 2.0 path
to align with the zoned interface, so this value will be dependent on
whether the target is using the scalar or vector interface.

Javier


signature.asc
Description: Message signed with OpenPGP

[PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Matias Bjørling]

The value of max_phys_sect is always static. Instead of
defining it in the nvm_dev_ops structure, declare it as a global
value.

Signed-off-by: Matias Bjørling 
---
 drivers/lightnvm/core.c  | 28 +++-
 drivers/lightnvm/pblk-init.c |  9 -
 drivers/lightnvm/pblk-recovery.c |  8 ++--
 drivers/nvme/host/lightnvm.c |  5 +
 include/linux/lightnvm.h |  5 ++---
 5 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 511c81c319ee..782f381e4d61 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -407,7 +407,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
nvm_ioctl_create *create)
tdisk->private_data = targetdata;
tqueue->queuedata = targetdata;
 
-   blk_queue_max_hw_sectors(tqueue, 8 * dev->ops->max_phys_sect);
+   blk_queue_max_hw_sectors(tqueue,
+   (dev->geo.sec_size >> 9) * NVM_MAX_VLBA);
 
set_capacity(tdisk, tt->capacity(targetdata));
add_disk(tdisk);
@@ -719,7 +720,7 @@ int nvm_set_tgt_bb_tbl(struct nvm_tgt_dev *tgt_dev, struct 
ppa_addr *ppas,
struct nvm_rq rqd;
int ret;
 
-   if (nr_ppas > dev->ops->max_phys_sect) {
+   if (nr_ppas > NVM_MAX_VLBA) {
pr_err("nvm: unable to update all blocks atomically\n");
return -EINVAL;
}
@@ -740,14 +741,6 @@ int nvm_set_tgt_bb_tbl(struct nvm_tgt_dev *tgt_dev, struct 
ppa_addr *ppas,
 }
 EXPORT_SYMBOL(nvm_set_tgt_bb_tbl);
 
-int nvm_max_phys_sects(struct nvm_tgt_dev *tgt_dev)
-{
-   struct nvm_dev *dev = tgt_dev->parent;
-
-   return dev->ops->max_phys_sect;
-}
-EXPORT_SYMBOL(nvm_max_phys_sects);
-
 int nvm_submit_io(struct nvm_tgt_dev *tgt_dev, struct nvm_rq *rqd)
 {
struct nvm_dev *dev = tgt_dev->parent;
@@ -965,17 +958,10 @@ int nvm_register(struct nvm_dev *dev)
if (!dev->q || !dev->ops)
return -EINVAL;
 
-   if (dev->ops->max_phys_sect > 256) {
-   pr_info("nvm: max sectors supported is 256.\n");
-   return -EINVAL;
-   }
-
-   if (dev->ops->max_phys_sect > 1) {
-   dev->dma_pool = dev->ops->create_dma_pool(dev, "ppalist");
-   if (!dev->dma_pool) {
-   pr_err("nvm: could not create dma pool\n");
-   return -ENOMEM;
-   }
+   dev->dma_pool = dev->ops->create_dma_pool(dev, "ppalist");
+   if (!dev->dma_pool) {
+   pr_err("nvm: could not create dma pool\n");
+   return -ENOMEM;
}
 
ret = nvm_init(dev);
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 86a94a7faa96..5261702e9ff7 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -260,8 +260,7 @@ static int pblk_core_init(struct pblk *pblk)
return -ENOMEM;
 
/* Internal bios can be at most the sectors signaled by the device. */
-   pblk->page_bio_pool = mempool_create_page_pool(nvm_max_phys_sects(dev),
-   0);
+   pblk->page_bio_pool = mempool_create_page_pool(NVM_MAX_VLBA, 0);
if (!pblk->page_bio_pool)
goto free_global_caches;
 
@@ -716,12 +715,12 @@ static int pblk_lines_init(struct pblk *pblk)
 
pblk->min_write_pgs = geo->sec_per_pl * (geo->sec_size / PAGE_SIZE);
max_write_ppas = pblk->min_write_pgs * geo->all_luns;
-   pblk->max_write_pgs = (max_write_ppas < nvm_max_phys_sects(dev)) ?
-   max_write_ppas : nvm_max_phys_sects(dev);
+   pblk->max_write_pgs = min_t(int, max_write_ppas, NVM_MAX_VLBA);
pblk_set_sec_per_write(pblk, pblk->min_write_pgs);
 
if (pblk->max_write_pgs > PBLK_MAX_REQ_ADDRS) {
-   pr_err("pblk: cannot support device max_phys_sect\n");
+   pr_err("pblk: vector list too big(%u > %u)\n",
+   pblk->max_write_pgs, PBLK_MAX_REQ_ADDRS);
return -EINVAL;
}
 
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index e75a1af2eebe..aaab9a5c17cc 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -21,17 +21,15 @@ void pblk_submit_rec(struct work_struct *work)
struct pblk_rec_ctx *recovery =
container_of(work, struct pblk_rec_ctx, ws_rec);
struct pblk *pblk = recovery->pblk;
-   struct nvm_tgt_dev *dev = pblk->dev;
struct nvm_rq *rqd = recovery->rqd;
struct pblk_c_ctx *c_ctx = nvm_rq_to_pdu(rqd);
-   int max_secs = nvm_max_phys_sects(dev);
struct bio *bio;
unsigned int nr_rec_secs;
unsigned int pgs_read;
int ret;
 
nr_rec_secs = bitmap_weight((unsigned long int *)>ppa_status,
-   

[PATCH v2 5/6] lightnvm: remove nvm_dev_ops->max_phys_sect

[Matias Bjørling]

The value of max_phys_sect is always static. Instead of
defining it in the nvm_dev_ops structure, declare it as a global
value.

Signed-off-by: Matias Bjørling 
---
 drivers/lightnvm/core.c  | 28 +++-
 drivers/lightnvm/pblk-init.c |  9 -
 drivers/lightnvm/pblk-recovery.c |  8 ++--
 drivers/nvme/host/lightnvm.c |  5 +
 include/linux/lightnvm.h |  5 ++---
 5 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 511c81c319ee..782f381e4d61 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -407,7 +407,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct 
nvm_ioctl_create *create)
tdisk->private_data = targetdata;
tqueue->queuedata = targetdata;
 
-   blk_queue_max_hw_sectors(tqueue, 8 * dev->ops->max_phys_sect);
+   blk_queue_max_hw_sectors(tqueue,
+   (dev->geo.sec_size >> 9) * NVM_MAX_VLBA);
 
set_capacity(tdisk, tt->capacity(targetdata));
add_disk(tdisk);
@@ -719,7 +720,7 @@ int nvm_set_tgt_bb_tbl(struct nvm_tgt_dev *tgt_dev, struct 
ppa_addr *ppas,
struct nvm_rq rqd;
int ret;
 
-   if (nr_ppas > dev->ops->max_phys_sect) {
+   if (nr_ppas > NVM_MAX_VLBA) {
pr_err("nvm: unable to update all blocks atomically\n");
return -EINVAL;
}
@@ -740,14 +741,6 @@ int nvm_set_tgt_bb_tbl(struct nvm_tgt_dev *tgt_dev, struct 
ppa_addr *ppas,
 }
 EXPORT_SYMBOL(nvm_set_tgt_bb_tbl);
 
-int nvm_max_phys_sects(struct nvm_tgt_dev *tgt_dev)
-{
-   struct nvm_dev *dev = tgt_dev->parent;
-
-   return dev->ops->max_phys_sect;
-}
-EXPORT_SYMBOL(nvm_max_phys_sects);
-
 int nvm_submit_io(struct nvm_tgt_dev *tgt_dev, struct nvm_rq *rqd)
 {
struct nvm_dev *dev = tgt_dev->parent;
@@ -965,17 +958,10 @@ int nvm_register(struct nvm_dev *dev)
if (!dev->q || !dev->ops)
return -EINVAL;
 
-   if (dev->ops->max_phys_sect > 256) {
-   pr_info("nvm: max sectors supported is 256.\n");
-   return -EINVAL;
-   }
-
-   if (dev->ops->max_phys_sect > 1) {
-   dev->dma_pool = dev->ops->create_dma_pool(dev, "ppalist");
-   if (!dev->dma_pool) {
-   pr_err("nvm: could not create dma pool\n");
-   return -ENOMEM;
-   }
+   dev->dma_pool = dev->ops->create_dma_pool(dev, "ppalist");
+   if (!dev->dma_pool) {
+   pr_err("nvm: could not create dma pool\n");
+   return -ENOMEM;
}
 
ret = nvm_init(dev);
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 86a94a7faa96..5261702e9ff7 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -260,8 +260,7 @@ static int pblk_core_init(struct pblk *pblk)
return -ENOMEM;
 
/* Internal bios can be at most the sectors signaled by the device. */
-   pblk->page_bio_pool = mempool_create_page_pool(nvm_max_phys_sects(dev),
-   0);
+   pblk->page_bio_pool = mempool_create_page_pool(NVM_MAX_VLBA, 0);
if (!pblk->page_bio_pool)
goto free_global_caches;
 
@@ -716,12 +715,12 @@ static int pblk_lines_init(struct pblk *pblk)
 
pblk->min_write_pgs = geo->sec_per_pl * (geo->sec_size / PAGE_SIZE);
max_write_ppas = pblk->min_write_pgs * geo->all_luns;
-   pblk->max_write_pgs = (max_write_ppas < nvm_max_phys_sects(dev)) ?
-   max_write_ppas : nvm_max_phys_sects(dev);
+   pblk->max_write_pgs = min_t(int, max_write_ppas, NVM_MAX_VLBA);
pblk_set_sec_per_write(pblk, pblk->min_write_pgs);
 
if (pblk->max_write_pgs > PBLK_MAX_REQ_ADDRS) {
-   pr_err("pblk: cannot support device max_phys_sect\n");
+   pr_err("pblk: vector list too big(%u > %u)\n",
+   pblk->max_write_pgs, PBLK_MAX_REQ_ADDRS);
return -EINVAL;
}
 
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index e75a1af2eebe..aaab9a5c17cc 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -21,17 +21,15 @@ void pblk_submit_rec(struct work_struct *work)
struct pblk_rec_ctx *recovery =
container_of(work, struct pblk_rec_ctx, ws_rec);
struct pblk *pblk = recovery->pblk;
-   struct nvm_tgt_dev *dev = pblk->dev;
struct nvm_rq *rqd = recovery->rqd;
struct pblk_c_ctx *c_ctx = nvm_rq_to_pdu(rqd);
-   int max_secs = nvm_max_phys_sects(dev);
struct bio *bio;
unsigned int nr_rec_secs;
unsigned int pgs_read;
int ret;
 
nr_rec_secs = bitmap_weight((unsigned long int *)>ppa_status,
-   max_secs);
+