Thank you. Will work with this.
On Tue, Nov 15, 2016 at 12:48 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 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 <ashmit...@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 <stefa...@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)
