Re: [Qemu-devel] [PATCH v3 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8479 bytes --]

On Tue, Nov 15, 2016 at 11:02:34AM -0800, ashish mittal wrote:
> I had replied to the QEMUBH suggestion in the email below.
> 
> Regards,
> Ashish
> 
> On Tue, Aug 23, 2016 at 3:22 PM, ashish mittal <ashmit602@gmail.com> wrote:
> > Thanks Stefan, I will look at block/quorum.c.
> >
> > I have sent V4 of the patch with a reworked parsing logic for both
> > JSON and URI. Both of them are quite compact now.
> >
> > URI parsing now follows the suggestion given by Kevin.
> > /================/
> > However, you should use the proper interfaces to implement this, which
> > is .bdrv_parse_filename(). This is a function that gets a string and
> > converts it into a QDict, which is then passed to .bdrv_open(). So it
> > uses exactly the same code path in .bdrv_open() as if used directly with
> > QAPI.
> > /================/
> >
> > Additionally, I have fixed all the issues pointed out by you on V1 of
> > the patch. The only change I haven't done is to replace pipes with
> > QEMUBH. I am hoping this will not hold up the patch from being
> > accepted, and I can make this transition later with proper dev and
> > testing.

Sorry, I forgot about this email.  I still think the QEMUBH approach
makes sense.  Please try the following patch.

I have compiled but not run it.  You are welcome to squash it into your
next patch.

The following is Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>.

---8<---

diff --git a/block/vxhs.c b/block/vxhs.c
index 8913e8f..22fd989 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -60,9 +60,6 @@ typedef struct VXHSvDiskHostsInfo {
  * Structure per vDisk maintained for state
  */
 typedef struct BDRVVXHSState {
-    int fds[2];
-    int event_reader_pos;
-    VXHSAIOCB *qnio_event_acb;
     VXHSvDiskHostsInfo vdisk_hostinfo; /* Per host info */
     char *vdisk_guid;
 } BDRVVXHSState;
@@ -73,12 +70,33 @@ static QNIOLibState qniolib;
 /* vdisk prefix to pass to qnio */
 static const char vdisk_prefix[] = "/dev/of/vdisk";
 
+static void vxhs_complete_aio_bh(void *opaque)
+{
+    VXHSAIOCB *acb = opaque;
+    BlockCompletionFunc *cb = acb->common.cb;
+    void *cb_opaque = acb->common.opaque;
+    int ret = 0;
+
+    if (acb->err != 0) {
+        trace_vxhs_complete_aio(acb, acb->err);
+        /*
+         * We mask all the IO errors generically as EIO for upper layers
+         * Right now our IO Manager uses non standard error codes. Instead
+         * of confusing upper layers with incorrect interpretation we are
+         * doing this workaround.
+         */
+        ret = (-EIO);
+    }
+
+    qemu_aio_unref(acb);
+    cb(cb_opaque, ret);
+}
+
+/* Called from a libqnio thread */
 static void vxhs_iio_callback(int32_t rfd, uint32_t reason, void *ctx,
                               uint32_t error, uint32_t opcode)
 {
     VXHSAIOCB *acb = NULL;
-    BDRVVXHSState *s = NULL;
-    ssize_t ret;
 
     switch (opcode) {
     case IRP_READ_REQUEST:
@@ -91,7 +109,6 @@ static void vxhs_iio_callback(int32_t rfd, uint32_t reason, void *ctx,
          */
         if (ctx) {
             acb = ctx;
-            s = acb->common.bs->opaque;
         } else {
             trace_vxhs_iio_callback(error, reason);
             goto out;
@@ -104,8 +121,8 @@ static void vxhs_iio_callback(int32_t rfd, uint32_t reason, void *ctx,
             trace_vxhs_iio_callback(error, reason);
         }
 
-        ret = qemu_write_full(s->fds[VDISK_FD_WRITE], &acb, sizeof(acb));
-        g_assert(ret == sizeof(acb));
+        aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
+                                vxhs_complete_aio_bh, acb);
         break;
 
     default:
@@ -223,53 +240,6 @@ static void vxhs_qnio_iio_close(BDRVVXHSState *s)
     vxhs_qnio_close();
 }
 
-static void vxhs_complete_aio(VXHSAIOCB *acb, BDRVVXHSState *s)
-{
-    BlockCompletionFunc *cb = acb->common.cb;
-    void *opaque = acb->common.opaque;
-    int ret = 0;
-
-    if (acb->err != 0) {
-        trace_vxhs_complete_aio(acb, acb->err);
-        /*
-         * We mask all the IO errors generically as EIO for upper layers
-         * Right now our IO Manager uses non standard error codes. Instead
-         * of confusing upper layers with incorrect interpretation we are
-         * doing this workaround.
-         */
-        ret = (-EIO);
-    }
-
-    qemu_aio_unref(acb);
-    cb(opaque, ret);
-}
-
-/*
- * This is the HyperScale event handler registered to QEMU.
- * It is invoked when any IO gets completed and written on pipe
- * by callback called from QNIO thread context. Then it marks
- * the AIO as completed, and releases HyperScale AIO callbacks.
- */
-static void vxhs_aio_event_reader(void *opaque)
-{
-    BDRVVXHSState *s = opaque;
-    char *p;
-    ssize_t ret;
-
-    do {
-        p = (char *)&s->qnio_event_acb;
-        ret = read(s->fds[VDISK_FD_READ], p + s->event_reader_pos,
-                   sizeof(s->qnio_event_acb) - s->event_reader_pos);
-        if (ret > 0) {
-            s->event_reader_pos += ret;
-            if (s->event_reader_pos == sizeof(s->qnio_event_acb)) {
-                s->event_reader_pos = 0;
-                vxhs_complete_aio(s->qnio_event_acb, s);
-            }
-        }
-    } while (ret < 0 && errno == EINTR);
-}
-
 static QemuOptsList runtime_opts = {
     .name = "vxhs",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -468,7 +438,6 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
                      int bdrv_flags, Error **errp)
 {
     BDRVVXHSState *s = bs->opaque;
-    AioContext *aio_context;
     int qemu_qnio_cfd = -1;
     int qemu_rfd = -1;
     int ret = 0;
@@ -481,32 +450,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
 
     s->vdisk_hostinfo.qnio_cfd = qemu_qnio_cfd;
     s->vdisk_hostinfo.vdisk_rfd = qemu_rfd;
-
-    /*
-     * Create a pipe for communicating between two threads in different
-     * context. Set handler for read event, which gets triggered when
-     * IO completion is done by non-QEMU context.
-     */
-    ret = qemu_pipe(s->fds);
-    if (ret < 0) {
-        trace_vxhs_open_epipe(ret);
-        ret = -errno;
-        goto errout;
-    }
-    fcntl(s->fds[VDISK_FD_READ], F_SETFL, O_NONBLOCK);
-
-    aio_context = bdrv_get_aio_context(bs);
-    aio_set_fd_handler(aio_context, s->fds[VDISK_FD_READ],
-                       false, vxhs_aio_event_reader, NULL, s);
     return 0;
-
-errout:
-    /*
-     * Close remote vDisk device if it was opened earlier
-     */
-    vxhs_qnio_iio_close(s);
-    trace_vxhs_open_fail(ret);
-    return ret;
 }
 
 static const AIOCBInfo vxhs_aiocb_info = {
@@ -596,14 +540,7 @@ static void vxhs_close(BlockDriverState *bs)
     BDRVVXHSState *s = bs->opaque;
 
     trace_vxhs_close(s->vdisk_guid);
-    close(s->fds[VDISK_FD_READ]);
-    close(s->fds[VDISK_FD_WRITE]);
 
-    /*
-     * Clearing all the event handlers for oflame registered to QEMU
-     */
-    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fds[VDISK_FD_READ],
-                       false, NULL, NULL, NULL);
     g_free(s->vdisk_guid);
     s->vdisk_guid = NULL;
     vxhs_qnio_iio_close(s);
@@ -650,23 +587,6 @@ static int64_t vxhs_getlength(BlockDriverState *bs)
     return vdisk_size;
 }
 
-static void vxhs_detach_aio_context(BlockDriverState *bs)
-{
-    BDRVVXHSState *s = bs->opaque;
-
-    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fds[VDISK_FD_READ],
-                       false, NULL, NULL, NULL);
-}
-
-static void vxhs_attach_aio_context(BlockDriverState *bs,
-                                   AioContext *new_context)
-{
-    BDRVVXHSState *s = bs->opaque;
-
-    aio_set_fd_handler(new_context, s->fds[VDISK_FD_READ],
-                       false, vxhs_aio_event_reader, NULL, s);
-}
-
 static BlockDriver bdrv_vxhs = {
     .format_name                  = "vxhs",
     .protocol_name                = "vxhs",
@@ -677,8 +597,6 @@ static BlockDriver bdrv_vxhs = {
     .bdrv_getlength               = vxhs_getlength,
     .bdrv_aio_readv               = vxhs_aio_readv,
     .bdrv_aio_writev              = vxhs_aio_writev,
-    .bdrv_detach_aio_context      = vxhs_detach_aio_context,
-    .bdrv_attach_aio_context      = vxhs_attach_aio_context,
 };
 
 static void bdrv_vxhs_init(void)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]