On Tue, 22 Sep 2009 10:38:32 -0700
Sage Weil <sage@xxxxxxxxxxxx> wrote:
> struct ceph_buffer is a simple ref-counted buffer. We transparently
> choose between kmalloc for small buffers and vmalloc for large ones.
>
> This is used for allocating memory for xattr data, among other things.
>
> Signed-off-by: Sage Weil <sage@xxxxxxxxxxxx>
> ---
> fs/ceph/buffer.h | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 83 insertions(+), 0 deletions(-)
> create mode 100644 fs/ceph/buffer.h
>
> diff --git a/fs/ceph/buffer.h b/fs/ceph/buffer.h
> new file mode 100644
> index 0000000..128593d
> --- /dev/null
> +++ b/fs/ceph/buffer.h
> @@ -0,0 +1,83 @@
> +#ifndef __FS_CEPH_BUFFER_H
> +#define __FS_CEPH_BUFFER_H
> +
> +#include <linux/mm.h>
> +#include <linux/types.h>
> +#include <linux/vmalloc.h>
> +
> +#include "ceph_debug.h"
> +
> +/*
> + * a simple reference counted buffer.
> + *
> + * use kmalloc for small sizes (<= one page), vmalloc for larger
> + * sizes.
> + */
> +struct ceph_buffer {
> + atomic_t nref;
> + struct kvec vec;
> + size_t alloc_len;
> + bool is_vmalloc;
> +};
vmalloc is a concern. It is vulnerable to (and can cause) internal
fragmentation. One that occurs, it's as good as a full machine
failure.
> +static inline struct ceph_buffer *ceph_buffer_new(gfp_t gfp)
> +{
> + struct ceph_buffer *b;
> +
> + b = kmalloc(sizeof(*b), gfp);
> + if (!b)
> + return NULL;
> + atomic_set(&b->nref, 1);
> + b->vec.iov_base = NULL;
> + b->vec.iov_len = 0;
> + b->alloc_len = 0;
> + return b;
> +}
I was going to stop commenting on all the nutty inlining decisions but gee.
> +static inline int ceph_buffer_alloc(struct ceph_buffer *b, int len, gfp_t gfp)
> +{
> + if (len <= PAGE_SIZE) {
> + b->vec.iov_base = kmalloc(len, gfp);
> + b->is_vmalloc = false;
> + } else {
> + b->vec.iov_base = __vmalloc(len, gfp, PAGE_KERNEL);
> + b->is_vmalloc = true;
> + }
> + if (!b->vec.iov_base)
> + return -ENOMEM;
> + b->alloc_len = len;
> + b->vec.iov_len = len;
> + return 0;
> +}
Do we *really* need vmalloc here? It much be one humongous vector!
How large can it really get?
A still-lame-but-less-lame option here would be to attempt the kmalloc
(with __GFP_NOWARN) and if it failed, fall back to vmalloc.
>
> ...
>
> +static inline void ceph_buffer_put(struct ceph_buffer *b)
> +{
> + if (b && atomic_dec_and_test(&b->nref)) {
> + if (b->vec.iov_base) {
> + if (b->is_vmalloc)
> + vfree(b->vec.iov_base);
> + else
> + kfree(b->vec.iov_base);
> + }
> + kfree(b);
> + }
> +}
> +
> +static inline struct ceph_buffer *ceph_buffer_new_alloc(int len, gfp_t gfp)
> +{
> + struct ceph_buffer *b = ceph_buffer_new(gfp);
> +
> + if (b && ceph_buffer_alloc(b, len, gfp) < 0) {
> + ceph_buffer_put(b);
> + b = NULL;
> + }
> + return b;
> +}
Do we really need to test for b==NULL here? Is that test potentially
hiding bugs in calling code?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
