dm-writeboost: Remove unsure BUG() from init_rambuf_pool

Commit Message

Satoru Takeuchi July 19, 2014, 11:40 a.m. UTC

Hi Joe and Akira,

At Sat, 19 Jul 2014 11:13:13 +0900,
Satoru Takeuchi wrote:
> 
> Hi Akira,
> 
> 2014-07-18 10:01 GMT+09:00 Akira Hayakawa <[e-mail]>:
> > Hi, Satoru,
> >
> > I totally agree with removing the BUG() at (2).
> >
> > However, I don't agree with setting the upper limit of nr_rambuf_pool.
> > You are saying that any memory allocation failure on initialization should be avoided
> > but that sounds going too far. Removing that possibility from all kernel codes is impossible in practice.
> > Carefully terminating the initialization on allocation failure is sufficient.

Both Akira and me seem to have completely different policy about the upper
limit of nr_rambuf_pool argument. However both of us agree with removing
unsure BUG() in int init_rambuf_pool(). So I wrote a patch to do so as a
 first step. Please apply this patch.

In addition, I'd like to know your opinion about whether setting
the upper limit of nr_rambuf_pool argument is neccesary or not.

Comments

Alasdair G Kergon July 19, 2014, 6:13 p.m. UTC | #1

On Sat, Jul 19, 2014 at 08:40:52PM +0900, Satoru Takeuchi wrote:
> In addition, I'd like to know your opinion about whether setting
> the upper limit of nr_rambuf_pool argument is neccesary or not.
 
1) If it's a parameter passed from userspace under the control of the
sysadmin, then you should just trust the sysadmin to set a sensible
value, and only return an error if the value they ask for is unobtainable.

Example: dm-ioctl.c copy_params()

2) If certain large values would be useless, leaving no memory for
anything else, then you could apply a sensible limit based on the amount 
of memory available.

Example: dm_bufio.c 

 * Memory management policy:
 *      Limit the number of buffers to DM_BUFIO_MEMORY_PERCENT of main memory
 *      or DM_BUFIO_VMALLOC_PERCENT of vmalloc memory (whichever is lower).
 *      Always allocate at least DM_BUFIO_MIN_BUFFERS buffers.

Alasdair

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Satoru Takeuchi July 21, 2014, 1:52 p.m. UTC | #2

Hi Alasdaia,

2014-07-20 3:13 GMT+09:00 Alasdair G Kergon <[e-mail]>:
> On Sat, Jul 19, 2014 at 08:40:52PM +0900, Satoru Takeuchi wrote:
>> In addition, I'd like to know your opinion about whether setting
>> the upper limit of nr_rambuf_pool argument is neccesary or not.
>
> 1) If it's a parameter passed from userspace under the control of the
> sysadmin, then you should just trust the sysadmin to set a sensible
> value, and only return an error if the value they ask for is unobtainable.

Thank you for your comment.

I don't say anything if this function returns an ENOMEM without any effect to
whole system. In this case, it fails *after* evicting massive amount of memory.
Since linux is used by very large amount of system, I consider someone will
eventually make a mistake. So I consider setting upper limit is better way
from the perspective of fool proof.

>
> Example: dm-ioctl.c copy_params()
>
> 2) If certain large values would be useless, leaving no memory for
> anything else, then you could apply a sensible limit based on the amount
> of memory available.

I agree with you.

It's difficult to guess the appropriate upper limit from the perspective of
dm-writeboost at this point since it's depend on system and requires
a number of performance measurements. So, the second best way is limiting
the rambuf size based on the available memory as you say. It can avoid
the above-mentioned problem anyway and very easy to implement.

Thanks,
Satoru

>
> Example: dm_bufio.c
>
>  * Memory management policy:
>  *      Limit the number of buffers to DM_BUFIO_MEMORY_PERCENT of main memory
>  *      or DM_BUFIO_VMALLOC_PERCENT of vmalloc memory (whichever is lower).
>  *      Always allocate at least DM_BUFIO_MIN_BUFFERS buffers.
>
> Alasdair
>

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Akira Hayakawa July 22, 2014, 7:14 a.m. UTC | #3

Hi, Satoru,

As for removing BUG() at the line I will definitely ack but
please also send a pull request to Joe's tree where we
are raising up dm-writeboost. Sorry, I didn't tell you this.

In my opinion, I like to see the patch in both pull request and dm-devel.
This shares where dm-writeboost is going to with DM guys.
If you have other works to dm-writeboost, please do as this time.

Note that where you send the pull request is
thin-dev branch of jthornber/linux-2.6.
It's where dm-writeboost is in.

Anyway, I think you start to involve dm-writeboost.
It is really welcome.
And bringing us discussion from user/sysadmin perspective
is greatly welcome, too.

- Akira



On Sat, 19 Jul 2014 20:40:52 +0900
Satoru Takeuchi <[e-mail]> wrote:

> Hi Joe and Akira,
> 
> At Sat, 19 Jul 2014 11:13:13 +0900,
> Satoru Takeuchi wrote:
> > 
> > Hi Akira,
> > 
> > 2014-07-18 10:01 GMT+09:00 Akira Hayakawa <[e-mail]>:
> > > Hi, Satoru,
> > >
> > > I totally agree with removing the BUG() at (2).
> > >
> > > However, I don't agree with setting the upper limit of nr_rambuf_pool.
> > > You are saying that any memory allocation failure on initialization should be avoided
> > > but that sounds going too far. Removing that possibility from all kernel codes is impossible in practice.
> > > Carefully terminating the initialization on allocation failure is sufficient.
> 
> Both Akira and me seem to have completely different policy about the upper
> limit of nr_rambuf_pool argument. However both of us agree with removing
> unsure BUG() in int init_rambuf_pool(). So I wrote a patch to do so as a
>  first step. Please apply this patch.
> 
> In addition, I'd like to know your opinion about whether setting
> the upper limit of nr_rambuf_pool argument is neccesary or not.
> 
> 
> 
> =======
> From: Satoru Takeuchi <[e-mail]>
> 
> If users set a very large value to nr_rambuf_pool argument, kernel would hit
> BUG() in the error route of init_rambuf_pool().
> 
> drivers/md/dm-writeboost-metadata.c:
> ===============================================================================
> ...
> static int init_rambuf_pool(struct wb_device *wb)
> {
> 	int r = 0;
> 	size_t i;
> 
> 	wb->rambuf_pool = kmalloc(sizeof(struct rambuffer) * wb->nr_rambuf_pool,
> 				  GFP_KERNEL);
> 	if (!wb->rambuf_pool)			# It is true here.
> 		return -ENOMEM;
> 
> 	wb->rambuf_cachep = kmem_cache_create("dmwb_rambuf",
> 			1 << (wb->segment_size_order + SECTOR_SHIFT),
> 			1 << (wb->segment_size_order + SECTOR_SHIFT),
> 			SLAB_RED_ZONE, NULL);
> 	if (!wb->rambuf_cachep) {
> 		r = -ENOMEM;			# Enter this route or ....
> 		goto bad_cachep;
> 	}
> 
> 	for (i = 0; i < wb->nr_rambuf_pool; i++) {
> 		size_t j;
> 		struct rambuffer *rambuf = wb->rambuf_pool + i;
> 
> 		rambuf->data = kmem_cache_alloc(wb->rambuf_cachep, GFP_KERNEL);
> 		if (!rambuf->data) {
> 			...			# ... enter this route.
> 			goto bad_alloc_data;
> 		}
> 		check_buffer_alignment(rambuf->data);
> 	}
> 
> 	return r;
> 
> bad_alloc_data:
> 	kmem_cache_destroy(wb->rambuf_cachep);
> bad_cachep:
> 	kfree(wb->rambuf_pool);
> 	BUG();					# Kernel panic happens here.
> 	return r;
> }
> ...
> ===============================================================================
> 
> Probably this BUG() was introduced erroneously and is safe to remove.
> 
> Signed-off-by: Satoru Takeuchi <[e-mail]>
> Cc: Joe Thornber <[e-mail]>
> Cc: Akira Hayakawa <[e-mail]>
> ---
>  drivers/md/dm-writeboost-metadata.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/md/dm-writeboost-metadata.c b/drivers/md/dm-writeboost-metadata.c
> index b7b3eb7..ce6ea056 100644
> --- a/drivers/md/dm-writeboost-metadata.c
> +++ b/drivers/md/dm-writeboost-metadata.c
> @@ -702,7 +702,6 @@ bad_alloc_data:
>  	kmem_cache_destroy(wb->rambuf_cachep);
>  bad_cachep:
>  	kfree(wb->rambuf_pool);
> -	BUG();
>  	return r;
>  }
>  
> -- 
> 2.0.1
> 
> --
> dm-devel mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/dm-devel

Patch

=======
From: Satoru Takeuchi <[e-mail]>

If users set a very large value to nr_rambuf_pool argument, kernel would hit
BUG() in the error route of init_rambuf_pool().

drivers/md/dm-writeboost-metadata.c:
===============================================================================
...
static int init_rambuf_pool(struct wb_device *wb)
{
	int r = 0;
	size_t i;

	wb->rambuf_pool = kmalloc(sizeof(struct rambuffer) * wb->nr_rambuf_pool,
				  GFP_KERNEL);
	if (!wb->rambuf_pool)			# It is true here.
		return -ENOMEM;

	wb->rambuf_cachep = kmem_cache_create("dmwb_rambuf",
			1 << (wb->segment_size_order + SECTOR_SHIFT),
			1 << (wb->segment_size_order + SECTOR_SHIFT),
			SLAB_RED_ZONE, NULL);
	if (!wb->rambuf_cachep) {
		r = -ENOMEM;			# Enter this route or ....
		goto bad_cachep;
	}

	for (i = 0; i < wb->nr_rambuf_pool; i++) {
		size_t j;
		struct rambuffer *rambuf = wb->rambuf_pool + i;

		rambuf->data = kmem_cache_alloc(wb->rambuf_cachep, GFP_KERNEL);
		if (!rambuf->data) {
			...			# ... enter this route.
			goto bad_alloc_data;
		}
		check_buffer_alignment(rambuf->data);
	}

	return r;

bad_alloc_data:
	kmem_cache_destroy(wb->rambuf_cachep);
bad_cachep:
	kfree(wb->rambuf_pool);
	BUG();					# Kernel panic happens here.
	return r;
}
...
===============================================================================

Probably this BUG() was introduced erroneously and is safe to remove.

Signed-off-by: Satoru Takeuchi <[e-mail]>
Cc: Joe Thornber <[e-mail]>
Cc: Akira Hayakawa <[e-mail]>
---
 drivers/md/dm-writeboost-metadata.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/dm-writeboost-metadata.c b/drivers/md/dm-writeboost-metadata.c
index b7b3eb7..ce6ea056 100644
--- a/drivers/md/dm-writeboost-metadata.c
+++ b/drivers/md/dm-writeboost-metadata.c
@@ -702,7 +702,6 @@  bad_alloc_data:
 	kmem_cache_destroy(wb->rambuf_cachep);
 bad_cachep:
 	kfree(wb->rambuf_pool);
-	BUG();
 	return r;
 }