Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Javier González]

> On 6 Sep 2017, at 17.20, Jens Axboe  wrote:
> 
> On 09/06/2017 09:13 AM, Jens Axboe wrote:
>> On 09/06/2017 09:12 AM, Javier González wrote:
 On 6 Sep 2017, at 17.09, Jens Axboe  wrote:
 
 On 09/06/2017 09:08 AM, Johannes Thumshirn wrote:
> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
>> Check for failed mempool allocations and act accordingly.
> 
> Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
> "[...] Note that due to preallocation, this function *never* fails when 
> called
> from process contexts. (it might fail if called from an IRQ context.) 
> [...]"
 
 It's not needed, mempool() will never fail if __GFP_WAIT is set in the
 mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.
>>> 
>>> Thanks for the clarification. Do you just drop the patch, or do you want
>>> me to re-send the series?
>> 
>> No need to resend. I'll pick up the others in a day or two, once people
>> have had some time to go over them.
> 
> I took a quick look at your mempool usage, and I'm not sure it's
> correct.  For a mempool to work, you have to ensure that you provide a
> forward progress guarantee. With that guarantee, you know that if you do
> end up sleeping on allocation, you already have items inflight that will
> be freed when that operation completes. In other words, all allocations
> must have a defined and finite life time, as any allocation can
> potentially sleep/block for that life time. You can't allocate something
> and hold on to it forever, then you are violating the terms of agreement
> that makes a mempool work.

I understood the part of guaranteeing the number of inflight items to
keep the mempool active without waiting, but I must admit that I assumed
that the mempool would resize when getting pressure and that the penalty
would be increased latency, not the mempool giving up and causing a
deadlock.

> 
> The first one that caught my eye is pblk->page_pool. You have this loop:
> 
> for (i = 0; i < nr_pages; i++) {
>page = mempool_alloc(pblk->page_pool, flags);
>if (!page)
>goto err;
> 
>ret = bio_add_pc_page(q, bio, page, PBLK_EXPOSED_PAGE_SIZE, 0);
>if (ret != PBLK_EXPOSED_PAGE_SIZE) {
>pr_err("pblk: could not add page to bio\n");
>mempool_free(page, pblk->page_pool);
>goto err;
>}
> }
> 
> which looks suspect. This mempool is created with a reserve pool of
> PAGE_POOL_SIZE (16) members. Do we know if the bio has 16 pages or less?
> If not, then this is broken and can deadlock forever.

I can see that in this case, the 16 elements do not hold. In the read
path, we can guarantee that a read will be <= 64 sectors (4KB pages), so
this is definitely wrong. I'll fix it tomorrow.

Since we are at it, I have for some time wondered what's the right way
to balance the mempool sizes so that we are a good citizen and perform
at the same time. I don't see a lot of code using mempool_resize to tune
the min_nr based on load. For example, in our write path, the numbers
are easy to calculate, but on the read path I completely
over-dimensioned the mempool to ensure not having to wait for the
completion path. Any good rule of thumb here?

> You have a lot of mempool usage in the code, would probably not hurt to
> audit all of them.

Yes. I will take a look and add comments to the sizes.

Thanks Jens,
Javier


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Jens Axboe]

On 09/06/2017 09:13 AM, Jens Axboe wrote:
> On 09/06/2017 09:12 AM, Javier González wrote:
>>> On 6 Sep 2017, at 17.09, Jens Axboe  wrote:
>>>
>>> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote:
 On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
> Check for failed mempool allocations and act accordingly.

 Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
 "[...] Note that due to preallocation, this function *never* fails when 
 called
 from process contexts. (it might fail if called from an IRQ context.) 
 [...]"
>>>
>>> It's not needed, mempool() will never fail if __GFP_WAIT is set in the
>>> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.
>>
>> Thanks for the clarification. Do you just drop the patch, or do you want
>> me to re-send the series?
> 
> No need to resend. I'll pick up the others in a day or two, once people
> have had some time to go over them.

I took a quick look at your mempool usage, and I'm not sure it's
correct.  For a mempool to work, you have to ensure that you provide a
forward progress guarantee. With that guarantee, you know that if you do
end up sleeping on allocation, you already have items inflight that will
be freed when that operation completes. In other words, all allocations
must have a defined and finite life time, as any allocation can
potentially sleep/block for that life time. You can't allocate something
and hold on to it forever, then you are violating the terms of agreement
that makes a mempool work.

The first one that caught my eye is pblk->page_pool. You have this loop:

for (i = 0; i < nr_pages; i++) {
page = mempool_alloc(pblk->page_pool, flags);   
if (!page)  
goto err;   

ret = bio_add_pc_page(q, bio, page, PBLK_EXPOSED_PAGE_SIZE, 0); 
if (ret != PBLK_EXPOSED_PAGE_SIZE) {
pr_err("pblk: could not add page to bio\n");
mempool_free(page, pblk->page_pool);
goto err;   
}   
}  

which looks suspect. This mempool is created with a reserve pool of
PAGE_POOL_SIZE (16) members. Do we know if the bio has 16 pages or less?
If not, then this is broken and can deadlock forever.

You have a lot of mempool usage in the code, would probably not hurt to
audit all of them.

-- 
Jens Axboe

Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Jens Axboe]

On 09/06/2017 09:12 AM, Javier González wrote:
>> On 6 Sep 2017, at 17.09, Jens Axboe  wrote:
>>
>> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote:
>>> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
 Check for failed mempool allocations and act accordingly.
>>>
>>> Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
>>> "[...] Note that due to preallocation, this function *never* fails when 
>>> called
>>> from process contexts. (it might fail if called from an IRQ context.) [...]"
>>
>> It's not needed, mempool() will never fail if __GFP_WAIT is set in the
>> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.
> 
> Thanks for the clarification. Do you just drop the patch, or do you want
> me to re-send the series?

No need to resend. I'll pick up the others in a day or two, once people
have had some time to go over them.

-- 
Jens Axboe

Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Johannes Thumshirn]

On Wed, Sep 06, 2017 at 09:09:29AM -0600, Jens Axboe wrote:
> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote:
> > On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
> >> Check for failed mempool allocations and act accordingly.
> > 
> > Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
> > "[...] Note that due to preallocation, this function *never* fails when 
> > called
> > from process contexts. (it might fail if called from an IRQ context.) [...]"
> 
> It's not needed, mempool() will never fail if __GFP_WAIT is set in the
> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.

Exactly. Maybe I shouldn't have it phrased as a question though... 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Javier González]

> On 6 Sep 2017, at 17.09, Jens Axboe  wrote:
> 
> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote:
>> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
>>> Check for failed mempool allocations and act accordingly.
>> 
>> Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
>> "[...] Note that due to preallocation, this function *never* fails when 
>> called
>> from process contexts. (it might fail if called from an IRQ context.) [...]"
> 
> It's not needed, mempool() will never fail if __GFP_WAIT is set in the
> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.

Thanks for the clarification. Do you just drop the patch, or do you want
me to re-send the series?

I see that I do this other places, I'll clean it up for next window.

Thanks,
Javier


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Jens Axboe]

On 09/06/2017 09:08 AM, Johannes Thumshirn wrote:
> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
>> Check for failed mempool allocations and act accordingly.
> 
> Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
> "[...] Note that due to preallocation, this function *never* fails when called
> from process contexts. (it might fail if called from an IRQ context.) [...]"

It's not needed, mempool() will never fail if __GFP_WAIT is set in the
mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.

-- 
Jens Axboe

Re: [PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Johannes Thumshirn]

On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
> Check for failed mempool allocations and act accordingly.

Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
"[...] Note that due to preallocation, this function *never* fails when called
from process contexts. (it might fail if called from an IRQ context.) [...]"


Byte,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/6] lightnvm: pblk: check for failed mempool alloc.

[Javier González]

Check for failed mempool allocations and act accordingly.

Signed-off-by: Javier González 
Signed-off-by: Matias Bjørling 
---
 drivers/lightnvm/pblk-core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 81501644fb15..acb07bbcb416 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -165,6 +165,8 @@ struct nvm_rq *pblk_alloc_rqd(struct pblk *pblk, int rw)
}
 
rqd = mempool_alloc(pool, GFP_KERNEL);
+   if (!rqd)
+   return NULL;
memset(rqd, 0, rq_size);
 
return rqd;
@@ -1478,6 +1480,8 @@ int pblk_blk_erase_async(struct pblk *pblk, struct 
ppa_addr ppa)
int err;
 
rqd = mempool_alloc(pblk->g_rq_pool, GFP_KERNEL);
+   if (!rqd)
+   return -ENOMEM;
memset(rqd, 0, pblk_g_rq_size);
 
pblk_setup_e_rq(pblk, rqd, ppa);
-- 
2.7.4