[Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

mempool_alloc() does not support __GFP_ZERO since elements may come from
memory that has already been released by mempool_free().

Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
initialize it to 0.

Cc: Lars Ellenberg <drbd-dev@lists.linbit.com>
Cc: Jens Axboe <axboe@fb.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/block/drbd/drbd_req.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c
--- a/drivers/block/drbd/drbd_req.c
+++ b/drivers/block/drbd/drbd_req.c
@@ -52,9 +52,10 @@ static struct drbd_request *drbd_req_new(struct drbd_device *device,
 {
 	struct drbd_request *req;
 
-	req = mempool_alloc(drbd_request_mempool, GFP_NOIO | __GFP_ZERO);
+	req = mempool_alloc(drbd_request_mempool, GFP_NOIO);
 	if (!req)
 		return NULL;
+	memset(req, 0, sizeof(*req));
 
 	drbd_req_make_private_bio(req, bio_src);
 	req->rq_state    = bio_data_dir(bio_src) == WRITE ? RQ_WRITE : 0;

[patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

mempool_alloc() does not support __GFP_ZERO since elements may come from
memory that has already been released by mempool_free().

Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
initialize it to 0.

Cc: Lars Ellenberg <drbd-dev@lists.linbit.com>
Cc: Jens Axboe <axboe@fb.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/block/drbd/drbd_req.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/drbd/drbd_req.c b/drivers/block/drbd/drbd_req.c
--- a/drivers/block/drbd/drbd_req.c
+++ b/drivers/block/drbd/drbd_req.c
@@ -52,9 +52,10 @@ static struct drbd_request *drbd_req_new(struct drbd_device *device,
 {
 	struct drbd_request *req;
 
-	req = mempool_alloc(drbd_request_mempool, GFP_NOIO | __GFP_ZERO);
+	req = mempool_alloc(drbd_request_mempool, GFP_NOIO);
 	if (!req)
 		return NULL;
+	memset(req, 0, sizeof(*req));
 
 	drbd_req_make_private_bio(req, bio_src);
 	req->rq_state    = bio_data_dir(bio_src) == WRITE ? RQ_WRITE : 0;

[Drbd-dev] [patch 2/2] block, drbd: use mempool_create_slab_pool()

[David Rientjes]

Mempools created for slab caches should use
mempool_create_slab_pool().

Cc: Lars Ellenberg <drbd-dev@lists.linbit.com>
Cc: Jens Axboe <axboe@fb.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/block/drbd/drbd_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -2107,13 +2107,12 @@ static int drbd_create_mempools(void)
 	if (drbd_md_io_page_pool == NULL)
 		goto Enomem;
 
-	drbd_request_mempool = mempool_create(number,
-		mempool_alloc_slab, mempool_free_slab, drbd_request_cache);
+	drbd_request_mempool = mempool_create_slab_pool(number,
+		drbd_request_cache);
 	if (drbd_request_mempool == NULL)
 		goto Enomem;
 
-	drbd_ee_mempool = mempool_create(number,
-		mempool_alloc_slab, mempool_free_slab, drbd_ee_cache);
+	drbd_ee_mempool = mempool_create_slab_pool(number, drbd_ee_cache);
 	if (drbd_ee_mempool == NULL)
 		goto Enomem;
 

[patch 2/2] block, drbd: use mempool_create_slab_pool()

[David Rientjes]

Mempools created for slab caches should use
mempool_create_slab_pool().

Cc: Lars Ellenberg <drbd-dev@lists.linbit.com>
Cc: Jens Axboe <axboe@fb.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/block/drbd/drbd_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -2107,13 +2107,12 @@ static int drbd_create_mempools(void)
 	if (drbd_md_io_page_pool == NULL)
 		goto Enomem;
 
-	drbd_request_mempool = mempool_create(number,
-		mempool_alloc_slab, mempool_free_slab, drbd_request_cache);
+	drbd_request_mempool = mempool_create_slab_pool(number,
+		drbd_request_cache);
 	if (drbd_request_mempool == NULL)
 		goto Enomem;
 
-	drbd_ee_mempool = mempool_create(number,
-		mempool_alloc_slab, mempool_free_slab, drbd_ee_cache);
+	drbd_ee_mempool = mempool_create_slab_pool(number, drbd_ee_cache);
 	if (drbd_ee_mempool == NULL)
 		goto Enomem;
 

Re: [Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Jens Axboe]

On 03/07/2015 05:24 PM, David Rientjes wrote:
> mempool_alloc() does not support __GFP_ZERO since elements may come from
> memory that has already been released by mempool_free().
>
> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> initialize it to 0.

You should add it to mempool instead, avoid having this issue show up 
for other folks as well. It'd be trivial to do. Normal ->alloc() should 
honor __GFP_ZERO, just do the same manually for removing an item from 
the internal pool.

-- 
Jens Axboe

Re: [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Jens Axboe]

On 03/07/2015 05:24 PM, David Rientjes wrote:
> mempool_alloc() does not support __GFP_ZERO since elements may come from
> memory that has already been released by mempool_free().
>
> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> initialize it to 0.

You should add it to mempool instead, avoid having this issue show up 
for other folks as well. It'd be trivial to do. Normal ->alloc() should 
honor __GFP_ZERO, just do the same manually for removing an item from 
the internal pool.

-- 
Jens Axboe

Re: [Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Sat, 7 Mar 2015, Jens Axboe wrote:

> > mempool_alloc() does not support __GFP_ZERO since elements may come from
> > memory that has already been released by mempool_free().
> > 
> > Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> > initialize it to 0.
> 
> You should add it to mempool instead, avoid having this issue show up for
> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
> __GFP_ZERO, just do the same manually for removing an item from the internal
> pool.
> 

Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
don't know the element size, in other words it wouldn't know the length to 
memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
element size since elements are allocated by the implementation of 
mempool_alloc_t and they could easily become inconsistent.  This patch is 
what you want to merge, really.

Re: [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Sat, 7 Mar 2015, Jens Axboe wrote:

> > mempool_alloc() does not support __GFP_ZERO since elements may come from
> > memory that has already been released by mempool_free().
> > 
> > Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> > initialize it to 0.
> 
> You should add it to mempool instead, avoid having this issue show up for
> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
> __GFP_ZERO, just do the same manually for removing an item from the internal
> pool.
> 

Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
don't know the element size, in other words it wouldn't know the length to 
memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
element size since elements are allocated by the implementation of 
mempool_alloc_t and they could easily become inconsistent.  This patch is 
what you want to merge, really.

Re: [Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Jens Axboe]

On 03/07/2015 05:53 PM, David Rientjes wrote:
> On Sat, 7 Mar 2015, Jens Axboe wrote:
> 
>>> mempool_alloc() does not support __GFP_ZERO since elements may come from
>>> memory that has already been released by mempool_free().
>>>
>>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
>>> initialize it to 0.
>>
>> You should add it to mempool instead, avoid having this issue show up for
>> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
>> __GFP_ZERO, just do the same manually for removing an item from the internal
>> pool.
>>
> 
> Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 

Uhm, it would make sense, though.

> don't know the element size, in other words it wouldn't know the length to 
> memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
> element size since elements are allocated by the implementation of 
> mempool_alloc_t and they could easily become inconsistent.  This patch is 
> what you want to merge, really.
> 

I forgot we don't have the size in there. Then I would suggest adding a
WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.

-- 
Jens Axboe

Re: [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Jens Axboe]

On 03/07/2015 05:53 PM, David Rientjes wrote:
> On Sat, 7 Mar 2015, Jens Axboe wrote:
> 
>>> mempool_alloc() does not support __GFP_ZERO since elements may come from
>>> memory that has already been released by mempool_free().
>>>
>>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
>>> initialize it to 0.
>>
>> You should add it to mempool instead, avoid having this issue show up for
>> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
>> __GFP_ZERO, just do the same manually for removing an item from the internal
>> pool.
>>
> 
> Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 

Uhm, it would make sense, though.

> don't know the element size, in other words it wouldn't know the length to 
> memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
> element size since elements are allocated by the implementation of 
> mempool_alloc_t and they could easily become inconsistent.  This patch is 
> what you want to merge, really.
> 

I forgot we don't have the size in there. Then I would suggest adding a
WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.

-- 
Jens Axboe

Re: [Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Sat, 7 Mar 2015, Jens Axboe wrote:

> >>> mempool_alloc() does not support __GFP_ZERO since elements may come from
> >>> memory that has already been released by mempool_free().
> >>>
> >>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> >>> initialize it to 0.
> >>
> >> You should add it to mempool instead, avoid having this issue show up for
> >> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
> >> __GFP_ZERO, just do the same manually for removing an item from the internal
> >> pool.
> >>
> > 
> > Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
> 
> Uhm, it would make sense, though.
> 

Disagree, I don't think we should extend mempool to know the element size, 
modify every user of mempool to pass it in, and keep it consistent with 
mempool_alloc_t for the benefit of __GFP_ZERO for this one buggy caller.  
Most users don't need __GFP_ZERO and just overwrite the entire element 
after mempool_alloc() and it would be an unnecessary overhead to even 
check for the bit set.  So it wouldn't make sense in terms of performance 
or maintainability.

> > don't know the element size, in other words it wouldn't know the length to 
> > memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
> > element size since elements are allocated by the implementation of 
> > mempool_alloc_t and they could easily become inconsistent.  This patch is 
> > what you want to merge, really.
> > 
> 
> I forgot we don't have the size in there. Then I would suggest adding a
> WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.
> 

There is, it's a VM_WARN_ON_ONCE() that will show up if you configure 
CONFIG_DEBUG_VM.

Re: [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Sat, 7 Mar 2015, Jens Axboe wrote:

> >>> mempool_alloc() does not support __GFP_ZERO since elements may come from
> >>> memory that has already been released by mempool_free().
> >>>
> >>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> >>> initialize it to 0.
> >>
> >> You should add it to mempool instead, avoid having this issue show up for
> >> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
> >> __GFP_ZERO, just do the same manually for removing an item from the internal
> >> pool.
> >>
> > 
> > Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
> 
> Uhm, it would make sense, though.
> 

Disagree, I don't think we should extend mempool to know the element size, 
modify every user of mempool to pass it in, and keep it consistent with 
mempool_alloc_t for the benefit of __GFP_ZERO for this one buggy caller.  
Most users don't need __GFP_ZERO and just overwrite the entire element 
after mempool_alloc() and it would be an unnecessary overhead to even 
check for the bit set.  So it wouldn't make sense in terms of performance 
or maintainability.

> > don't know the element size, in other words it wouldn't know the length to 
> > memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
> > element size since elements are allocated by the implementation of 
> > mempool_alloc_t and they could easily become inconsistent.  This patch is 
> > what you want to merge, really.
> > 
> 
> I forgot we don't have the size in there. Then I would suggest adding a
> WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.
> 

There is, it's a VM_WARN_ON_ONCE() that will show up if you configure 
CONFIG_DEBUG_VM.

Re: [Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Jens Axboe]

On 03/07/2015 06:27 PM, David Rientjes wrote:
> On Sat, 7 Mar 2015, Jens Axboe wrote:
> 
>>>>> mempool_alloc() does not support __GFP_ZERO since elements may come from
>>>>> memory that has already been released by mempool_free().
>>>>>
>>>>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
>>>>> initialize it to 0.
>>>>
>>>> You should add it to mempool instead, avoid having this issue show up for
>>>> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
>>>> __GFP_ZERO, just do the same manually for removing an item from the internal
>>>> pool.
>>>>
>>>
>>> Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
>>
>> Uhm, it would make sense, though.
>>
> 
> Disagree, I don't think we should extend mempool to know the element size, 
> modify every user of mempool to pass it in, and keep it consistent with 
> mempool_alloc_t for the benefit of __GFP_ZERO for this one buggy caller.  
> Most users don't need __GFP_ZERO and just overwrite the entire element 
> after mempool_alloc() and it would be an unnecessary overhead to even 
> check for the bit set.  So it wouldn't make sense in terms of performance 
> or maintainability.

My point is that conceptually, of course it makes sense to do and it
_should_ do it. We don't have the size, too bad, I don't disagree that
adding it just for this is necessarily the best idea.

>>> don't know the element size, in other words it wouldn't know the length to 
>>> memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
>>> element size since elements are allocated by the implementation of 
>>> mempool_alloc_t and they could easily become inconsistent.  This patch is 
>>> what you want to merge, really.
>>>
>>
>> I forgot we don't have the size in there. Then I would suggest adding a
>> WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.
>>
> 
> There is, it's a VM_WARN_ON_ONCE() that will show up if you configure 
> CONFIG_DEBUG_VM.

OK, that's good enough then.

-- 
Jens Axboe

Re: [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Jens Axboe]

On 03/07/2015 06:27 PM, David Rientjes wrote:
> On Sat, 7 Mar 2015, Jens Axboe wrote:
> 
>>>>> mempool_alloc() does not support __GFP_ZERO since elements may come from
>>>>> memory that has already been released by mempool_free().
>>>>>
>>>>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
>>>>> initialize it to 0.
>>>>
>>>> You should add it to mempool instead, avoid having this issue show up for
>>>> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
>>>> __GFP_ZERO, just do the same manually for removing an item from the internal
>>>> pool.
>>>>
>>>
>>> Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
>>
>> Uhm, it would make sense, though.
>>
> 
> Disagree, I don't think we should extend mempool to know the element size, 
> modify every user of mempool to pass it in, and keep it consistent with 
> mempool_alloc_t for the benefit of __GFP_ZERO for this one buggy caller.  
> Most users don't need __GFP_ZERO and just overwrite the entire element 
> after mempool_alloc() and it would be an unnecessary overhead to even 
> check for the bit set.  So it wouldn't make sense in terms of performance 
> or maintainability.

My point is that conceptually, of course it makes sense to do and it
_should_ do it. We don't have the size, too bad, I don't disagree that
adding it just for this is necessarily the best idea.

>>> don't know the element size, in other words it wouldn't know the length to 
>>> memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
>>> element size since elements are allocated by the implementation of 
>>> mempool_alloc_t and they could easily become inconsistent.  This patch is 
>>> what you want to merge, really.
>>>
>>
>> I forgot we don't have the size in there. Then I would suggest adding a
>> WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.
>>
> 
> There is, it's a VM_WARN_ON_ONCE() that will show up if you configure 
> CONFIG_DEBUG_VM.

OK, that's good enough then.

-- 
Jens Axboe

Re: [Drbd-dev] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Sat, 7 Mar 2015, David Rientjes wrote:

> > >>> mempool_alloc() does not support __GFP_ZERO since elements may come from
> > >>> memory that has already been released by mempool_free().
> > >>>
> > >>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> > >>> initialize it to 0.
> > >>
> > >> You should add it to mempool instead, avoid having this issue show up for
> > >> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
> > >> __GFP_ZERO, just do the same manually for removing an item from the internal
> > >> pool.
> > >>
> > > 
> > > Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
> > 
> > Uhm, it would make sense, though.
> > 
> 
> Disagree, I don't think we should extend mempool to know the element size, 
> modify every user of mempool to pass it in, and keep it consistent with 
> mempool_alloc_t for the benefit of __GFP_ZERO for this one buggy caller.  
> Most users don't need __GFP_ZERO and just overwrite the entire element 
> after mempool_alloc() and it would be an unnecessary overhead to even 
> check for the bit set.  So it wouldn't make sense in terms of performance 
> or maintainability.
> 
> > > don't know the element size, in other words it wouldn't know the length to 
> > > memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
> > > element size since elements are allocated by the implementation of 
> > > mempool_alloc_t and they could easily become inconsistent.  This patch is 
> > > what you want to merge, really.
> > > 
> > 
> > I forgot we don't have the size in there. Then I would suggest adding a
> > WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.
> > 
> 
> There is, it's a VM_WARN_ON_ONCE() that will show up if you configure 
> CONFIG_DEBUG_VM.
> 

Jens, are these two patches going to be merged into linux-block?

Thanks.

Re: [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Sat, 7 Mar 2015, David Rientjes wrote:

> > >>> mempool_alloc() does not support __GFP_ZERO since elements may come from
> > >>> memory that has already been released by mempool_free().
> > >>>
> > >>> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> > >>> initialize it to 0.
> > >>
> > >> You should add it to mempool instead, avoid having this issue show up for
> > >> other folks as well. It'd be trivial to do. Normal ->alloc() should honor
> > >> __GFP_ZERO, just do the same manually for removing an item from the internal
> > >> pool.
> > >>
> > > 
> > > Umm, it's not trivial to do and wouldn't make sense to do it.  Mempools 
> > 
> > Uhm, it would make sense, though.
> > 
> 
> Disagree, I don't think we should extend mempool to know the element size, 
> modify every user of mempool to pass it in, and keep it consistent with 
> mempool_alloc_t for the benefit of __GFP_ZERO for this one buggy caller.  
> Most users don't need __GFP_ZERO and just overwrite the entire element 
> after mempool_alloc() and it would be an unnecessary overhead to even 
> check for the bit set.  So it wouldn't make sense in terms of performance 
> or maintainability.
> 
> > > don't know the element size, in other words it wouldn't know the length to 
> > > memset() to 0 for mempool_alloc().  It shouldn't be modified to know the 
> > > element size since elements are allocated by the implementation of 
> > > mempool_alloc_t and they could easily become inconsistent.  This patch is 
> > > what you want to merge, really.
> > > 
> > 
> > I forgot we don't have the size in there. Then I would suggest adding a
> > WARN_ON() for __GFP_ZERO being set in mempool_alloc(), at the very least.
> > 
> 
> There is, it's a VM_WARN_ON_ONCE() that will show up if you configure 
> CONFIG_DEBUG_VM.
> 

Jens, are these two patches going to be merged into linux-block?

Thanks.

Re: [Drbd-dev] [DRBD-user] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Lars Ellenberg]

On Sat, Mar 07, 2015 at 04:24:02PM -0800, David Rientjes wrote:
> mempool_alloc() does not support __GFP_ZERO since elements may come from
> memory that has already been released by mempool_free().
> 
> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> initialize it to 0.

We used to have the explicit memset there,
but then changed that, because

I was under the impression that since
2007-07-17 d07dbea, Slab allocators: support __GFP_ZERO in all allocators
it was supported?

	Lars Ellenberg

Re: [DRBD-user] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[Lars Ellenberg]

On Sat, Mar 07, 2015 at 04:24:02PM -0800, David Rientjes wrote:
> mempool_alloc() does not support __GFP_ZERO since elements may come from
> memory that has already been released by mempool_free().
> 
> Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> initialize it to 0.

We used to have the explicit memset there,
but then changed that, because

I was under the impression that since
2007-07-17 d07dbea, Slab allocators: support __GFP_ZERO in all allocators
it was supported?

	Lars Ellenberg

Re: [Drbd-dev] [DRBD-user] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Tue, 10 Mar 2015, Lars Ellenberg wrote:

> > mempool_alloc() does not support __GFP_ZERO since elements may come from
> > memory that has already been released by mempool_free().
> > 
> > Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> > initialize it to 0.
> 
> We used to have the explicit memset there,
> but then changed that, because
> 
> I was under the impression that since
> 2007-07-17 d07dbea, Slab allocators: support __GFP_ZERO in all allocators
> it was supported?
> 

The slab allocators do support __GFP_ZERO, and they can do so because they 
know the object size to zero.

The problem is that this is a mempool, not a slab cache.

The mempool layer, based on top of the slab allocator in this case, will 
preserve elements (slab objects) for contexts when allocation may not be 
possible.  mempool_alloc(GFP_NOIO) may be able to allocate from the slab 
allocator and the object will be properly zeroed.  However, if it has to 
fallback to the reserved pool then mempool_alloc() will pull an element 
that may have already been allocated and freed back to the reserved pool.

In that latter case, the memory contents of the element is what it was 
when freed, assuming no use-after-free issues.  It cannot be zeroed by the 
mempool layer since mempools do not know of the object size.  We can't 
special-case mempools based on the slab allocator since then we have a 
situation where __GFP_ZERO works on some mempools but not others.  The 
rule is that __GFP_ZERO is never guaranteed for mempool_alloc() and that's 
included in the comment of the function as well as a WARN_ON_ONCE() if 
CONFIG_DEBUG_VM is enabled.

Re: [DRBD-user] [patch 1/2] block, drbd: fix drbd_req_new() initialization

[David Rientjes]

On Tue, 10 Mar 2015, Lars Ellenberg wrote:

> > mempool_alloc() does not support __GFP_ZERO since elements may come from
> > memory that has already been released by mempool_free().
> > 
> > Remove __GFP_ZERO from mempool_alloc() in drbd_req_new() and properly
> > initialize it to 0.
> 
> We used to have the explicit memset there,
> but then changed that, because
> 
> I was under the impression that since
> 2007-07-17 d07dbea, Slab allocators: support __GFP_ZERO in all allocators
> it was supported?
> 

The slab allocators do support __GFP_ZERO, and they can do so because they 
know the object size to zero.

The problem is that this is a mempool, not a slab cache.

The mempool layer, based on top of the slab allocator in this case, will 
preserve elements (slab objects) for contexts when allocation may not be 
possible.  mempool_alloc(GFP_NOIO) may be able to allocate from the slab 
allocator and the object will be properly zeroed.  However, if it has to 
fallback to the reserved pool then mempool_alloc() will pull an element 
that may have already been allocated and freed back to the reserved pool.

In that latter case, the memory contents of the element is what it was 
when freed, assuming no use-after-free issues.  It cannot be zeroed by the 
mempool layer since mempools do not know of the object size.  We can't 
special-case mempools based on the slab allocator since then we have a 
situation where __GFP_ZERO works on some mempools but not others.  The 
rule is that __GFP_ZERO is never guaranteed for mempool_alloc() and that's 
included in the comment of the function as well as a WARN_ON_ONCE() if 
CONFIG_DEBUG_VM is enabled.