lkml.org 
[lkml]   [2009]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 04/21] ceph: ref counted buffer
    On Tue, 22 Sep 2009 10:38:32 -0700
    Sage Weil <sage@newdream.net> 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@newdream.net>
    > ---
    > 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?



    \
     
     \ /
      Last update: 2009-09-30 02:05    [W:0.026 / U:2.272 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site