Re: [Qemu-devel] [RFC PATCH 3/3] block: gluster as block backend

[Bharata B Rao]

On Mon, Jun 18, 2012 at 06:35:28PM +0100, Stefan Hajnoczi wrote:
> On Mon, Jun 11, 2012 at 3:21 PM, Bharata B Rao
> <address@hidden> wrote:
> > +#include "block_int.h"
> > +#include "gluster-helpers.h"
> > +
> > +typedef void *gluster_file_t;
> 
> This typedef is already in gluster-helpers.h.

Yes, will fix that.

> It's ugly BTW, "typedef
> struct gluster_file gluster_file_t" is nicer since it won't cast to
> other pointer types automatically.

Gluster routines in libglusterfsclient operate on gluster specific descriptor
called fd_t.

glusterfs_open returns a pointer to fd_t and rest of the read/write routines
take that pointer as input. libglusterfsclient hides this pointer by doing

typedef void *glusterfs_file_t.

I wanted to return an integer fd from open and then use them with read and
write. But that would need some code in gluster backend to convert integer
fd to fd_t and vice versa. Since libglusterfsclient doesn't deal with integer
fd's, I retained this ugly typedef.

> 
> > +
> > +typedef struct glusterConf {
> > +    char volfile[PATH_MAX];
> > +    char image[PATH_MAX];
> > +} glusterConf;
> 
> QEMU coding style always uses UpperCase for struct names.

Ok, will fix.

> 
> > +static void qemu_gluster_aio_event_reader(void *opaque)
> > +{
> > +    BDRVGlusterState *s = opaque;
> > +    ssize_t ret;
> > +
> > +    do {
> > +        char *p = (char *)&s->event_gaiocb;
> 
> Why make this a BDRVGlusterState field?  It could be a local, I think.

I could I guess, I was just following what rbd does.

> 
> > +    /* Use O_DSYNC for write-through caching, no flags for write-back 
> > caching,
> > +     * and O_DIRECT for no caching. */
> > +    if ((bdrv_flags & BDRV_O_NOCACHE))
> > +        s->open_flags |= O_DIRECT;
> > +    if (!(bdrv_flags & BDRV_O_CACHE_WB))
> > +        s->open_flags |= O_DSYNC;
> 
> Paolo has changed this recently, you might need to use
> bs->enable_write_cache instead.

I picked up this logic from block/raw-posix.c:raw_open_common(). Don't see
anything related to bs->enable_write_cache there. Will find out more about
bs->enable_write_cache.

> 
> > +out:
> > +    if (c) {
> > +        g_free(c);
> > +    }
> 
> g_free(NULL) is a nop, you never need to test that the pointer is non-NULL.

Ok.

> 
> > +static void gluster_finish_aiocb(void *arg)
> > +{
> > +    int ret;
> > +    gluster_aiocb_t *gaiocb = (gluster_aiocb_t *)arg;
> > +    BDRVGlusterState *s = ((glusterAIOCB *)gaiocb->opaque)->s;
> > +
> > +    ret = qemu_gluster_send_pipe(s, gaiocb);
> > +    if (ret < 0) {
> > +        g_free(gaiocb);
> 
> What about the glusterAIOCB?  You need to invoke the callback with an
> error value.
> 
> What about decrementing the in-flight I/O request count?

Again, this comes from rbd. gluster_finish_aiocb() is the callback
that we have registered with gluster. I am not doing any error handling when
we even fail to write to the pipe. An even reader would be waiting to read
from the other end of the pipe. Typically error handling and decrementing
the in-flight IO request count is done by that event reader. But in this
case, we even failed to kick (via pipe write) the even reader.

> 
> > +static BlockDriverAIOCB *qemu_gluster_aio_rw(BlockDriverState *bs,
> > +        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> > +        BlockDriverCompletionFunc *cb, void *opaque, int write)
> > +{
> > +    int ret;
> > +    glusterAIOCB *acb;
> > +    gluster_aiocb_t *gaiocb;
> > +    BDRVGlusterState *s = bs->opaque;
> > +    char *buf;
> > +    size_t size;
> > +    off_t offset;
> > +
> > +    acb = qemu_aio_get(&gluster_aio_pool, bs, cb, opaque);
> > +    acb->write = write;
> > +    acb->qiov = qiov;
> > +    acb->bounce = qemu_blockalign(bs, qiov->size);
> > +    acb->ret = 0;
> > +    acb->bh = NULL;
> > +    acb->s = s;
> > +
> > +    if (write) {
> > +        qemu_iovec_to_buffer(acb->qiov, acb->bounce);
> > +    }
> > +
> > +    buf = acb->bounce;
> > +    offset = sector_num * BDRV_SECTOR_SIZE;
> > +    size = nb_sectors * BDRV_SECTOR_SIZE;
> > +    s->qemu_aio_count++;
> > +
> > +    gaiocb = g_malloc(sizeof(gluster_aiocb_t));
> 
> Can you make this a field of glusterAIOCB?  Then you don't need to
> worry about freeing gaiocb later.

Hmm, I already have glusterAIOCB as part of gaiocb.

> 
> > +static int64_t qemu_gluster_getlength(BlockDriverState *bs)
> > +{
> > +    BDRVGlusterState *s = bs->opaque;
> > +    gluster_file_t fd = s->fd;
> > +    struct stat st;
> > +    int ret;
> > +
> > +    ret = gluster_fstat(fd, &st);
> > +    if (ret < 0) {
> > +        return -1;
> 
> Please return a negative errno instead of -1.

Ok. May be I could just return value from gluster_fstat().

Thanks for your review.

Regards,
Bharata.