From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpCny-0004nJ-L1 for qemu-devel@nongnu.org; Wed, 28 Sep 2016 07:13:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpCnv-0000S1-Er for qemu-devel@nongnu.org; Wed, 28 Sep 2016 07:13:38 -0400 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]:35540) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpCnv-0000Rq-3v for qemu-devel@nongnu.org; Wed, 28 Sep 2016 07:13:35 -0400 Received: by mail-wm0-x233.google.com with SMTP id l132so229692592wmf.0 for ; Wed, 28 Sep 2016 04:13:35 -0700 (PDT) Date: Wed, 28 Sep 2016 12:13:32 +0100 From: Stefan Hajnoczi Message-ID: <20160928111332.GD4196@stefanha-x1.localdomain> References: <1475035789-685-1-git-send-email-ashish.mittal@veritas.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zS7rBR6csb6tI2e1" Content-Disposition: inline In-Reply-To: <1475035789-685-1-git-send-email-ashish.mittal@veritas.com> Subject: Re: [Qemu-devel] [PATCH v7 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: qemu-devel@nongnu.org, pbonzini@redhat.com, kwolf@redhat.com, armbru@redhat.com, berrange@redhat.com, jcody@redhat.com, famz@redhat.com, ashish.mittal@veritas.com, Ketan.Nilangekar@veritas.com, Abhijit.Dey@veritas.com --zS7rBR6csb6tI2e1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Sep 27, 2016 at 09:09:49PM -0700, Ashish Mittal wrote: > +vxhs_bdrv_init(const char c) "Registering VxHS AIO driver%c" Why do several trace events have a %c format specifier at the end and it always takes a '.' value? > +#define QNIO_CONNECT_TIMOUT_SECS 120 This isn't used and there is a typo (s/TIMOUT/TIMEOUT/). Can it be dropped? > +static int32_t > +vxhs_qnio_iio_ioctl(void *apictx, uint32_t rfd, uint32_t opcode, int64_t *in, > + void *ctx, uint32_t flags) > +{ > + int ret = 0; > + > + switch (opcode) { > + case VDISK_STAT: It seems unnecessary to abstract the iio_ioctl() constants and then have a switch statement to translate to the actual library constants. It makes little sense since the flags argument already uses the library constants. Just use the library's constants. > + ret = iio_ioctl(apictx, rfd, IOR_VDISK_STAT, > + in, ctx, flags); > + break; > + > + case VDISK_AIO_FLUSH: > + ret = iio_ioctl(apictx, rfd, IOR_VDISK_FLUSH, > + in, ctx, flags); > + break; > + > + case VDISK_CHECK_IO_FAILOVER_READY: > + ret = iio_ioctl(apictx, rfd, IOR_VDISK_CHECK_IO_FAILOVER_READY, > + in, ctx, flags); > + break; > + > + default: > + ret = -ENOTSUP; > + break; > + } > + > + if (ret) { > + *in = 0; Some callers pass in = NULL so this will crash. The naming seems wrong: this is an output argument, not an input argument. Please call it "out_val" or similar. > + res = vxhs_reopen_vdisk(s, s->vdisk_ask_failover_idx); > + if (res == 0) { > + res = vxhs_qnio_iio_ioctl(s->qnio_ctx, > + s->vdisk_hostinfo[s->vdisk_ask_failover_idx].vdisk_rfd, > + VDISK_CHECK_IO_FAILOVER_READY, NULL, s, flags); Looking at iio_ioctl(), I'm not sure how this can ever work. The fourth argument is NULL and iio_ioctl() will attempt *vdisk_size = 0 so this will crash. Do you have tests that exercise this code path? > +/* > + * This is called by QEMU when a flush gets triggered from within > + * a guest at the block layer, either for IDE or SCSI disks. > + */ > +static int vxhs_co_flush(BlockDriverState *bs) This is called from coroutine context, please add the coroutine_fn function attribute to document this. > +{ > + BDRVVXHSState *s = bs->opaque; > + int64_t size = 0; > + int ret = 0; > + > + /* > + * VDISK_AIO_FLUSH ioctl is a no-op at present and will > + * always return success. This could change in the future. > + */ > + ret = vxhs_qnio_iio_ioctl(s->qnio_ctx, > + s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd, > + VDISK_AIO_FLUSH, &size, NULL, IIO_FLAG_SYNC); This function is not allowed to block. It cannot do a synchronous flush. This line is misleading because the constant is called VDISK_AIO_FLUSH, but looking at the library code I see it's actually a synchronous call that ends up in a loop that sleeps (!) waiting for the response. Please do an async flush and qemu_coroutine_yield() to return control to QEMU's event loop. When the flush completes you can qemu_coroutine_enter() again to return from this function. > + > + if (ret < 0) { > + trace_vxhs_co_flush(s->vdisk_guid, ret, errno); > + vxhs_close(bs); This looks unsafe. Won't it cause double close() calls for s->fds[] when bdrv_close() is called later? > + } > + > + return ret; > +} > + > +static unsigned long vxhs_get_vdisk_stat(BDRVVXHSState *s) sizeof(unsigned long) = 4 on some machines, please change it to int64_t. I also suggest changing the function name to vxhs_get_vdisk_size() since it only provides the size. > +static int64_t vxhs_get_allocated_blocks(BlockDriverState *bs) > +{ > + BDRVVXHSState *s = bs->opaque; > + int64_t vdisk_size = 0; > + > + if (s->vdisk_size > 0) { > + vdisk_size = s->vdisk_size; > + } else { > + /* > + * TODO: > + * Once HyperScale storage-virtualizer provides > + * actual physical allocation of blocks then > + * fetch that information and return back to the > + * caller but for now just get the full size. > + */ > + vdisk_size = vxhs_get_vdisk_stat(s); > + if (vdisk_size > 0) { > + s->vdisk_size = vdisk_size; > + } > + } > + > + if (vdisk_size > 0) { > + return vdisk_size; /* return size in bytes */ > + } > + > + return -EIO; > +} Why are you implementing this function if vxhs doesn't support querying the allocated file size? Don't return a bogus number. Just don't implement it like other block drivers. --zS7rBR6csb6tI2e1 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJX66XcAAoJEJykq7OBq3PIhzUIAIU0aFesJdOiivFav1wqALb0 loaZ1UOy36ClC8MibHB3LgQzqtSswkXQA7om5Wio11KxOa7CfuoEcUFJgZRloVoZ F7LeJIEpCuF4btMp31WZSenRyRJZ6Cd6Vk5QLpGZCtfoPdX+qwB63hHdjnXNOxy1 0Y3mYr6Atkzbs7k+qTfYU+sVQRCUu4aYTX7J8v4fh5NLcKpJEcKEt7Z4qxBrazxI yRcRWFgknMwFAYE/bDfy8lI3kanSORvb/RpYJuV8uMKFNOlc8O6d8buN/AjQygMI Q6ugLeMDKdJ2qHRCxzgQn+KbwrHkA96+gNl6WWFwiB2UP/cXToEsvN47xwPAUHc= =rPsP -----END PGP SIGNATURE----- --zS7rBR6csb6tI2e1--