[PATCH 0/4] rbd: use slab caches for frequently allocated structures

[PATCH 0/4] rbd: use slab caches for frequently allocated structures

[Alex Elder]

This series adds some slab caches for structures related
to rbd osd requests.

					-Alex

[PATCH 1/4] rbd: allocate image requests with a slab allocator
[PATCH 2/4] rbd: allocate name separate from obj_request
[PATCH 3/4] rbd: allocate object requests with a slab allocator
[PATCH 4/4] rbd: allocate image object names with a slab allocator

[PATCH 1/4] rbd: allocate image requests with a slab allocator

[Alex Elder]

Create a slab cache to manage rbd_img_request allocation.  Nothing
too fancy at this point--we'll still initialize everything at
allocation time (no constructor)

This is part of:
    http://tracker.ceph.com/issues/3926

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a6e5fe3..005c397 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -40,6 +40,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/blkdev.h>
+#include <linux/slab.h>

 #include "rbd_types.h"

@@ -344,6 +345,8 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
 static LIST_HEAD(rbd_client_list);		/* clients */
 static DEFINE_SPINLOCK(rbd_client_list_lock);

+static struct kmem_cache	*rbd_img_request_cache;
+
 static int rbd_img_request_submit(struct rbd_img_request *img_request);

 static void rbd_dev_device_release(struct device *dev);
@@ -1821,7 +1824,7 @@ static struct rbd_img_request *rbd_img_request_create(
 {
 	struct rbd_img_request *img_request;

-	img_request = kmalloc(sizeof (*img_request), GFP_ATOMIC);
+	img_request = kmem_cache_alloc(rbd_img_request_cache, GFP_KERNEL);
 	if (!img_request)
 		return NULL;

@@ -1884,7 +1887,7 @@ static void rbd_img_request_destroy(struct kref *kref)
 	if (img_request_child_test(img_request))
 		rbd_obj_request_put(img_request->obj_request);

-	kfree(img_request);
+	kmem_cache_free(rbd_img_request_cache, img_request);
 }

 static bool rbd_img_obj_end_request(struct rbd_obj_request *obj_request)
@@ -4992,6 +4995,26 @@ static void rbd_sysfs_cleanup(void)
 	device_unregister(&rbd_root_dev);
 }

+static int rbd_slab_init(void)
+{
+	rbd_assert(!rbd_img_request_cache);
+	rbd_img_request_cache = kmem_cache_create("rbd_img_request",
+					sizeof (struct rbd_img_request),
+					__alignof__(struct rbd_img_request),
+					0, NULL);
+	if (rbd_img_request_cache)
+		return 0;
+
+	return -ENOMEM;
+}
+
+static void rbd_slab_exit(void)
+{
+	rbd_assert(rbd_img_request_cache);
+	kmem_cache_destroy(rbd_img_request_cache);
+	rbd_img_request_cache = NULL;
+}
+
 static int __init rbd_init(void)
 {
 	int rc;
@@ -5001,16 +5024,22 @@ static int __init rbd_init(void)

 		return -EINVAL;
 	}
-	rc = rbd_sysfs_init();
+	rc = rbd_slab_init();
 	if (rc)
 		return rc;
-	pr_info("loaded " RBD_DRV_NAME_LONG "\n");
-	return 0;
+	rc = rbd_sysfs_init();
+	if (rc)
+		rbd_slab_exit();
+	else
+		pr_info("loaded " RBD_DRV_NAME_LONG "\n");
+
+	return rc;
 }

 static void __exit rbd_exit(void)
 {
 	rbd_sysfs_cleanup();
+	rbd_slab_exit();
 }

 module_init(rbd_init);
-- 
1.7.9.5

[PATCH 2/4] rbd: allocate name separate from obj_request

[Alex Elder]

The next patch will define a slab allocator for a object requests.
To use that we'll need to allocate the name of an object separate
from the request structure itself.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 005c397..da9f41d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1758,11 +1758,16 @@ static struct rbd_obj_request
*rbd_obj_request_create(const char *object_name,
 	rbd_assert(obj_request_type_valid(type));

 	size = strlen(object_name) + 1;
-	obj_request = kzalloc(sizeof (*obj_request) + size, GFP_KERNEL);
-	if (!obj_request)
+	name = kmalloc(size, GFP_KERNEL);
+	if (!name)
+		return NULL;
+
+	obj_request = kzalloc(sizeof (*obj_request), GFP_KERNEL);
+	if (!obj_request) {
+		kfree(name);
 		return NULL;
+	}

-	name = (char *)(obj_request + 1);
 	obj_request->object_name = memcpy(name, object_name, size);
 	obj_request->offset = offset;
 	obj_request->length = length;
@@ -1808,6 +1813,7 @@ static void rbd_obj_request_destroy(struct kref *kref)
 		break;
 	}

+	kfree(obj_request->object_name);
 	kfree(obj_request);
 }

-- 
1.7.9.5

[PATCH 3/4] rbd: allocate object requests with a slab allocator

[Alex Elder]

Create a slab cache to manage rbd_obj_request allocation.  We aren't
using a constructor, and we'll zero-fill object request structures
when they're allocated.

This is part of:
    http://tracker.ceph.com/issues/3926

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index da9f41d..28a5ea3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -346,6 +346,7 @@ static LIST_HEAD(rbd_client_list);		/* clients */
 static DEFINE_SPINLOCK(rbd_client_list_lock);

 static struct kmem_cache	*rbd_img_request_cache;
+static struct kmem_cache	*rbd_obj_request_cache;

 static int rbd_img_request_submit(struct rbd_img_request *img_request);

@@ -1762,7 +1763,7 @@ static struct rbd_obj_request
*rbd_obj_request_create(const char *object_name,
 	if (!name)
 		return NULL;

-	obj_request = kzalloc(sizeof (*obj_request), GFP_KERNEL);
+	obj_request = kmem_cache_zalloc(rbd_obj_request_cache, GFP_KERNEL);
 	if (!obj_request) {
 		kfree(name);
 		return NULL;
@@ -1814,7 +1815,8 @@ static void rbd_obj_request_destroy(struct kref *kref)
 	}

 	kfree(obj_request->object_name);
-	kfree(obj_request);
+	obj_request->object_name = NULL;
+	kmem_cache_free(rbd_obj_request_cache, obj_request);
 }

 /*
@@ -5008,14 +5010,29 @@ static int rbd_slab_init(void)
 					sizeof (struct rbd_img_request),
 					__alignof__(struct rbd_img_request),
 					0, NULL);
-	if (rbd_img_request_cache)
+	if (!rbd_img_request_cache)
+		return -ENOMEM;
+
+	rbd_assert(!rbd_obj_request_cache);
+	rbd_obj_request_cache = kmem_cache_create("rbd_obj_request",
+					sizeof (struct rbd_obj_request),
+					__alignof__(struct rbd_obj_request),
+					0, NULL);
+	if (rbd_obj_request_cache)
 		return 0;

+	kmem_cache_destroy(rbd_img_request_cache);
+	rbd_img_request_cache = NULL;
+
 	return -ENOMEM;
 }

 static void rbd_slab_exit(void)
 {
+	rbd_assert(rbd_obj_request_cache);
+	kmem_cache_destroy(rbd_obj_request_cache);
+	rbd_obj_request_cache = NULL;
+
 	rbd_assert(rbd_img_request_cache);
 	kmem_cache_destroy(rbd_img_request_cache);
 	rbd_img_request_cache = NULL;
-- 
1.7.9.5

[PATCH 4/4] rbd: allocate image object names with a slab allocator

[Alex Elder]

The names of objects used for image object requests are always fixed
size.  So create a slab cache to manage them.  Define a new function
rbd_segment_name_free() to match rbd_segment_name() (which is what
supplies the dynamically-allocated name buffer).

This is part of:
    http://tracker.ceph.com/issues/3926

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 28a5ea3..8d9aeef 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -345,8 +345,11 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
 static LIST_HEAD(rbd_client_list);		/* clients */
 static DEFINE_SPINLOCK(rbd_client_list_lock);

+/* Slab caches for frequently-allocated structures */
+
 static struct kmem_cache	*rbd_img_request_cache;
 static struct kmem_cache	*rbd_obj_request_cache;
+static struct kmem_cache	*rbd_segment_name_cache;

 static int rbd_img_request_submit(struct rbd_img_request *img_request);

@@ -985,7 +988,7 @@ static const char *rbd_segment_name(struct
rbd_device *rbd_dev, u64 offset)
 	u64 segment;
 	int ret;

-	name = kmalloc(MAX_OBJ_NAME_SIZE + 1, GFP_NOIO);
+	name = kmem_cache_alloc(rbd_segment_name_cache, GFP_NOIO);
 	if (!name)
 		return NULL;
 	segment = offset >> rbd_dev->header.obj_order;
@@ -1001,6 +1004,13 @@ static const char *rbd_segment_name(struct
rbd_device *rbd_dev, u64 offset)
 	return name;
 }

+static void rbd_segment_name_free(const char *name)
+{
+	/* The explicit cast here is needed to drop the const qualifier */
+
+	kmem_cache_free(rbd_segment_name_cache, (void *)name);
+}
+
 static u64 rbd_segment_offset(struct rbd_device *rbd_dev, u64 offset)
 {
 	u64 segment_size = (u64) 1 << rbd_dev->header.obj_order;
@@ -2033,7 +2043,8 @@ static int rbd_img_request_fill(struct
rbd_img_request *img_request,
 		length = rbd_segment_length(rbd_dev, img_offset, resid);
 		obj_request = rbd_obj_request_create(object_name,
 						offset, length, type);
-		kfree(object_name);	/* object request has its own copy */
+		/* object request has its own copy of the object name */
+		rbd_segment_name_free(object_name);
 		if (!obj_request)
 			goto out_unwind;

@@ -5018,17 +5029,32 @@ static int rbd_slab_init(void)
 					sizeof (struct rbd_obj_request),
 					__alignof__(struct rbd_obj_request),
 					0, NULL);
-	if (rbd_obj_request_cache)
-		return 0;
+	if (!rbd_obj_request_cache)
+		goto out_err;

+	rbd_assert(!rbd_segment_name_cache);
+	rbd_segment_name_cache = kmem_cache_create("rbd_segment_name",
+					MAX_OBJ_NAME_SIZE + 1, 1, 0, NULL);
+	if (rbd_segment_name_cache)
+		return 0;
+out_err:
 	kmem_cache_destroy(rbd_img_request_cache);
 	rbd_img_request_cache = NULL;

+	if (rbd_img_request_cache) {
+		kmem_cache_destroy(rbd_obj_request_cache);
+		rbd_img_request_cache = NULL;
+	}
+
 	return -ENOMEM;
 }

 static void rbd_slab_exit(void)
 {
+	rbd_assert(rbd_segment_name_cache);
+	kmem_cache_destroy(rbd_segment_name_cache);
+	rbd_segment_name_cache = NULL;
+
 	rbd_assert(rbd_obj_request_cache);
 	kmem_cache_destroy(rbd_obj_request_cache);
 	rbd_obj_request_cache = NULL;
-- 
1.7.9.5

Re: [PATCH 1/4] rbd: allocate image requests with a slab allocator

[Josh Durgin]

It looks like the GFP flags used throughout request creation
could be cleanup up (there's a mix of ATOMIC, KERNEL, and NOIO).
I just noticed since this changes a GFP_ATOMIC to GFP_KERNEL.
Making them consistent can be a later patch though.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 05/01/2013 02:35 PM, Alex Elder wrote:
> Create a slab cache to manage rbd_img_request allocation.  Nothing
> too fancy at this point--we'll still initialize everything at
> allocation time (no constructor)
>
> This is part of:
>      http://tracker.ceph.com/issues/3926
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   39 ++++++++++++++++++++++++++++++++++-----
>   1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a6e5fe3..005c397 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -40,6 +40,7 @@
>   #include <linux/module.h>
>   #include <linux/fs.h>
>   #include <linux/blkdev.h>
> +#include <linux/slab.h>
>
>   #include "rbd_types.h"
>
> @@ -344,6 +345,8 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
>   static LIST_HEAD(rbd_client_list);		/* clients */
>   static DEFINE_SPINLOCK(rbd_client_list_lock);
>
> +static struct kmem_cache	*rbd_img_request_cache;
> +
>   static int rbd_img_request_submit(struct rbd_img_request *img_request);
>
>   static void rbd_dev_device_release(struct device *dev);
> @@ -1821,7 +1824,7 @@ static struct rbd_img_request *rbd_img_request_create(
>   {
>   	struct rbd_img_request *img_request;
>
> -	img_request = kmalloc(sizeof (*img_request), GFP_ATOMIC);
> +	img_request = kmem_cache_alloc(rbd_img_request_cache, GFP_KERNEL);
>   	if (!img_request)
>   		return NULL;
>
> @@ -1884,7 +1887,7 @@ static void rbd_img_request_destroy(struct kref *kref)
>   	if (img_request_child_test(img_request))
>   		rbd_obj_request_put(img_request->obj_request);
>
> -	kfree(img_request);
> +	kmem_cache_free(rbd_img_request_cache, img_request);
>   }
>
>   static bool rbd_img_obj_end_request(struct rbd_obj_request *obj_request)
> @@ -4992,6 +4995,26 @@ static void rbd_sysfs_cleanup(void)
>   	device_unregister(&rbd_root_dev);
>   }
>
> +static int rbd_slab_init(void)
> +{
> +	rbd_assert(!rbd_img_request_cache);
> +	rbd_img_request_cache = kmem_cache_create("rbd_img_request",
> +					sizeof (struct rbd_img_request),
> +					__alignof__(struct rbd_img_request),
> +					0, NULL);
> +	if (rbd_img_request_cache)
> +		return 0;
> +
> +	return -ENOMEM;
> +}
> +
> +static void rbd_slab_exit(void)
> +{
> +	rbd_assert(rbd_img_request_cache);
> +	kmem_cache_destroy(rbd_img_request_cache);
> +	rbd_img_request_cache = NULL;
> +}
> +
>   static int __init rbd_init(void)
>   {
>   	int rc;
> @@ -5001,16 +5024,22 @@ static int __init rbd_init(void)
>
>   		return -EINVAL;
>   	}
> -	rc = rbd_sysfs_init();
> +	rc = rbd_slab_init();
>   	if (rc)
>   		return rc;
> -	pr_info("loaded " RBD_DRV_NAME_LONG "\n");
> -	return 0;
> +	rc = rbd_sysfs_init();
> +	if (rc)
> +		rbd_slab_exit();
> +	else
> +		pr_info("loaded " RBD_DRV_NAME_LONG "\n");
> +
> +	return rc;
>   }
>
>   static void __exit rbd_exit(void)
>   {
>   	rbd_sysfs_cleanup();
> +	rbd_slab_exit();
>   }
>
>   module_init(rbd_init);
>

Re: [PATCH 2/4] rbd: allocate name separate from obj_request

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 05/01/2013 02:35 PM, Alex Elder wrote:
> The next patch will define a slab allocator for a object requests.
> To use that we'll need to allocate the name of an object separate
> from the request structure itself.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 005c397..da9f41d 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1758,11 +1758,16 @@ static struct rbd_obj_request
> *rbd_obj_request_create(const char *object_name,
>   	rbd_assert(obj_request_type_valid(type));
>
>   	size = strlen(object_name) + 1;
> -	obj_request = kzalloc(sizeof (*obj_request) + size, GFP_KERNEL);
> -	if (!obj_request)
> +	name = kmalloc(size, GFP_KERNEL);
> +	if (!name)
> +		return NULL;
> +
> +	obj_request = kzalloc(sizeof (*obj_request), GFP_KERNEL);
> +	if (!obj_request) {
> +		kfree(name);
>   		return NULL;
> +	}
>
> -	name = (char *)(obj_request + 1);
>   	obj_request->object_name = memcpy(name, object_name, size);
>   	obj_request->offset = offset;
>   	obj_request->length = length;
> @@ -1808,6 +1813,7 @@ static void rbd_obj_request_destroy(struct kref *kref)
>   		break;
>   	}
>
> +	kfree(obj_request->object_name);
>   	kfree(obj_request);
>   }
>

Re: [PATCH 3/4] rbd: allocate object requests with a slab allocator

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 05/01/2013 02:36 PM, Alex Elder wrote:
> Create a slab cache to manage rbd_obj_request allocation.  We aren't
> using a constructor, and we'll zero-fill object request structures
> when they're allocated.
>
> This is part of:
>      http://tracker.ceph.com/issues/3926
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index da9f41d..28a5ea3 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -346,6 +346,7 @@ static LIST_HEAD(rbd_client_list);		/* clients */
>   static DEFINE_SPINLOCK(rbd_client_list_lock);
>
>   static struct kmem_cache	*rbd_img_request_cache;
> +static struct kmem_cache	*rbd_obj_request_cache;
>
>   static int rbd_img_request_submit(struct rbd_img_request *img_request);
>
> @@ -1762,7 +1763,7 @@ static struct rbd_obj_request
> *rbd_obj_request_create(const char *object_name,
>   	if (!name)
>   		return NULL;
>
> -	obj_request = kzalloc(sizeof (*obj_request), GFP_KERNEL);
> +	obj_request = kmem_cache_zalloc(rbd_obj_request_cache, GFP_KERNEL);
>   	if (!obj_request) {
>   		kfree(name);
>   		return NULL;
> @@ -1814,7 +1815,8 @@ static void rbd_obj_request_destroy(struct kref *kref)
>   	}
>
>   	kfree(obj_request->object_name);
> -	kfree(obj_request);
> +	obj_request->object_name = NULL;
> +	kmem_cache_free(rbd_obj_request_cache, obj_request);
>   }
>
>   /*
> @@ -5008,14 +5010,29 @@ static int rbd_slab_init(void)
>   					sizeof (struct rbd_img_request),
>   					__alignof__(struct rbd_img_request),
>   					0, NULL);
> -	if (rbd_img_request_cache)
> +	if (!rbd_img_request_cache)
> +		return -ENOMEM;
> +
> +	rbd_assert(!rbd_obj_request_cache);
> +	rbd_obj_request_cache = kmem_cache_create("rbd_obj_request",
> +					sizeof (struct rbd_obj_request),
> +					__alignof__(struct rbd_obj_request),
> +					0, NULL);
> +	if (rbd_obj_request_cache)
>   		return 0;
>
> +	kmem_cache_destroy(rbd_img_request_cache);
> +	rbd_img_request_cache = NULL;
> +
>   	return -ENOMEM;
>   }
>
>   static void rbd_slab_exit(void)
>   {
> +	rbd_assert(rbd_obj_request_cache);
> +	kmem_cache_destroy(rbd_obj_request_cache);
> +	rbd_obj_request_cache = NULL;
> +
>   	rbd_assert(rbd_img_request_cache);
>   	kmem_cache_destroy(rbd_img_request_cache);
>   	rbd_img_request_cache = NULL;
>

Re: [PATCH 4/4] rbd: allocate image object names with a slab allocator

[Josh Durgin]

On 05/01/2013 02:36 PM, Alex Elder wrote:
> The names of objects used for image object requests are always fixed
> size.  So create a slab cache to manage them.  Define a new function
> rbd_segment_name_free() to match rbd_segment_name() (which is what
> supplies the dynamically-allocated name buffer).
>
> This is part of:
>      http://tracker.ceph.com/issues/3926
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   34 ++++++++++++++++++++++++++++++----
>   1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 28a5ea3..8d9aeef 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -345,8 +345,11 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
>   static LIST_HEAD(rbd_client_list);		/* clients */
>   static DEFINE_SPINLOCK(rbd_client_list_lock);
>
> +/* Slab caches for frequently-allocated structures */
> +
>   static struct kmem_cache	*rbd_img_request_cache;
>   static struct kmem_cache	*rbd_obj_request_cache;
> +static struct kmem_cache	*rbd_segment_name_cache;
>
>   static int rbd_img_request_submit(struct rbd_img_request *img_request);
>
> @@ -985,7 +988,7 @@ static const char *rbd_segment_name(struct
> rbd_device *rbd_dev, u64 offset)
>   	u64 segment;
>   	int ret;
>
> -	name = kmalloc(MAX_OBJ_NAME_SIZE + 1, GFP_NOIO);
> +	name = kmem_cache_alloc(rbd_segment_name_cache, GFP_NOIO);
>   	if (!name)
>   		return NULL;
>   	segment = offset >> rbd_dev->header.obj_order;
> @@ -1001,6 +1004,13 @@ static const char *rbd_segment_name(struct
> rbd_device *rbd_dev, u64 offset)
>   	return name;
>   }
>
> +static void rbd_segment_name_free(const char *name)
> +{
> +	/* The explicit cast here is needed to drop the const qualifier */
> +
> +	kmem_cache_free(rbd_segment_name_cache, (void *)name);
> +}
> +
>   static u64 rbd_segment_offset(struct rbd_device *rbd_dev, u64 offset)
>   {
>   	u64 segment_size = (u64) 1 << rbd_dev->header.obj_order;
> @@ -2033,7 +2043,8 @@ static int rbd_img_request_fill(struct
> rbd_img_request *img_request,
>   		length = rbd_segment_length(rbd_dev, img_offset, resid);
>   		obj_request = rbd_obj_request_create(object_name,
>   						offset, length, type);
> -		kfree(object_name);	/* object request has its own copy */
> +		/* object request has its own copy of the object name */
> +		rbd_segment_name_free(object_name);
>   		if (!obj_request)
>   			goto out_unwind;
>
> @@ -5018,17 +5029,32 @@ static int rbd_slab_init(void)
>   					sizeof (struct rbd_obj_request),
>   					__alignof__(struct rbd_obj_request),
>   					0, NULL);
> -	if (rbd_obj_request_cache)
> -		return 0;
> +	if (!rbd_obj_request_cache)
> +		goto out_err;
>
> +	rbd_assert(!rbd_segment_name_cache);
> +	rbd_segment_name_cache = kmem_cache_create("rbd_segment_name",
> +					MAX_OBJ_NAME_SIZE + 1, 1, 0, NULL);
> +	if (rbd_segment_name_cache)
> +		return 0;
> +out_err:
>   	kmem_cache_destroy(rbd_img_request_cache);
>   	rbd_img_request_cache = NULL;
>
> +	if (rbd_img_request_cache) {
> +		kmem_cache_destroy(rbd_obj_request_cache);
> +		rbd_img_request_cache = NULL;

Should be rbd_obj_request_cache here and in the condition two lines up.
With that fixed: Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

> +	}
> +
>   	return -ENOMEM;
>   }
>
>   static void rbd_slab_exit(void)
>   {
> +	rbd_assert(rbd_segment_name_cache);
> +	kmem_cache_destroy(rbd_segment_name_cache);
> +	rbd_segment_name_cache = NULL;
> +
>   	rbd_assert(rbd_obj_request_cache);
>   	kmem_cache_destroy(rbd_obj_request_cache);
>   	rbd_obj_request_cache = NULL;
>

Re: [PATCH 1/4] rbd: allocate image requests with a slab allocator

[Alex Elder]

On 05/02/2013 11:19 AM, Josh Durgin wrote:
> It looks like the GFP flags used throughout request creation
> could be cleanup up (there's a mix of ATOMIC, KERNEL, and NOIO).
> I just noticed since this changes a GFP_ATOMIC to GFP_KERNEL.
> Making them consistent can be a later patch though.

Well that was unintentional--a copy-paste with haste error.

But yes, I've already got a bug open on that.

    http://tracker.ceph.com/issues/4233

> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
> On 05/01/2013 02:35 PM, Alex Elder wrote:
>> Create a slab cache to manage rbd_img_request allocation.  Nothing
>> too fancy at this point--we'll still initialize everything at
>> allocation time (no constructor)
>>
>> This is part of:
>>      http://tracker.ceph.com/issues/3926
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   39 ++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index a6e5fe3..005c397 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -40,6 +40,7 @@
>>   #include <linux/module.h>
>>   #include <linux/fs.h>
>>   #include <linux/blkdev.h>
>> +#include <linux/slab.h>
>>
>>   #include "rbd_types.h"
>>
>> @@ -344,6 +345,8 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
>>   static LIST_HEAD(rbd_client_list);        /* clients */
>>   static DEFINE_SPINLOCK(rbd_client_list_lock);
>>
>> +static struct kmem_cache    *rbd_img_request_cache;
>> +
>>   static int rbd_img_request_submit(struct rbd_img_request *img_request);
>>
>>   static void rbd_dev_device_release(struct device *dev);
>> @@ -1821,7 +1824,7 @@ static struct rbd_img_request
>> *rbd_img_request_create(
>>   {
>>       struct rbd_img_request *img_request;
>>
>> -    img_request = kmalloc(sizeof (*img_request), GFP_ATOMIC);
>> +    img_request = kmem_cache_alloc(rbd_img_request_cache, GFP_KERNEL);
>>       if (!img_request)
>>           return NULL;
>>
>> @@ -1884,7 +1887,7 @@ static void rbd_img_request_destroy(struct kref
>> *kref)
>>       if (img_request_child_test(img_request))
>>           rbd_obj_request_put(img_request->obj_request);
>>
>> -    kfree(img_request);
>> +    kmem_cache_free(rbd_img_request_cache, img_request);
>>   }
>>
>>   static bool rbd_img_obj_end_request(struct rbd_obj_request
>> *obj_request)
>> @@ -4992,6 +4995,26 @@ static void rbd_sysfs_cleanup(void)
>>       device_unregister(&rbd_root_dev);
>>   }
>>
>> +static int rbd_slab_init(void)
>> +{
>> +    rbd_assert(!rbd_img_request_cache);
>> +    rbd_img_request_cache = kmem_cache_create("rbd_img_request",
>> +                    sizeof (struct rbd_img_request),
>> +                    __alignof__(struct rbd_img_request),
>> +                    0, NULL);
>> +    if (rbd_img_request_cache)
>> +        return 0;
>> +
>> +    return -ENOMEM;
>> +}
>> +
>> +static void rbd_slab_exit(void)
>> +{
>> +    rbd_assert(rbd_img_request_cache);
>> +    kmem_cache_destroy(rbd_img_request_cache);
>> +    rbd_img_request_cache = NULL;
>> +}
>> +
>>   static int __init rbd_init(void)
>>   {
>>       int rc;
>> @@ -5001,16 +5024,22 @@ static int __init rbd_init(void)
>>
>>           return -EINVAL;
>>       }
>> -    rc = rbd_sysfs_init();
>> +    rc = rbd_slab_init();
>>       if (rc)
>>           return rc;
>> -    pr_info("loaded " RBD_DRV_NAME_LONG "\n");
>> -    return 0;
>> +    rc = rbd_sysfs_init();
>> +    if (rc)
>> +        rbd_slab_exit();
>> +    else
>> +        pr_info("loaded " RBD_DRV_NAME_LONG "\n");
>> +
>> +    return rc;
>>   }
>>
>>   static void __exit rbd_exit(void)
>>   {
>>       rbd_sysfs_cleanup();
>> +    rbd_slab_exit();
>>   }
>>
>>   module_init(rbd_init);
>>
> 

Re: [PATCH 4/4] rbd: allocate image object names with a slab allocator

[Alex Elder]

On 05/02/2013 11:24 AM, Josh Durgin wrote:
> On 05/01/2013 02:36 PM, Alex Elder wrote:
>> The names of objects used for image object requests are always fixed
>> size.  So create a slab cache to manage them.  Define a new function
>> rbd_segment_name_free() to match rbd_segment_name() (which is what
>> supplies the dynamically-allocated name buffer).
>>
>> This is part of:
>>      http://tracker.ceph.com/issues/3926
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   34 ++++++++++++++++++++++++++++++----
>>   1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 28a5ea3..8d9aeef 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -345,8 +345,11 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
>>   static LIST_HEAD(rbd_client_list);        /* clients */
>>   static DEFINE_SPINLOCK(rbd_client_list_lock);
>>
>> +/* Slab caches for frequently-allocated structures */
>> +
>>   static struct kmem_cache    *rbd_img_request_cache;
>>   static struct kmem_cache    *rbd_obj_request_cache;
>> +static struct kmem_cache    *rbd_segment_name_cache;
>>
>>   static int rbd_img_request_submit(struct rbd_img_request *img_request);
>>
>> @@ -985,7 +988,7 @@ static const char *rbd_segment_name(struct
>> rbd_device *rbd_dev, u64 offset)
>>       u64 segment;
>>       int ret;
>>
>> -    name = kmalloc(MAX_OBJ_NAME_SIZE + 1, GFP_NOIO);
>> +    name = kmem_cache_alloc(rbd_segment_name_cache, GFP_NOIO);
>>       if (!name)
>>           return NULL;
>>       segment = offset >> rbd_dev->header.obj_order;
>> @@ -1001,6 +1004,13 @@ static const char *rbd_segment_name(struct
>> rbd_device *rbd_dev, u64 offset)
>>       return name;
>>   }
>>
>> +static void rbd_segment_name_free(const char *name)
>> +{
>> +    /* The explicit cast here is needed to drop the const qualifier */
>> +
>> +    kmem_cache_free(rbd_segment_name_cache, (void *)name);
>> +}
>> +
>>   static u64 rbd_segment_offset(struct rbd_device *rbd_dev, u64 offset)
>>   {
>>       u64 segment_size = (u64) 1 << rbd_dev->header.obj_order;
>> @@ -2033,7 +2043,8 @@ static int rbd_img_request_fill(struct
>> rbd_img_request *img_request,
>>           length = rbd_segment_length(rbd_dev, img_offset, resid);
>>           obj_request = rbd_obj_request_create(object_name,
>>                           offset, length, type);
>> -        kfree(object_name);    /* object request has its own copy */
>> +        /* object request has its own copy of the object name */
>> +        rbd_segment_name_free(object_name);
>>           if (!obj_request)
>>               goto out_unwind;
>>
>> @@ -5018,17 +5029,32 @@ static int rbd_slab_init(void)
>>                       sizeof (struct rbd_obj_request),
>>                       __alignof__(struct rbd_obj_request),
>>                       0, NULL);
>> -    if (rbd_obj_request_cache)
>> -        return 0;
>> +    if (!rbd_obj_request_cache)
>> +        goto out_err;
>>
>> +    rbd_assert(!rbd_segment_name_cache);
>> +    rbd_segment_name_cache = kmem_cache_create("rbd_segment_name",
>> +                    MAX_OBJ_NAME_SIZE + 1, 1, 0, NULL);
>> +    if (rbd_segment_name_cache)
>> +        return 0;
>> +out_err:
>>       kmem_cache_destroy(rbd_img_request_cache);
>>       rbd_img_request_cache = NULL;
>>
>> +    if (rbd_img_request_cache) {
>> +        kmem_cache_destroy(rbd_obj_request_cache);
>> +        rbd_img_request_cache = NULL;
> 
> Should be rbd_obj_request_cache here and in the condition two lines up.
> With that fixed: Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

Got it, I'll fix that, and I'll also fix that GFP
mixup in the previous one.  Thanks for the review.

					-Alex

>> +    }
>> +
>>       return -ENOMEM;
>>   }
>>
>>   static void rbd_slab_exit(void)
>>   {
>> +    rbd_assert(rbd_segment_name_cache);
>> +    kmem_cache_destroy(rbd_segment_name_cache);
>> +    rbd_segment_name_cache = NULL;
>> +
>>       rbd_assert(rbd_obj_request_cache);
>>       kmem_cache_destroy(rbd_obj_request_cache);
>>       rbd_obj_request_cache = NULL;
>>
>