[PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Wenwei Tao]

rrpc->nr_sects is calculated after rrpc init luns succeeds,
before that the value of rrpc->nr_sects is zero, so we cannot
use it to calcuate rrpc area size, we use rrpc->nr_luns instead.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
 drivers/lightnvm/rrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
index 3ab6495..516a045 100644
--- a/drivers/lightnvm/rrpc.c
+++ b/drivers/lightnvm/rrpc.c
@@ -1223,7 +1223,7 @@ static int rrpc_area_init(struct rrpc *rrpc, sector_t *begin)
 {
 	struct nvm_dev *dev = rrpc->dev;
 	struct nvmm_type *mt = dev->mt;
-	sector_t size = rrpc->nr_sects * dev->sec_size;
+	sector_t size = dev->sec_size * dev->sec_per_lun * rrpc->nr_luns;
 
 	size >>= 9;
 
-- 
2.7.2.333.g70bd996

[PATCH 2/2] lightnvm: use relative logical address in rrpc_l2p_update

[Wenwei Tao]

Since every target now has their own logical address area,
we should deal trans_map and rev_trans_map with relative
logical address instead of the global one.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
 drivers/lightnvm/rrpc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
index 516a045..2279bd7 100644
--- a/drivers/lightnvm/rrpc.c
+++ b/drivers/lightnvm/rrpc.c
@@ -998,10 +998,14 @@ static int rrpc_l2p_update(u64 slba, u32 nlb, __le64 *entries, void *private)
 {
 	struct rrpc *rrpc = (struct rrpc *)private;
 	struct nvm_dev *dev = rrpc->dev;
-	struct rrpc_addr *addr = rrpc->trans_map + slba;
-	struct rrpc_rev_addr *raddr = rrpc->rev_trans_map;
-	u64 elba = slba + nlb;
-	u64 i;
+	struct rrpc_addr *addr;
+	struct rrpc_rev_addr *raddr;
+	u64 elba, i;
+
+	slba -= rrpc->soffset >> (ilog2(dev->sec_size) - 9);
+	addr = rrpc->trans_map + slba;
+	raddr = rrpc->rev_trans_map;
+	elba = slba + nlb;
 
 	if (unlikely(elba > dev->total_secs)) {
 		pr_err("nvm: L2P data from device is out of bounds!\n");
-- 
2.7.2.333.g70bd996

Re: [PATCH 2/2] lightnvm: use relative logical address in rrpc_l2p_update

[Matias Bjørling]

On 03/30/2016 04:28 PM, Wenwei Tao wrote:
> Since every target now has their own logical address area,
> we should deal trans_map and rev_trans_map with relative
> logical address instead of the global one.
>
> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
> ---
>   drivers/lightnvm/rrpc.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
> index 516a045..2279bd7 100644
> --- a/drivers/lightnvm/rrpc.c
> +++ b/drivers/lightnvm/rrpc.c
> @@ -998,10 +998,14 @@ static int rrpc_l2p_update(u64 slba, u32 nlb, __le64 *entries, void *private)
>   {
>   	struct rrpc *rrpc = (struct rrpc *)private;
>   	struct nvm_dev *dev = rrpc->dev;
> -	struct rrpc_addr *addr = rrpc->trans_map + slba;
> -	struct rrpc_rev_addr *raddr = rrpc->rev_trans_map;
> -	u64 elba = slba + nlb;
> -	u64 i;
> +	struct rrpc_addr *addr;
> +	struct rrpc_rev_addr *raddr;
> +	u64 elba, i;
> +
> +	slba -= rrpc->soffset >> (ilog2(dev->sec_size) - 9);
> +	addr = rrpc->trans_map + slba;
> +	raddr = rrpc->rev_trans_map;
> +	elba = slba + nlb;
>
>   	if (unlikely(elba > dev->total_secs)) {
>   		pr_err("nvm: L2P data from device is out of bounds!\n");
>

Hi Wenwei,

How about storing the value of "rrpc->soffset >> (ilog2(dev->sec_size) - 
9)" in rrpc->soffset. That let us only do the shifting for get/put area 
calls.

Re: [PATCH 2/2] lightnvm: use relative logical address in rrpc_l2p_update

[Wenwei Tao]

Okay, I'll send a patch to cover that change.

2016-03-31 17:11 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 03/30/2016 04:28 PM, Wenwei Tao wrote:
>>
>> Since every target now has their own logical address area,
>> we should deal trans_map and rev_trans_map with relative
>> logical address instead of the global one.
>>
>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>> ---
>>   drivers/lightnvm/rrpc.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
>> index 516a045..2279bd7 100644
>> --- a/drivers/lightnvm/rrpc.c
>> +++ b/drivers/lightnvm/rrpc.c
>> @@ -998,10 +998,14 @@ static int rrpc_l2p_update(u64 slba, u32 nlb, __le64
>> *entries, void *private)
>>   {
>>         struct rrpc *rrpc = (struct rrpc *)private;
>>         struct nvm_dev *dev = rrpc->dev;
>> -       struct rrpc_addr *addr = rrpc->trans_map + slba;
>> -       struct rrpc_rev_addr *raddr = rrpc->rev_trans_map;
>> -       u64 elba = slba + nlb;
>> -       u64 i;
>> +       struct rrpc_addr *addr;
>> +       struct rrpc_rev_addr *raddr;
>> +       u64 elba, i;
>> +
>> +       slba -= rrpc->soffset >> (ilog2(dev->sec_size) - 9);
>> +       addr = rrpc->trans_map + slba;
>> +       raddr = rrpc->rev_trans_map;
>> +       elba = slba + nlb;
>>
>>         if (unlikely(elba > dev->total_secs)) {
>>                 pr_err("nvm: L2P data from device is out of bounds!\n");
>>
>
> Hi Wenwei,
>
> How about storing the value of "rrpc->soffset >> (ilog2(dev->sec_size) - 9)"
> in rrpc->soffset. That let us only do the shifting for get/put area calls.

Re: [PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Wenwei Tao]

2016-03-30 22:28 GMT+08:00 Wenwei Tao <ww.tao0320@gmail.com>:
> rrpc->nr_sects is calculated after rrpc init luns succeeds,
> before that the value of rrpc->nr_sects is zero, so we cannot
> use it to calcuate rrpc area size, we use rrpc->nr_luns instead.
>
> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
> ---
>  drivers/lightnvm/rrpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
> index 3ab6495..516a045 100644
> --- a/drivers/lightnvm/rrpc.c
> +++ b/drivers/lightnvm/rrpc.c
> @@ -1223,7 +1223,7 @@ static int rrpc_area_init(struct rrpc *rrpc, sector_t *begin)
>  {
>         struct nvm_dev *dev = rrpc->dev;
>         struct nvmm_type *mt = dev->mt;
> -       sector_t size = rrpc->nr_sects * dev->sec_size;
> +       sector_t size = dev->sec_size * dev->sec_per_lun * rrpc->nr_luns;

dev->sec_size * dev->sec_per_lun * rrpc->nr_luns could be oveflow,
should use (sector_t)dev->sec_size * dev->sec_per_lun * rrpc->nr_luns
instead. Will submit another patch to fix it.
>
>         size >>= 9;
>
> --
> 2.7.2.333.g70bd996
>

Re: [PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Matias Bjørling]

On 03/31/2016 10:31 AM, Wenwei Tao wrote:
> 2016-03-30 22:28 GMT+08:00 Wenwei Tao <ww.tao0320@gmail.com>:
>> rrpc->nr_sects is calculated after rrpc init luns succeeds,
>> before that the value of rrpc->nr_sects is zero, so we cannot
>> use it to calcuate rrpc area size, we use rrpc->nr_luns instead.
>>
>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>> ---
>>   drivers/lightnvm/rrpc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
>> index 3ab6495..516a045 100644
>> --- a/drivers/lightnvm/rrpc.c
>> +++ b/drivers/lightnvm/rrpc.c
>> @@ -1223,7 +1223,7 @@ static int rrpc_area_init(struct rrpc *rrpc, sector_t *begin)
>>   {
>>          struct nvm_dev *dev = rrpc->dev;
>>          struct nvmm_type *mt = dev->mt;
>> -       sector_t size = rrpc->nr_sects * dev->sec_size;
>> +       sector_t size = dev->sec_size * dev->sec_per_lun * rrpc->nr_luns;
>
> dev->sec_size * dev->sec_per_lun * rrpc->nr_luns could be oveflow,
> should use (sector_t)dev->sec_size * dev->sec_per_lun * rrpc->nr_luns
> instead. Will submit another patch to fix it.

Hi Wenwei,

How about moving rrpc_area_init call under the rrpc_luns_init call 
instead. Then nr_sects will have been initialized?

>>
>>          size >>= 9;
>>
>> --
>> 2.7.2.333.g70bd996
>>

Re: [PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Wenwei Tao]

This could be work, but it needs more steps when rrpc_area_init fails.
If rrpc_area_init fail we may come to rrpc_free and call
rrpc_area_free, we find and put the area by the rrpc->soffset value,
this value is zero when we fail to get an area, we may put an exist
area that really start from zero by mistake. If we move rrpc_area_init
call under the rrpc_luns_init call instead, we need a way to avoid it.

2016-03-31 16:57 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>
>
> On 03/31/2016 10:31 AM, Wenwei Tao wrote:
>>
>> 2016-03-30 22:28 GMT+08:00 Wenwei Tao <ww.tao0320@gmail.com>:
>>>
>>> rrpc->nr_sects is calculated after rrpc init luns succeeds,
>>> before that the value of rrpc->nr_sects is zero, so we cannot
>>> use it to calcuate rrpc area size, we use rrpc->nr_luns instead.
>>>
>>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>>> ---
>>>   drivers/lightnvm/rrpc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
>>> index 3ab6495..516a045 100644
>>> --- a/drivers/lightnvm/rrpc.c
>>> +++ b/drivers/lightnvm/rrpc.c
>>> @@ -1223,7 +1223,7 @@ static int rrpc_area_init(struct rrpc *rrpc,
>>> sector_t *begin)
>>>   {
>>>          struct nvm_dev *dev = rrpc->dev;
>>>          struct nvmm_type *mt = dev->mt;
>>> -       sector_t size = rrpc->nr_sects * dev->sec_size;
>>> +       sector_t size = dev->sec_size * dev->sec_per_lun * rrpc->nr_luns;
>>
>>
>> dev->sec_size * dev->sec_per_lun * rrpc->nr_luns could be oveflow,
>> should use (sector_t)dev->sec_size * dev->sec_per_lun * rrpc->nr_luns
>> instead. Will submit another patch to fix it.
>
>
> Hi Wenwei,
>
> How about moving rrpc_area_init call under the rrpc_luns_init call instead.
> Then nr_sects will have been initialized?
>
>
>>>
>>>          size >>= 9;
>>>
>>> --
>>> 2.7.2.333.g70bd996
>>>
>

Re: [PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Matias Bjørling]

On 03/31/2016 11:51 AM, Wenwei Tao wrote:
> This could be work, but it needs more steps when rrpc_area_init fails.
> If rrpc_area_init fail we may come to rrpc_free and call
> rrpc_area_free, we find and put the area by the rrpc->soffset value,
> this value is zero when we fail to get an area, we may put an exist
> area that really start from zero by mistake. If we move rrpc_area_init
> call under the rrpc_luns_init call instead, we need a way to avoid it.

Fair enough. How about this:

diff --git i/drivers/lightnvm/rrpc.c w/drivers/lightnvm/rrpc.c
index 3ab6495..05a0698 100644
--- i/drivers/lightnvm/rrpc.c
+++ w/drivers/lightnvm/rrpc.c
@@ -1207,10 +1207,6 @@ static int rrpc_luns_init(struct rrpc *rrpc, int 
lun_begin, int lun_end)

                 INIT_WORK(&rlun->ws_gc, rrpc_lun_gc);
                 spin_lock_init(&rlun->lock);
-
-               rrpc->total_blocks += dev->blks_per_lun;
-               rrpc->nr_sects += dev->sec_per_lun;
-
         }

         return 0;
@@ -1388,6 +1384,8 @@ static void *rrpc_init(struct nvm_dev *dev, struct 
gendisk *tdisk,
         INIT_WORK(&rrpc->ws_requeue, rrpc_requeue);

         rrpc->nr_luns = lun_end - lun_begin + 1;
+       rrpc->total_blocks = dev->blks_per_lun * rrpc->nr_luns;
+       rrpc->nr_sects = dev->sec_per_lun * rrpc->nr_luns;

         /* simple round-robin strategy */
         atomic_set(&rrpc->next_lun, -1);

That nr_sects is initialized and we can use it in rrpc_area_init without 
moving the initialization order?

Re: [PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Wenwei Tao]

This is okay with me.
By the way, why don't you like  (sector_t)dev->sec_size *
dev->sec_per_lun * rrpc->nr_luns, the change seems smaller?

2016-03-31 18:07 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 03/31/2016 11:51 AM, Wenwei Tao wrote:
>>
>> This could be work, but it needs more steps when rrpc_area_init fails.
>> If rrpc_area_init fail we may come to rrpc_free and call
>> rrpc_area_free, we find and put the area by the rrpc->soffset value,
>> this value is zero when we fail to get an area, we may put an exist
>> area that really start from zero by mistake. If we move rrpc_area_init
>> call under the rrpc_luns_init call instead, we need a way to avoid it.
>
>
> Fair enough. How about this:
>
> diff --git i/drivers/lightnvm/rrpc.c w/drivers/lightnvm/rrpc.c
> index 3ab6495..05a0698 100644
> --- i/drivers/lightnvm/rrpc.c
> +++ w/drivers/lightnvm/rrpc.c
> @@ -1207,10 +1207,6 @@ static int rrpc_luns_init(struct rrpc *rrpc, int
> lun_begin, int lun_end)
>
>                 INIT_WORK(&rlun->ws_gc, rrpc_lun_gc);
>                 spin_lock_init(&rlun->lock);
> -
> -               rrpc->total_blocks += dev->blks_per_lun;
> -               rrpc->nr_sects += dev->sec_per_lun;
> -
>         }
>
>         return 0;
> @@ -1388,6 +1384,8 @@ static void *rrpc_init(struct nvm_dev *dev, struct
> gendisk *tdisk,
>         INIT_WORK(&rrpc->ws_requeue, rrpc_requeue);
>
>         rrpc->nr_luns = lun_end - lun_begin + 1;
> +       rrpc->total_blocks = dev->blks_per_lun * rrpc->nr_luns;
> +       rrpc->nr_sects = dev->sec_per_lun * rrpc->nr_luns;
>
>         /* simple round-robin strategy */
>         atomic_set(&rrpc->next_lun, -1);
>
> That nr_sects is initialized and we can use it in rrpc_area_init without
> moving the initialization order?

Re: [PATCH 1/2] lightnvm: use rrpc->nr_luns to calculate the rrpc area size

[Matias Bjørling]

On 03/31/2016 12:26 PM, Wenwei Tao wrote:
> This is okay with me.
> By the way, why don't you like  (sector_t)dev->sec_size *
> dev->sec_per_lun * rrpc->nr_luns, the change seems smaller?

Both works. I wanted to fix the underlying issue (which was rrpc values 
wasn't initialized enough to be used). Back when luns could be of 
different sizes, it made sense. Now that we can have the values up 
front, it makes sense to use them. :)