From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45571) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sgukk-0001SK-8c for qemu-devel@nongnu.org; Tue, 19 Jun 2012 05:29:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sgukd-0002sU-O9 for qemu-devel@nongnu.org; Tue, 19 Jun 2012 05:29:37 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:43312) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sgukc-0002rd-V2 for qemu-devel@nongnu.org; Tue, 19 Jun 2012 05:29:31 -0400 Received: from /spool/local by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 19 Jun 2012 09:09:59 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q5J9LroS55836708 for ; Tue, 19 Jun 2012 19:21:54 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q5J9TJCj021194 for ; Tue, 19 Jun 2012 19:29:20 +1000 Date: Tue, 19 Jun 2012 15:00:23 +0530 From: Bharata B Rao Message-ID: <20120619093023.GC27963@in.ibm.com> References: <20120611141806.GA2737@in.ibm.com> <20120611142144.GD2737@in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Qemu-devel] [RFC PATCH 3/3] block: gluster as block backend Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Paolo Bonzini , Amar Tumballi , qemu-devel@nongnu.org, Vijay Bellur 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 > 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.