RE: [PATCH 1/3] engines/xnvme: add xnvme engine

From: Vincent Fu <vincent.fu@samsung.com>
To: Ankit Kumar <ankit.kumar@samsung.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>
Cc: "fio@vger.kernel.org" <fio@vger.kernel.org>,
	"krish.reddy@samsung.com" <krish.reddy@samsung.com>,
	"simon.lund@samsung.com" <simon.lund@samsung.com>
Subject: RE: [PATCH 1/3] engines/xnvme: add xnvme engine
Date: Fri, 6 May 2022 16:55:31 +0000	[thread overview]
Message-ID: <c57c33726a334fb5a49100f40fcce016@samsung.com> (raw)
In-Reply-To: <20220505131935.32076-2-ankit.kumar@samsung.com>

> -----Original Message-----
> From: Ankit Kumar [mailto:ankit.kumar@samsung.com]
> Sent: Thursday, May 5, 2022 9:20 AM
> To: axboe@kernel.dk
> Cc: fio@vger.kernel.org; krish.reddy@samsung.com;
> simon.lund@samsung.com; Ankit Kumar <ankit.kumar@samsung.com>
> Subject: [PATCH 1/3] engines/xnvme: add xnvme engine
> 
> This patch introduces a new fio engine to work with xNVMe >= 0.2.0.
> xNVMe provides a user space library (libxnvme) to work with NVMe
> devices. The NVMe driver being used by libxnvme is re-targetable and
> can be any one of the GNU/Linux Kernel NVMe driver via libaio,
> IOCTLs, io_uring, the SPDK NVMe driver, or your own custom NVMe
> driver.
> 
> For more info visit
> https://urldefense.com/v3/__https://protect2.fireeye.com/v1/url?k=ad
> 739a68-ccf88f52-ad721127-74fe4860008a-
> e490d5c8a92d072c&q=1&e=07140046-8b99-485a-a6cf-
> 0c1685707a67&u=https*3A*2F*2Fxnvme.io*2F__;JSUlJQ!!EwVzqGoTKB
> qv-
> 0DWAJBm!ChkUAg8V4FGdLAgw54MUDD8PRpVmo1RBeReZpfMKxWz8a
> DMsUtG1GYPPc9rhJot4-Vw9$
> https://urldefense.com/v3/__https://protect2.fireeye.com/v1/url?k=4f
> 9d8ae4-2e169fde-4f9c01ab-74fe4860008a-
> b5cd486f43ed047d&q=1&e=07140046-8b99-485a-a6cf-
> 0c1685707a67&u=https*3A*2F*2Fgithub.com*2FOpenMPDK*2FxNVMe
> __;JSUlJSU!!EwVzqGoTKBqv-
> 0DWAJBm!ChkUAg8V4FGdLAgw54MUDD8PRpVmo1RBeReZpfMKxWz8a
> DMsUtG1GYPPc9rhJqXGQ87L$
> 
> Co-Authored-By: Ankit Kumar <ankit.kumar@samsung.com>
> Co-Authored-By: Simon A. F. Lund <simon.lund@samsung.com>
> Co-Authored-By: Mads Ynddal <m.ynddal@samsung.com>
> Co-Authored-By: Michael Bang <mi.bang@samsung.com>
> Co-Authored-By: Karl Bonde Torp <k.torp@samsung.com>
> Co-Authored-By: Gurmeet Singh <gur.singh@samsung.com>
> Co-Authored-By: Pierre Labat <plabat@micron.com>
> ---

<snip>

Do we need an SPDX license identifier at the top of xnvme.c?

> +++ b/engines/xnvme.c
> @@ -0,0 +1,1000 @@
> +/*
> + * fio xNVMe IO Engine
> + *
> + * IO engine using the xNVMe C API.
> + *

<snip>

> +static void xnvme_fioe_cleanup(struct thread_data *td)
> +{
> +	struct xnvme_fioe_data *xd = td->io_ops_data;
> +	int err;
> +
> +	err = pthread_mutex_lock(&g_serialize);
> +	if (err)
> +		XNVME_DEBUG("FAILED: pthread_mutex_lock(),
> err: %d", err);
> +		/* NOTE: not returning here */
> +
> +	for (uint64_t i = 0; i < xd->nallocated; ++i) {
> +		int err;
> +
> +		err = _dev_close(td, &xd->files[i]);
> +		if (err)
> +			XNVME_DEBUG("xnvme_fioe: cleanup():
> Unexpected error");
> +	}
> +	err = pthread_mutex_unlock(&g_serialize);
> +	if (err)
> +		XNVME_DEBUG("FAILED: pthread_mutex_unlock(),
> err: %d", err);
> +
> +	free(xd->iocq);
> +	free(xd->iovec);
> +	free(xd);
> +	td->io_ops_data = NULL;
> +}

If we fail to acquire the lock above we should not try to unlock it. Also, we
declare err a second time inside the for loop.

With the minor issues addressed:

Reviewed-by: Vincent Fu <vincent.fu@samsung.com>