Re: [PATCH v2] fio: add NVMe engine

[Keith Busch <Keith.Busch@xxxxxxx>]

On 3/27/20 1:12 PM, Alexey Dobriyan wrote:
> +static inline uint32_t get_nsid(const struct fio_file *f)
> +{
> +	return (uintptr_t)f->engine_data;
> +}
> +
> +static int nvme_open_file(struct thread_data *td, struct fio_file *f)
> +{
> +	struct nvme_admin_cmd cmd;
> +	struct nvme_id_ns id;
> +	struct stat st;
> +	uint32_t nsid;
> +
> +	/* NVMe ioctls ignore open flags, require CAP_SYS_ADMIN only. */
> +	f->fd = open(f->file_name, O_RDONLY);
> +	if (f->fd < 0) {
> +		return -errno;
> +	}
> +	if (fstat(f->fd, &st) == -1) {
> +		return -errno;
> +	}
> +	if (!S_ISBLK(st.st_mode)) {
> +		log_err("%s: nvme engine requires NVMe block device\n",
> +			f->file_name);
> +		return 1;
> +	}
> +
> +	nsid = ioctl(f->fd, NVME_IOCTL_ID);
> +	if (nsid < 1) {
> +		log_err("%s: ioctl NVME_IOCTL_ID\n", f->file_name);
> +		return 1;
> +	}
> +
> +	f->engine_data = (void *)(uintptr_t)nsid;
> +


...


> +
> +	f->lba_shift = id.lbaf[id.flbas & 15].ds;
> +	return 0;
> +}

...


> --- a/file.h
> +++ b/file.h
> @@ -99,6 +99,7 @@ struct fio_file {
>  	uint64_t real_file_size;
>  	uint64_t file_offset;
>  	uint64_t io_size;
> +	unsigned int lba_shift;



We have the engine_data to stash engine specific data. We shouldn't add
the 'lba_shift' to the fio_file just for the nvme engine. Rather than
saving just the 'nsid' in 'engine_data', you should save a struct that
has everything the engine needs.


Just FYI, this is how small the nvme engine becomes with libnvme:


#include <libnvme.h>
#include "../fio.h"

static int nvme_open_file(struct thread_data *td, struct fio_file *f)
{

        nvme_ns_t n = nvme_ns_open(f->file_name);

        if (!n) {
                log_err("%s: failed to open, %s\n", f->file_name,
                        strerror(errno));
                return 1;
        }

        f->fd = nvme_ns_get_fd(n);
        f->engine_data = n;
        return 0;
}

static enum fio_q_status nvme_queue(struct thread_data *td, struct io_u
*io_u)
{
        struct fio_file *f = io_u->file;
        nvme_ns_t n = f->engine_data;

        fio_ro_check(td, io_u);

        switch (io_u->ddir) {
        case DDIR_READ:
                io_u->error = nvme_ns_read(n, io_u->xfer_buf, io_u->offset,
                                           io_u->xfer_buflen);
                break;
        case DDIR_WRITE:
                io_u->error = nvme_ns_write(n, io_u->xfer_buf, io_u->offset,
                                            io_u->xfer_buflen);
                break;
        case DDIR_SYNC:
        case DDIR_DATASYNC:
                io_u->error = nvme_ns_flush(n);
                break;
        default:
                io_u->error = EINVAL;
                break;
        }

        if (io_u->error == 0) {
                io_u->resid = 0;
                io_u->error = 0;
        } else {
                io_u_log_error(td, io_u);
                td_verror(td, io_u->error, "xfer");
        }

        return FIO_Q_COMPLETED;
}

static struct ioengine_ops ioengine = {
        .name           = "nvme",
        .version        = FIO_IOOPS_VERSION,
        .flags          = FIO_SYNCIO|FIO_RAWIO|FIO_NOEXTEND,
        .queue          = nvme_queue,
        .open_file      = nvme_open_file,
        .close_file     = generic_close_file,
        .get_file_size  = generic_get_file_size,
};

fio_init
static void register_nvme_ioengine(void)
{
        register_ioengine(&ioengine);
}

fio_exit
static void unregister_nvme_ioengine(void)
{
        unregister_ioengine(&ioengine);
}