[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 RFC] block/vxhs: Initial commit to add Verita
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support |
Date: |
Tue, 15 Nov 2016 20:48:52 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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 <address@hidden> 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 <address@hidden>.
---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)
signature.asc
Description: PGP signature