From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6kf5-0000lW-TA for qemu-devel@nongnu.org; Tue, 15 Nov 2016 15:49:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6kf2-0003Ns-Nw for qemu-devel@nongnu.org; Tue, 15 Nov 2016 15:48:59 -0500 Received: from mail-wm0-x244.google.com ([2a00:1450:400c:c09::244]:36162) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c6kf2-0003NC-Bv for qemu-devel@nongnu.org; Tue, 15 Nov 2016 15:48:56 -0500 Received: by mail-wm0-x244.google.com with SMTP id m203so3988807wma.3 for ; Tue, 15 Nov 2016 12:48:56 -0800 (PST) Date: Tue, 15 Nov 2016 20:48:52 +0000 From: Stefan Hajnoczi Message-ID: <20161115204852.GA6784@stefanha-x1.localdomain> References: <1471149312-28148-1-git-send-email-ashish.mittal@veritas.com> <20160815102046.GC13261@redhat.com> <23ea8348-7ed9-7e45-1079-235614dc90c2@redhat.com> <20160823215803.GA24259@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DocE+STaALJfprDB" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: ashish mittal Cc: Paolo Bonzini , "Daniel P. Berrange" , qemu-devel , Kevin Wolf , Markus Armbruster , Ashish Mittal , Ketan Nilangekar , Abhijit Dey , Jeff Cody , Buddhi.Madhav@veritas.com, Venkatesha.Mg@veritas.com, Rakesh Ranjan --DocE+STaALJfprDB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 15, 2016 at 11:02:34AM -0800, ashish mittal wrote: > I had replied to the QEMUBH suggestion in the email below. >=20 > Regards, > Ashish >=20 > On Tue, Aug 23, 2016 at 3:22 PM, ashish mittal wrot= e: > > 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. > > /=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D/ > > 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. > > /=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D/ > > > > 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 . ---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[] =3D "/dev/of/vdisk"; =20 +static void vxhs_complete_aio_bh(void *opaque) +{ + VXHSAIOCB *acb =3D opaque; + BlockCompletionFunc *cb =3D acb->common.cb; + void *cb_opaque =3D acb->common.opaque; + int ret =3D 0; + + if (acb->err !=3D 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 =3D (-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 =3D NULL; - BDRVVXHSState *s =3D NULL; - ssize_t ret; =20 switch (opcode) { case IRP_READ_REQUEST: @@ -91,7 +109,6 @@ static void vxhs_iio_callback(int32_t rfd, uint32_t reas= on, void *ctx, */ if (ctx) { acb =3D ctx; - s =3D 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 rea= son, void *ctx, trace_vxhs_iio_callback(error, reason); } =20 - ret =3D qemu_write_full(s->fds[VDISK_FD_WRITE], &acb, sizeof(acb)); - g_assert(ret =3D=3D sizeof(acb)); + aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs), + vxhs_complete_aio_bh, acb); break; =20 default: @@ -223,53 +240,6 @@ static void vxhs_qnio_iio_close(BDRVVXHSState *s) vxhs_qnio_close(); } =20 -static void vxhs_complete_aio(VXHSAIOCB *acb, BDRVVXHSState *s) -{ - BlockCompletionFunc *cb =3D acb->common.cb; - void *opaque =3D acb->common.opaque; - int ret =3D 0; - - if (acb->err !=3D 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 =3D (-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 =3D opaque; - char *p; - ssize_t ret; - - do { - p =3D (char *)&s->qnio_event_acb; - ret =3D 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 +=3D ret; - if (s->event_reader_pos =3D=3D sizeof(s->qnio_event_acb)) { - s->event_reader_pos =3D 0; - vxhs_complete_aio(s->qnio_event_acb, s); - } - } - } while (ret < 0 && errno =3D=3D EINTR); -} - static QemuOptsList runtime_opts =3D { .name =3D "vxhs", .head =3D QTAILQ_HEAD_INITIALIZER(runtime_opts.head), @@ -468,7 +438,6 @@ static int vxhs_open(BlockDriverState *bs, QDict *optio= ns, int bdrv_flags, Error **errp) { BDRVVXHSState *s =3D bs->opaque; - AioContext *aio_context; int qemu_qnio_cfd =3D -1; int qemu_rfd =3D -1; int ret =3D 0; @@ -481,32 +450,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *opti= ons, =20 s->vdisk_hostinfo.qnio_cfd =3D qemu_qnio_cfd; s->vdisk_hostinfo.vdisk_rfd =3D 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 =3D qemu_pipe(s->fds); - if (ret < 0) { - trace_vxhs_open_epipe(ret); - ret =3D -errno; - goto errout; - } - fcntl(s->fds[VDISK_FD_READ], F_SETFL, O_NONBLOCK); - - aio_context =3D 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; } =20 static const AIOCBInfo vxhs_aiocb_info =3D { @@ -596,14 +540,7 @@ static void vxhs_close(BlockDriverState *bs) BDRVVXHSState *s =3D bs->opaque; =20 trace_vxhs_close(s->vdisk_guid); - close(s->fds[VDISK_FD_READ]); - close(s->fds[VDISK_FD_WRITE]); =20 - /* - * 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 =3D NULL; vxhs_qnio_iio_close(s); @@ -650,23 +587,6 @@ static int64_t vxhs_getlength(BlockDriverState *bs) return vdisk_size; } =20 -static void vxhs_detach_aio_context(BlockDriverState *bs) -{ - BDRVVXHSState *s =3D 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 =3D bs->opaque; - - aio_set_fd_handler(new_context, s->fds[VDISK_FD_READ], - false, vxhs_aio_event_reader, NULL, s); -} - static BlockDriver bdrv_vxhs =3D { .format_name =3D "vxhs", .protocol_name =3D "vxhs", @@ -677,8 +597,6 @@ static BlockDriver bdrv_vxhs =3D { .bdrv_getlength =3D vxhs_getlength, .bdrv_aio_readv =3D vxhs_aio_readv, .bdrv_aio_writev =3D vxhs_aio_writev, - .bdrv_detach_aio_context =3D vxhs_detach_aio_context, - .bdrv_attach_aio_context =3D vxhs_attach_aio_context, }; =20 static void bdrv_vxhs_init(void) --DocE+STaALJfprDB Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJYK3S0AAoJEJykq7OBq3PIPZ8H/j6VGbuXSFTy/g3EVqkuKjfO K/JnIWzMs1Q7P8dkGbpNcUX5hssRs1vfNJjPtf3zv/Wi69gaqahZHhOAH5Cv4cG6 NPb2e31ZJleMs2JEKLz+r8bcc9nePzT5ln90uo1OYJrGmxKsgOabS5ZNNLSr4mPl ZS5/lq7J2bJEHJbXcrwTVA5PLyCppcheRXuEMcwb8Jmz7g02g+PuOl0rhF7f1Wu1 /PLoviOaHmJaSCwNO+1k5ndgwc37Bi38bsbGWTt+nAiMjfsyIXAI9QMZZkkhCOLL WhySDoI8R+0zxme8jQ8gF4BRO3L8l9GoE/pHJFV4rffvkmuE/UDyvyQv3uWGzbc= =IwIG -----END PGP SIGNATURE----- --DocE+STaALJfprDB--