Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

[Michael S. Tsirkin]

On Tue, Sep 03, 2019 at 01:42:02PM +0200, Miklos Szeredi wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Add a basic file system module for virtio-fs.  This does not yet contain
> shared data support between host and guest or metadata coherency speedups.
> However it is already significantly faster than virtio-9p.
> 
> Design Overview
> ===============
> 
> With the goal of designing something with better performance and local file
> system semantics, a bunch of ideas were proposed.
> 
>  - Use fuse protocol (instead of 9p) for communication between guest and
>    host.  Guest kernel will be fuse client and a fuse server will run on
>    host to serve the requests.
> 
>  - For data access inside guest, mmap portion of file in QEMU address space
>    and guest accesses this memory using dax.  That way guest page cache is
>    bypassed and there is only one copy of data (on host).  This will also
>    enable mmap(MAP_SHARED) between guests.
> 
>  - For metadata coherency, there is a shared memory region which contains
>    version number associated with metadata and any guest changing metadata
>    updates version number and other guests refresh metadata on next access.
>    This is yet to be implemented.
> 
> How virtio-fs differs from existing approaches
> ==============================================
> 
> The unique idea behind virtio-fs is to take advantage of the co-location of
> the virtual machine and hypervisor to avoid communication (vmexits).
> 
> DAX allows file contents to be accessed without communication with the
> hypervisor.  The shared memory region for metadata avoids communication in
> the common case where metadata is unchanged.
> 
> By replacing expensive communication with cheaper shared memory accesses,
> we expect to achieve better performance than approaches based on network
> file system protocols.  In addition, this also makes it easier to achieve
> local file system semantics (coherency).
> 
> These techniques are not applicable to network file system protocols since
> the communications channel is bypassed by taking advantage of shared memory
> on a local machine.  This is why we decided to build virtio-fs rather than
> focus on 9P or NFS.
> 
> Caching Modes
> =============
> 
> Like virtio-9p, different caching modes are supported which determine the
> coherency level as well.  The “cache=FOO” and “writeback” options control
> the level of coherence between the guest and host filesystems.
> 
>  - cache=none
>    metadata, data and pathname lookup are not cached in guest.  They are
>    always fetched from host and any changes are immediately pushed to host.
> 
>  - cache=always
>    metadata, data and pathname lookup are cached in guest and never expire.
> 
>  - cache=auto
>    metadata and pathname lookup cache expires after a configured amount of
>    time (default is 1 second).  Data is cached while the file is open
>    (close to open consistency).
> 
>  - writeback/no_writeback
>    These options control the writeback strategy.  If writeback is disabled,
>    then normal writes will immediately be synchronized with the host fs.
>    If writeback is enabled, then writes may be cached in the guest until
>    the file is closed or an fsync(2) performed.  This option has no effect
>    on mmap-ed writes or writes going through the DAX mechanism.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

What's with all of the TODOs? Some of these are really scary,
looks like they need to be figured out before this is merged.

Endian-ness for fuse header also looks wrong.



> ---
>  fs/fuse/Kconfig                 |   11 +
>  fs/fuse/Makefile                |    1 +
>  fs/fuse/fuse_i.h                |    5 +
>  fs/fuse/virtio_fs.c             | 1072 +++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_fs.h  |   19 +
>  include/uapi/linux/virtio_ids.h |    1 +
>  6 files changed, 1109 insertions(+)
>  create mode 100644 fs/fuse/virtio_fs.c
>  create mode 100644 include/uapi/linux/virtio_fs.h
> 
> diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
> index 24fc5a5c1b97..0635cba19971 100644
> --- a/fs/fuse/Kconfig
> +++ b/fs/fuse/Kconfig
> @@ -27,3 +27,14 @@ config CUSE
>  
>  	  If you want to develop or use a userspace character device
>  	  based on CUSE, answer Y or M.
> +
> +config VIRTIO_FS
> +	tristate "Virtio Filesystem"
> +	depends on FUSE_FS
> +	select VIRTIO
> +	help
> +	  The Virtio Filesystem allows guests to mount file systems from the
> +          host.
> +
> +	  If you want to share files between guests or with the host, answer Y
> +          or M.
> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> index 9485019c2a14..6419a2b3510d 100644
> --- a/fs/fuse/Makefile
> +++ b/fs/fuse/Makefile
> @@ -5,5 +5,6 @@
>  
>  obj-$(CONFIG_FUSE_FS) += fuse.o
>  obj-$(CONFIG_CUSE) += cuse.o
> +obj-$(CONFIG_VIRTIO_FS) += virtio_fs.o
>  
>  fuse-objs := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index dbf73e5d5b38..85e2dcad68c1 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -444,6 +444,11 @@ struct fuse_req {
>  
>  	/** Request is stolen from fuse_file->reserved_req */
>  	struct file *stolen_file;
> +
> +#if IS_ENABLED(CONFIG_VIRTIO_FS)
> +	/** virtio-fs's physically contiguous buffer for in and out args */
> +	void *argbuf;
> +#endif
>  };
>  
>  struct fuse_iqueue;
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> new file mode 100644
> index 000000000000..197e79e536f9
> --- /dev/null
> +++ b/fs/fuse/virtio_fs.c
> @@ -0,0 +1,1072 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * virtio-fs: Virtio Filesystem
> + * Copyright (C) 2018 Red Hat, Inc.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_fs.h>
> +#include <linux/delay.h>
> +#include <linux/fs_context.h>
> +#include <linux/highmem.h>
> +#include "fuse_i.h"
> +
> +/* List of virtio-fs device instances and a lock for the list */
> +static DEFINE_MUTEX(virtio_fs_mutex);
> +static LIST_HEAD(virtio_fs_instances);
> +
> +enum {
> +	VQ_HIPRIO,
> +	VQ_REQUEST
> +};
> +
> +/* Per-virtqueue state */
> +struct virtio_fs_vq {
> +	spinlock_t lock;
> +	struct virtqueue *vq;     /* protected by ->lock */
> +	struct work_struct done_work;
> +	struct list_head queued_reqs;
> +	struct delayed_work dispatch_work;
> +	struct fuse_dev *fud;
> +	bool connected;
> +	long in_flight;
> +	char name[24];

I'd keep names somewhere separate as they are not used on data path.

> +} ____cacheline_aligned_in_smp;
> +
> +/* A virtio-fs device instance */
> +struct virtio_fs {
> +	struct list_head list;    /* on virtio_fs_instances */
> +	char *tag;
> +	struct virtio_fs_vq *vqs;
> +	unsigned int nvqs;            /* number of virtqueues */
> +	unsigned int num_queues;      /* number of request queues */
> +};
> +
> +struct virtio_fs_forget {
> +	struct fuse_in_header ih;
> +	struct fuse_forget_in arg;

These structures are all native endian.

Passing them to host will make cross-endian setups painful to support,
and hardware implementations impossible.

How about converting everything to LE?


> +	/* This request can be temporarily queued on virt queue */
> +	struct list_head list;
> +};
> +
> +static inline struct virtio_fs_vq *vq_to_fsvq(struct virtqueue *vq)
> +{
> +	struct virtio_fs *fs = vq->vdev->priv;
> +
> +	return &fs->vqs[vq->index];
> +}
> +
> +static inline struct fuse_pqueue *vq_to_fpq(struct virtqueue *vq)
> +{
> +	return &vq_to_fsvq(vq)->fud->pq;
> +}
> +
> +/* Add a new instance to the list or return -EEXIST if tag name exists*/
> +static int virtio_fs_add_instance(struct virtio_fs *fs)
> +{
> +	struct virtio_fs *fs2;
> +	bool duplicate = false;
> +
> +	mutex_lock(&virtio_fs_mutex);
> +
> +	list_for_each_entry(fs2, &virtio_fs_instances, list) {
> +		if (strcmp(fs->tag, fs2->tag) == 0)
> +			duplicate = true;
> +	}
> +
> +	if (!duplicate)
> +		list_add_tail(&fs->list, &virtio_fs_instances);


This is O(N^2) as it's presumably called for each istance.
Doesn't scale - please switch to a tree or such.

> +
> +	mutex_unlock(&virtio_fs_mutex);
> +
> +	if (duplicate)
> +		return -EEXIST;
> +	return 0;
> +}
> +
> +/* Return the virtio_fs with a given tag, or NULL */
> +static struct virtio_fs *virtio_fs_find_instance(const char *tag)
> +{
> +	struct virtio_fs *fs;
> +
> +	mutex_lock(&virtio_fs_mutex);
> +
> +	list_for_each_entry(fs, &virtio_fs_instances, list) {
> +		if (strcmp(fs->tag, tag) == 0)
> +			goto found;
> +	}
> +
> +	fs = NULL; /* not found */
> +
> +found:
> +	mutex_unlock(&virtio_fs_mutex);
> +
> +	return fs;
> +}
> +
> +static void virtio_fs_free_devs(struct virtio_fs *fs)
> +{
> +	unsigned int i;
> +
> +	/* TODO lock */

Doesn't inspire confidence, does it?

> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> +
> +		if (!fsvq->fud)
> +			continue;
> +
> +		flush_work(&fsvq->done_work);
> +		flush_delayed_work(&fsvq->dispatch_work);
> +
> +		/* TODO need to quiesce/end_requests/decrement dev_count */

Indeed. Won't this crash if we don't?

> +		fuse_dev_free(fsvq->fud);
> +		fsvq->fud = NULL;
> +	}
> +}
> +
> +/* Read filesystem name from virtio config into fs->tag (must kfree()). */
> +static int virtio_fs_read_tag(struct virtio_device *vdev, struct virtio_fs *fs)
> +{
> +	char tag_buf[sizeof_field(struct virtio_fs_config, tag)];
> +	char *end;
> +	size_t len;
> +
> +	virtio_cread_bytes(vdev, offsetof(struct virtio_fs_config, tag),
> +			   &tag_buf, sizeof(tag_buf));
> +	end = memchr(tag_buf, '\0', sizeof(tag_buf));
> +	if (end == tag_buf)
> +		return -EINVAL; /* empty tag */
> +	if (!end)
> +		end = &tag_buf[sizeof(tag_buf)];
> +
> +	len = end - tag_buf;
> +	fs->tag = devm_kmalloc(&vdev->dev, len + 1, GFP_KERNEL);
> +	if (!fs->tag)
> +		return -ENOMEM;
> +	memcpy(fs->tag, tag_buf, len);
> +	fs->tag[len] = '\0';
> +	return 0;
> +}
> +
> +/* Work function for hiprio completion */
> +static void virtio_fs_hiprio_done_work(struct work_struct *work)
> +{
> +	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> +						 done_work);
> +	struct virtqueue *vq = fsvq->vq;
> +
> +	/* Free completed FUSE_FORGET requests */
> +	spin_lock(&fsvq->lock);
> +	do {
> +		unsigned int len;
> +		void *req;
> +
> +		virtqueue_disable_cb(vq);
> +
> +		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> +			kfree(req);
> +			fsvq->in_flight--;
> +		}
> +	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
> +	spin_unlock(&fsvq->lock);
> +}
> +
> +static void virtio_fs_dummy_dispatch_work(struct work_struct *work)
> +{
> +}
> +
> +static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
> +{
> +	struct virtio_fs_forget *forget;
> +	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> +						 dispatch_work.work);
> +	struct virtqueue *vq = fsvq->vq;
> +	struct scatterlist sg;
> +	struct scatterlist *sgs[] = {&sg};
> +	bool notify;
> +	int ret;
> +
> +	pr_debug("virtio-fs: worker %s called.\n", __func__);
> +	while (1) {
> +		spin_lock(&fsvq->lock);
> +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> +					struct virtio_fs_forget, list);
> +		if (!forget) {
> +			spin_unlock(&fsvq->lock);
> +			return;
> +		}
> +
> +		list_del(&forget->list);
> +		if (!fsvq->connected) {
> +			spin_unlock(&fsvq->lock);
> +			kfree(forget);
> +			continue;
> +		}
> +
> +		sg_init_one(&sg, forget, sizeof(*forget));

This passes to host a structure including "struct list_head list";

Not a good idea.


> +
> +		/* Enqueue the request */
> +		dev_dbg(&vq->vdev->dev, "%s\n", __func__);
> +		ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);


This is easier as add_outbuf.

Also - why GFP_ATOMIC?

> +		if (ret < 0) {
> +			if (ret == -ENOMEM || ret == -ENOSPC) {
> +				pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
> +					 ret);
> +				list_add_tail(&forget->list,
> +						&fsvq->queued_reqs);
> +				schedule_delayed_work(&fsvq->dispatch_work,
> +						msecs_to_jiffies(1));

Can't we we requeue after some buffers get consumed?

> +			} else {
> +				pr_debug("virtio-fs: Could not queue FORGET: err=%d. Dropping it.\n",
> +					 ret);
> +				kfree(forget);
> +			}
> +			spin_unlock(&fsvq->lock);
> +			return;
> +		}
> +
> +		fsvq->in_flight++;
> +		notify = virtqueue_kick_prepare(vq);
> +		spin_unlock(&fsvq->lock);
> +
> +		if (notify)
> +			virtqueue_notify(vq);
> +		pr_debug("virtio-fs: worker %s dispatched one forget request.\n",
> +			 __func__);
> +	}
> +}
> +
> +/* Allocate and copy args into req->argbuf */
> +static int copy_args_to_argbuf(struct fuse_req *req)
> +{
> +	unsigned int offset = 0;
> +	unsigned int num_in;
> +	unsigned int num_out;
> +	unsigned int len;
> +	unsigned int i;
> +
> +	num_in = req->in.numargs - req->in.argpages;
> +	num_out = req->out.numargs - req->out.argpages;
> +	len = fuse_len_args(num_in, (struct fuse_arg *)req->in.args) +
> +	      fuse_len_args(num_out, req->out.args);
> +
> +	req->argbuf = kmalloc(len, GFP_ATOMIC);
> +	if (!req->argbuf)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_in; i++) {
> +		memcpy(req->argbuf + offset,
> +		       req->in.args[i].value,
> +		       req->in.args[i].size);
> +		offset += req->in.args[i].size;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Copy args out of and free req->argbuf */
> +static void copy_args_from_argbuf(struct fuse_req *req)
> +{
> +	unsigned int remaining;
> +	unsigned int offset;
> +	unsigned int num_in;
> +	unsigned int num_out;
> +	unsigned int i;
> +
> +	remaining = req->out.h.len - sizeof(req->out.h);
> +	num_in = req->in.numargs - req->in.argpages;
> +	num_out = req->out.numargs - req->out.argpages;
> +	offset = fuse_len_args(num_in, (struct fuse_arg *)req->in.args);
> +
> +	for (i = 0; i < num_out; i++) {
> +		unsigned int argsize = req->out.args[i].size;
> +
> +		if (req->out.argvar &&
> +		    i == req->out.numargs - 1 &&
> +		    argsize > remaining) {
> +			argsize = remaining;
> +		}
> +
> +		memcpy(req->out.args[i].value, req->argbuf + offset, argsize);
> +		offset += argsize;
> +
> +		if (i != req->out.numargs - 1)
> +			remaining -= argsize;
> +	}
> +
> +	/* Store the actual size of the variable-length arg */
> +	if (req->out.argvar)
> +		req->out.args[req->out.numargs - 1].size = remaining;
> +
> +	kfree(req->argbuf);
> +	req->argbuf = NULL;
> +}
> +
> +/* Work function for request completion */
> +static void virtio_fs_requests_done_work(struct work_struct *work)
> +{
> +	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> +						 done_work);
> +	struct fuse_pqueue *fpq = &fsvq->fud->pq;
> +	struct fuse_conn *fc = fsvq->fud->fc;
> +	struct virtqueue *vq = fsvq->vq;
> +	struct fuse_req *req;
> +	struct fuse_req *next;
> +	unsigned int len, i, thislen;
> +	struct page *page;
> +	LIST_HEAD(reqs);
> +
> +	/* Collect completed requests off the virtqueue */
> +	spin_lock(&fsvq->lock);
> +	do {
> +		virtqueue_disable_cb(vq);
> +
> +		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> +			spin_lock(&fpq->lock);
> +			list_move_tail(&req->list, &reqs);
> +			spin_unlock(&fpq->lock);
> +		}
> +	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
> +	spin_unlock(&fsvq->lock);
> +
> +	/* End requests */
> +	list_for_each_entry_safe(req, next, &reqs, list) {
> +		/* TODO check unique */
> +		/* TODO fuse_len_args(out) against oh.len */
> +
> +		copy_args_from_argbuf(req);
> +
> +		if (req->out.argpages && req->out.page_zeroing) {
> +			len = req->out.args[req->out.numargs - 1].size;
> +			for (i = 0; i < req->num_pages; i++) {
> +				thislen = req->page_descs[i].length;
> +				if (len < thislen) {
> +					WARN_ON(req->page_descs[i].offset);
> +					page = req->pages[i];
> +					zero_user_segment(page, len, thislen);
> +					len = 0;
> +				} else {
> +					len -= thislen;
> +				}
> +			}
> +		}
> +
> +		spin_lock(&fpq->lock);
> +		clear_bit(FR_SENT, &req->flags);
> +		list_del_init(&req->list);
> +		spin_unlock(&fpq->lock);
> +
> +		fuse_request_end(fc, req);
> +	}
> +}
> +
> +/* Virtqueue interrupt handler */
> +static void virtio_fs_vq_done(struct virtqueue *vq)
> +{
> +	struct virtio_fs_vq *fsvq = vq_to_fsvq(vq);
> +
> +	dev_dbg(&vq->vdev->dev, "%s %s\n", __func__, fsvq->name);
> +
> +	schedule_work(&fsvq->done_work);
> +}
> +
> +/* Initialize virtqueues */
> +static int virtio_fs_setup_vqs(struct virtio_device *vdev,
> +			       struct virtio_fs *fs)
> +{
> +	struct virtqueue **vqs;
> +	vq_callback_t **callbacks;
> +	const char **names;
> +	unsigned int i;
> +	int ret;
> +
> +	virtio_cread(vdev, struct virtio_fs_config, num_queues,
> +		     &fs->num_queues);
> +	if (fs->num_queues == 0)
> +		return -EINVAL;
> +
> +	fs->nvqs = 1 + fs->num_queues;
> +
> +	fs->vqs = devm_kcalloc(&vdev->dev, fs->nvqs,
> +				sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
> +	if (!fs->vqs)
> +		return -ENOMEM;
> +
> +	vqs = kmalloc_array(fs->nvqs, sizeof(vqs[VQ_HIPRIO]), GFP_KERNEL);
> +	callbacks = kmalloc_array(fs->nvqs, sizeof(callbacks[VQ_HIPRIO]),
> +					GFP_KERNEL);
> +	names = kmalloc_array(fs->nvqs, sizeof(names[VQ_HIPRIO]), GFP_KERNEL);
> +	if (!vqs || !callbacks || !names) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	callbacks[VQ_HIPRIO] = virtio_fs_vq_done;
> +	snprintf(fs->vqs[VQ_HIPRIO].name, sizeof(fs->vqs[VQ_HIPRIO].name),
> +			"hiprio");
> +	names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
> +	INIT_WORK(&fs->vqs[VQ_HIPRIO].done_work, virtio_fs_hiprio_done_work);
> +	INIT_LIST_HEAD(&fs->vqs[VQ_HIPRIO].queued_reqs);
> +	INIT_DELAYED_WORK(&fs->vqs[VQ_HIPRIO].dispatch_work,
> +			virtio_fs_hiprio_dispatch_work);
> +	spin_lock_init(&fs->vqs[VQ_HIPRIO].lock);
> +
> +	/* Initialize the requests virtqueues */
> +	for (i = VQ_REQUEST; i < fs->nvqs; i++) {
> +		spin_lock_init(&fs->vqs[i].lock);
> +		INIT_WORK(&fs->vqs[i].done_work, virtio_fs_requests_done_work);
> +		INIT_DELAYED_WORK(&fs->vqs[i].dispatch_work,
> +					virtio_fs_dummy_dispatch_work);
> +		INIT_LIST_HEAD(&fs->vqs[i].queued_reqs);
> +		snprintf(fs->vqs[i].name, sizeof(fs->vqs[i].name),
> +			 "requests.%u", i - VQ_REQUEST);
> +		callbacks[i] = virtio_fs_vq_done;
> +		names[i] = fs->vqs[i].name;
> +	}
> +
> +	ret = virtio_find_vqs(vdev, fs->nvqs, vqs, callbacks, names, NULL);
> +	if (ret < 0)
> +		goto out;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		fs->vqs[i].vq = vqs[i];
> +		fs->vqs[i].connected = true;
> +	}
> +out:
> +	kfree(names);
> +	kfree(callbacks);
> +	kfree(vqs);
> +	return ret;
> +}
> +
> +/* Free virtqueues (device must already be reset) */
> +static void virtio_fs_cleanup_vqs(struct virtio_device *vdev,
> +				  struct virtio_fs *fs)
> +{
> +	vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_fs_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_fs *fs;
> +	int ret;
> +
> +	fs = devm_kzalloc(&vdev->dev, sizeof(*fs), GFP_KERNEL);
> +	if (!fs)
> +		return -ENOMEM;
> +	vdev->priv = fs;
> +
> +	ret = virtio_fs_read_tag(vdev, fs);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = virtio_fs_setup_vqs(vdev, fs);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* TODO vq affinity */
> +	/* TODO populate notifications vq */

what's notifications vq?

> +
> +	/* Bring the device online in case the filesystem is mounted and
> +	 * requests need to be sent before we return.
> +	 */
> +	virtio_device_ready(vdev);
> +
> +	ret = virtio_fs_add_instance(fs);
> +	if (ret < 0)
> +		goto out_vqs;
> +
> +	return 0;
> +
> +out_vqs:
> +	vdev->config->reset(vdev);
> +	virtio_fs_cleanup_vqs(vdev, fs);
> +
> +out:
> +	vdev->priv = NULL;
> +	return ret;
> +}
> +
> +static void virtio_fs_remove(struct virtio_device *vdev)
> +{
> +	struct virtio_fs *fs = vdev->priv;
> +
> +	virtio_fs_free_devs(fs);
> +
> +	vdev->config->reset(vdev);
> +	virtio_fs_cleanup_vqs(vdev, fs);
> +
> +	mutex_lock(&virtio_fs_mutex);
> +	list_del(&fs->list);
> +	mutex_unlock(&virtio_fs_mutex);
> +
> +	vdev->priv = NULL;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int virtio_fs_freeze(struct virtio_device *vdev)
> +{
> +	return 0; /* TODO */
> +}
> +
> +static int virtio_fs_restore(struct virtio_device *vdev)
> +{
> +	return 0; /* TODO */
> +}

Is this really a good idea? I'd rather it was implemented,
but if not possible at all disabling PM seems better than just
keep going.

> +#endif /* CONFIG_PM_SLEEP */
> +
> +const static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_FS, VIRTIO_DEV_ANY_ID },
> +	{},
> +};
> +
> +const static unsigned int feature_table[] = {};
> +
> +static struct virtio_driver virtio_fs_driver = {
> +	.driver.name		= KBUILD_MODNAME,
> +	.driver.owner		= THIS_MODULE,
> +	.id_table		= id_table,
> +	.feature_table		= feature_table,
> +	.feature_table_size	= ARRAY_SIZE(feature_table),
> +	/* TODO validate config_get != NULL */

Why?

> +	.probe			= virtio_fs_probe,
> +	.remove			= virtio_fs_remove,
> +#ifdef CONFIG_PM_SLEEP
> +	.freeze			= virtio_fs_freeze,
> +	.restore		= virtio_fs_restore,
> +#endif
> +};
> +
> +static void virtio_fs_wake_forget_and_unlock(struct fuse_iqueue *fiq)
> +__releases(fiq->waitq.lock)
> +{
> +	struct fuse_forget_link *link;
> +	struct virtio_fs_forget *forget;
> +	struct scatterlist sg;
> +	struct scatterlist *sgs[] = {&sg};
> +	struct virtio_fs *fs;
> +	struct virtqueue *vq;
> +	struct virtio_fs_vq *fsvq;
> +	bool notify;
> +	u64 unique;
> +	int ret;
> +
> +	link = fuse_dequeue_forget(fiq, 1, NULL);
> +	unique = fuse_get_unique(fiq);
> +
> +	fs = fiq->priv;
> +	fsvq = &fs->vqs[VQ_HIPRIO];
> +	spin_unlock(&fiq->waitq.lock);
> +
> +	/* Allocate a buffer for the request */
> +	forget = kmalloc(sizeof(*forget), GFP_NOFS | __GFP_NOFAIL);
> +
> +	forget->ih = (struct fuse_in_header){
> +		.opcode = FUSE_FORGET,
> +		.nodeid = link->forget_one.nodeid,
> +		.unique = unique,
> +		.len = sizeof(*forget),
> +	};
> +	forget->arg = (struct fuse_forget_in){
> +		.nlookup = link->forget_one.nlookup,
> +	};
> +
> +	sg_init_one(&sg, forget, sizeof(*forget));
> +
> +	/* Enqueue the request */
> +	vq = fsvq->vq;
> +	dev_dbg(&vq->vdev->dev, "%s\n", __func__);
> +	spin_lock(&fsvq->lock);
> +
> +	ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
> +	if (ret < 0) {
> +		if (ret == -ENOMEM || ret == -ENOSPC) {
> +			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later.\n",
> +				 ret);
> +			list_add_tail(&forget->list, &fsvq->queued_reqs);
> +			schedule_delayed_work(&fsvq->dispatch_work,
> +					msecs_to_jiffies(1));
> +		} else {
> +			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Dropping it.\n",
> +				 ret);
> +			kfree(forget);
> +		}
> +		spin_unlock(&fsvq->lock);
> +		goto out;
> +	}


code duplicated from virtio_fs_hiprio_dispatch_work

> +
> +	fsvq->in_flight++;
> +	notify = virtqueue_kick_prepare(vq);
> +
> +	spin_unlock(&fsvq->lock);
> +
> +	if (notify)
> +		virtqueue_notify(vq);
> +out:
> +	kfree(link);
> +}
> +
> +static void virtio_fs_wake_interrupt_and_unlock(struct fuse_iqueue *fiq)
> +__releases(fiq->waitq.lock)
> +{
> +	/* TODO */

what's needed?

> +	spin_unlock(&fiq->waitq.lock);
> +}
> +
> +/* Return the number of scatter-gather list elements required */
> +static unsigned int sg_count_fuse_req(struct fuse_req *req)
> +{
> +	unsigned int total_sgs = 1 /* fuse_in_header */;
> +
> +	if (req->in.numargs - req->in.argpages)
> +		total_sgs += 1;
> +
> +	if (req->in.argpages)
> +		total_sgs += req->num_pages;
> +
> +	if (!test_bit(FR_ISREPLY, &req->flags))
> +		return total_sgs;
> +
> +	total_sgs += 1 /* fuse_out_header */;
> +
> +	if (req->out.numargs - req->out.argpages)
> +		total_sgs += 1;
> +
> +	if (req->out.argpages)
> +		total_sgs += req->num_pages;
> +
> +	return total_sgs;
> +}
> +
> +/* Add pages to scatter-gather list and return number of elements used */
> +static unsigned int sg_init_fuse_pages(struct scatterlist *sg,
> +				       struct page **pages,
> +				       struct fuse_page_desc *page_descs,
> +				       unsigned int num_pages,
> +				       unsigned int total_len)
> +{
> +	unsigned int i;
> +	unsigned int this_len;
> +
> +	for (i = 0; i < num_pages && total_len; i++) {
> +		sg_init_table(&sg[i], 1);
> +		this_len =  min(page_descs[i].length, total_len);
> +		sg_set_page(&sg[i], pages[i], this_len, page_descs[i].offset);
> +		total_len -= this_len;
> +	}
> +
> +	return i;
> +}
> +
> +/* Add args to scatter-gather list and return number of elements used */
> +static unsigned int sg_init_fuse_args(struct scatterlist *sg,
> +				      struct fuse_req *req,
> +				      struct fuse_arg *args,
> +				      unsigned int numargs,
> +				      bool argpages,
> +				      void *argbuf,
> +				      unsigned int *len_used)
> +{
> +	unsigned int total_sgs = 0;
> +	unsigned int len;
> +
> +	len = fuse_len_args(numargs - argpages, args);
> +	if (len)
> +		sg_init_one(&sg[total_sgs++], argbuf, len);
> +
> +	if (argpages)
> +		total_sgs += sg_init_fuse_pages(&sg[total_sgs],
> +						req->pages,
> +						req->page_descs,
> +						req->num_pages,
> +						args[numargs - 1].size);
> +
> +	if (len_used)
> +		*len_used = len;
> +
> +	return total_sgs;
> +}
> +
> +/* Add a request to a virtqueue and kick the device */
> +static int virtio_fs_enqueue_req(struct virtqueue *vq, struct fuse_req *req)
> +{
> +	/* requests need at least 4 elements */
> +	struct scatterlist *stack_sgs[6];
> +	struct scatterlist stack_sg[ARRAY_SIZE(stack_sgs)];
> +	struct scatterlist **sgs = stack_sgs;
> +	struct scatterlist *sg = stack_sg;
> +	struct virtio_fs_vq *fsvq;
> +	unsigned int argbuf_used = 0;
> +	unsigned int out_sgs = 0;
> +	unsigned int in_sgs = 0;
> +	unsigned int total_sgs;
> +	unsigned int i;
> +	int ret;
> +	bool notify;
> +
> +	/* Does the sglist fit on the stack? */
> +	total_sgs = sg_count_fuse_req(req);
> +	if (total_sgs > ARRAY_SIZE(stack_sgs)) {
> +		sgs = kmalloc_array(total_sgs, sizeof(sgs[0]), GFP_ATOMIC);
> +		sg = kmalloc_array(total_sgs, sizeof(sg[0]), GFP_ATOMIC);
> +		if (!sgs || !sg) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +	}
> +
> +	/* Use a bounce buffer since stack args cannot be mapped */
> +	ret = copy_args_to_argbuf(req);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Request elements */
> +	sg_init_one(&sg[out_sgs++], &req->in.h, sizeof(req->in.h));
> +	out_sgs += sg_init_fuse_args(&sg[out_sgs], req,
> +				     (struct fuse_arg *)req->in.args,
> +				     req->in.numargs, req->in.argpages,
> +				     req->argbuf, &argbuf_used);
> +
> +	/* Reply elements */
> +	if (test_bit(FR_ISREPLY, &req->flags)) {
> +		sg_init_one(&sg[out_sgs + in_sgs++],
> +			    &req->out.h, sizeof(req->out.h));
> +		in_sgs += sg_init_fuse_args(&sg[out_sgs + in_sgs], req,
> +					    req->out.args, req->out.numargs,
> +					    req->out.argpages,
> +					    req->argbuf + argbuf_used, NULL);
> +	}
> +
> +	WARN_ON(out_sgs + in_sgs != total_sgs);
> +
> +	for (i = 0; i < total_sgs; i++)
> +		sgs[i] = &sg[i];
> +
> +	fsvq = vq_to_fsvq(vq);
> +	spin_lock(&fsvq->lock);
> +
> +	ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
> +	if (ret < 0) {
> +		/* TODO handle full virtqueue */

Indeed. What prevents this?


> +		spin_unlock(&fsvq->lock);
> +		goto out;
> +	}
> +
> +	notify = virtqueue_kick_prepare(vq);
> +
> +	spin_unlock(&fsvq->lock);
> +
> +	if (notify)
> +		virtqueue_notify(vq);
> +
> +out:
> +	if (ret < 0 && req->argbuf) {
> +		kfree(req->argbuf);
> +		req->argbuf = NULL;
> +	}
> +	if (sgs != stack_sgs) {
> +		kfree(sgs);
> +		kfree(sg);
> +	}
> +
> +	return ret;
> +}
> +
> +static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
> +__releases(fiq->waitq.lock)
> +{
> +	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> +	struct virtio_fs *fs;
> +	struct fuse_conn *fc;
> +	struct fuse_req *req;
> +	struct fuse_pqueue *fpq;
> +	int ret;
> +
> +	WARN_ON(list_empty(&fiq->pending));
> +	req = list_last_entry(&fiq->pending, struct fuse_req, list);
> +	clear_bit(FR_PENDING, &req->flags);
> +	list_del_init(&req->list);
> +	WARN_ON(!list_empty(&fiq->pending));
> +	spin_unlock(&fiq->waitq.lock);
> +
> +	fs = fiq->priv;
> +	fc = fs->vqs[queue_id].fud->fc;
> +
> +	dev_dbg(&fs->vqs[queue_id].vq->vdev->dev,
> +		"%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
> +		__func__, req->in.h.opcode, req->in.h.unique, req->in.h.nodeid,
> +		req->in.h.len, fuse_len_args(req->out.numargs, req->out.args));
> +
> +	fpq = &fs->vqs[queue_id].fud->pq;
> +	spin_lock(&fpq->lock);
> +	if (!fpq->connected) {
> +		spin_unlock(&fpq->lock);
> +		req->out.h.error = -ENODEV;
> +		pr_err("virtio-fs: %s disconnected\n", __func__);
> +		fuse_request_end(fc, req);
> +		return;
> +	}
> +	list_add_tail(&req->list, fpq->processing);
> +	spin_unlock(&fpq->lock);
> +	set_bit(FR_SENT, &req->flags);
> +	/* matches barrier in request_wait_answer() */
> +	smp_mb__after_atomic();
> +	/* TODO check for FR_INTERRUPTED? */


?

> +
> +retry:
> +	ret = virtio_fs_enqueue_req(fs->vqs[queue_id].vq, req);
> +	if (ret < 0) {
> +		if (ret == -ENOMEM || ret == -ENOSPC) {
> +			/* Virtqueue full. Retry submission */
> +			usleep_range(20, 30);

again, why not respond to completion?

> +			goto retry;
> +		}
> +		req->out.h.error = ret;
> +		pr_err("virtio-fs: virtio_fs_enqueue_req() failed %d\n", ret);
> +		fuse_request_end(fc, req);
> +		return;
> +	}
> +}
> +
> +static void virtio_fs_flush_hiprio_queue(struct virtio_fs_vq *fsvq)
> +{
> +	struct virtio_fs_forget *forget;
> +
> +	WARN_ON(fsvq->in_flight < 0);
> +
> +	/* Go through pending forget requests and free them */
> +	spin_lock(&fsvq->lock);
> +	while (1) {
> +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> +					struct virtio_fs_forget, list);
> +		if (!forget)
> +			break;
> +		list_del(&forget->list);
> +		kfree(forget);
> +	}
> +
> +	spin_unlock(&fsvq->lock);
> +
> +	/* Wait for in flight requests to finish.*/
> +	while (1) {
> +		spin_lock(&fsvq->lock);
> +		if (!fsvq->in_flight) {
> +			spin_unlock(&fsvq->lock);
> +			break;
> +		}
> +		spin_unlock(&fsvq->lock);
> +		usleep_range(1000, 2000);

Same thing here. Can we use e.g. a completion and not usleep?

> +	}
> +}
> +
> +const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
> +	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
> +	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
> +	.wake_pending_and_unlock	= virtio_fs_wake_pending_and_unlock,
> +};
> +
> +static int virtio_fs_fill_super(struct super_block *sb)
> +{
> +	struct fuse_conn *fc = get_fuse_conn_super(sb);
> +	struct virtio_fs *fs = fc->iq.priv;
> +	unsigned int i;
> +	int err;
> +	struct fuse_req *init_req;
> +	struct fuse_fs_context ctx = {
> +		.rootmode = S_IFDIR,
> +		.default_permissions = 1,
> +		.allow_other = 1,
> +		.max_read = UINT_MAX,
> +		.blksize = 512,
> +		.destroy = true,
> +		.no_control = true,
> +		.no_force_umount = true,
> +	};
> +
> +	/* TODO lock */

what does this refer to?

> +	if (fs->vqs[VQ_REQUEST].fud) {
> +		pr_err("virtio-fs: device already in use\n");
> +		err = -EBUSY;
> +		goto err;
> +	}
> +
> +	err = -ENOMEM;
> +	/* Allocate fuse_dev for hiprio and notification queues */
> +	for (i = 0; i < VQ_REQUEST; i++) {
> +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> +
> +		fsvq->fud = fuse_dev_alloc();
> +		if (!fsvq->fud)
> +			goto err_free_fuse_devs;
> +	}
> +
> +	init_req = fuse_request_alloc(0);
> +	if (!init_req)
> +		goto err_free_fuse_devs;
> +	__set_bit(FR_BACKGROUND, &init_req->flags);
> +
> +	ctx.fudptr = (void **)&fs->vqs[VQ_REQUEST].fud;
> +	err = fuse_fill_super_common(sb, &ctx);
> +	if (err < 0)
> +		goto err_free_init_req;
> +
> +	fc = fs->vqs[VQ_REQUEST].fud->fc;
> +
> +	/* TODO take fuse_mutex around this loop? */

Don't we need to figure this kind of thing out before this
code lands upstream?

> +	for (i = 0; i < fs->nvqs; i++) {
> +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> +
> +		if (i == VQ_REQUEST)
> +			continue; /* already initialized */
> +		fuse_dev_install(fsvq->fud, fc);
> +		atomic_inc(&fc->dev_count);
> +	}
> +
> +	fuse_send_init(fc, init_req);
> +	return 0;
> +
> +err_free_init_req:
> +	fuse_request_free(init_req);
> +err_free_fuse_devs:
> +	for (i = 0; i < fs->nvqs; i++)
> +		fuse_dev_free(fs->vqs[i].fud);
> +err:
> +	return err;
> +}
> +
> +static void virtio_kill_sb(struct super_block *sb)
> +{
> +	struct fuse_conn *fc = get_fuse_conn_super(sb);
> +	struct virtio_fs *vfs;
> +	struct virtio_fs_vq *fsvq;
> +
> +	/* If mount failed, we can still be called without any fc */
> +	if (!fc)
> +		return fuse_kill_sb_anon(sb);
> +
> +	vfs = fc->iq.priv;
> +	fsvq = &vfs->vqs[VQ_HIPRIO];
> +
> +	/* Stop forget queue. Soon destroy will be sent */
> +	spin_lock(&fsvq->lock);
> +	fsvq->connected = false;
> +	spin_unlock(&fsvq->lock);
> +	virtio_fs_flush_hiprio_queue(fsvq);
> +
> +	fuse_kill_sb_anon(sb);
> +	virtio_fs_free_devs(vfs);
> +}
> +
> +static int virtio_fs_test_super(struct super_block *sb,
> +				struct fs_context *fsc)
> +{
> +	struct fuse_conn *fc = fsc->s_fs_info;
> +
> +	return fc->iq.priv == get_fuse_conn_super(sb)->iq.priv;
> +}
> +
> +static int virtio_fs_set_super(struct super_block *sb,
> +			       struct fs_context *fsc)
> +{
> +	int err;
> +
> +	err = get_anon_bdev(&sb->s_dev);
> +	if (!err)
> +		fuse_conn_get(fsc->s_fs_info);
> +
> +	return err;
> +}
> +
> +static int virtio_fs_get_tree(struct fs_context *fsc)
> +{
> +	struct virtio_fs *fs;
> +	struct super_block *sb;
> +	struct fuse_conn *fc;
> +	int err;
> +
> +	fs = virtio_fs_find_instance(fsc->source);
> +	if (!fs) {
> +		pr_info("virtio-fs: tag <%s> not found\n", fsc->source);
> +		return -EINVAL;
> +	}
> +
> +	fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
> +	if (!fc)
> +		return -ENOMEM;
> +
> +	fuse_conn_init(fc, get_user_ns(current_user_ns()), &virtio_fs_fiq_ops,
> +		       fs);
> +	fc->release = fuse_free_conn;
> +	fc->delete_stale = true;
> +
> +	fsc->s_fs_info = fc;
> +	sb = sget_fc(fsc, virtio_fs_test_super, virtio_fs_set_super);
> +	fuse_conn_put(fc);
> +	if (IS_ERR(sb))
> +		return PTR_ERR(sb);
> +
> +	if (!sb->s_root) {
> +		err = virtio_fs_fill_super(sb);
> +		if (err) {
> +			deactivate_locked_super(sb);
> +			return err;
> +		}
> +
> +		sb->s_flags |= SB_ACTIVE;
> +	}
> +
> +	WARN_ON(fsc->root);
> +	fsc->root = dget(sb->s_root);
> +	return 0;
> +}
> +
> +static const struct fs_context_operations virtio_fs_context_ops = {
> +	.get_tree	= virtio_fs_get_tree,
> +};
> +
> +static int virtio_fs_init_fs_context(struct fs_context *fsc)
> +{
> +	fsc->ops = &virtio_fs_context_ops;
> +	return 0;
> +}
> +
> +static struct file_system_type virtio_fs_type = {
> +	.owner		= THIS_MODULE,
> +	.name		= "virtiofs",
> +	.init_fs_context = virtio_fs_init_fs_context,
> +	.kill_sb	= virtio_kill_sb,
> +};
> +
> +static int __init virtio_fs_init(void)
> +{
> +	int ret;
> +
> +	ret = register_virtio_driver(&virtio_fs_driver);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = register_filesystem(&virtio_fs_type);
> +	if (ret < 0) {
> +		unregister_virtio_driver(&virtio_fs_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +module_init(virtio_fs_init);
> +
> +static void __exit virtio_fs_exit(void)
> +{
> +	unregister_filesystem(&virtio_fs_type);
> +	unregister_virtio_driver(&virtio_fs_driver);
> +}
> +module_exit(virtio_fs_exit);
> +
> +MODULE_AUTHOR("Stefan Hajnoczi <stefanha@redhat.com>");
> +MODULE_DESCRIPTION("Virtio Filesystem");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_FS(KBUILD_MODNAME);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> diff --git a/include/uapi/linux/virtio_fs.h b/include/uapi/linux/virtio_fs.h
> new file mode 100644
> index 000000000000..b5e99c217c86
> --- /dev/null
> +++ b/include/uapi/linux/virtio_fs.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_FS_H
> +#define _UAPI_LINUX_VIRTIO_FS_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_types.h>
> +
> +struct virtio_fs_config {
> +	/* Filesystem name (UTF-8, not NUL-terminated, padded with NULs) */
> +	__u8 tag[36];
> +
> +	/* Number of request queues */
> +	__u32 num_queues;
> +} __attribute__((packed));
> +
> +#endif /* _UAPI_LINUX_VIRTIO_FS_H */
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 348fd0176f75..585e07b27333 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -44,6 +44,7 @@
>  #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
>  #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
>  #define VIRTIO_ID_IOMMU        23 /* virtio IOMMU */
> +#define VIRTIO_ID_FS           26 /* virtio filesystem */
>  #define VIRTIO_ID_PMEM         27 /* virtio pmem */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> -- 
> 2.21.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 913 bytes --]

On Tue, Sep 03, 2019 at 09:55:49AM -0400, Michael S. Tsirkin wrote:
> On Tue, Sep 03, 2019 at 01:42:02PM +0200, Miklos Szeredi wrote:
> Endian-ness for fuse header also looks wrong.
[...]
> > +struct virtio_fs_forget {
> > +	struct fuse_in_header ih;
> > +	struct fuse_forget_in arg;
> 
> These structures are all native endian.
> 
> Passing them to host will make cross-endian setups painful to support,
> and hardware implementations impossible.
> 
> How about converting everything to LE?

The driver dictates the endianness of the FUSE protocol session.  The
virtio-fs device specification states that the device looks at the first
request's fuse_in_header::opcode field to detect the guest endianness.

If it sees FUSE_INIT in its native endianness then no byte-swapping is
necessary.  If it sees FUSE_INIT in the opposite endianness then
byte-swapping is necessary on the device side.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

[Michael S. Tsirkin]

On Wed, Sep 04, 2019 at 07:16:30PM +0100, Stefan Hajnoczi wrote:
> On Tue, Sep 03, 2019 at 09:55:49AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Sep 03, 2019 at 01:42:02PM +0200, Miklos Szeredi wrote:
> > Endian-ness for fuse header also looks wrong.
> [...]
> > > +struct virtio_fs_forget {
> > > +	struct fuse_in_header ih;
> > > +	struct fuse_forget_in arg;
> > 
> > These structures are all native endian.
> > 
> > Passing them to host will make cross-endian setups painful to support,
> > and hardware implementations impossible.
> > 
> > How about converting everything to LE?
> 
> The driver dictates the endianness of the FUSE protocol session.  The
> virtio-fs device specification states that the device looks at the first
> request's fuse_in_header::opcode field to detect the guest endianness.
> 
> If it sees FUSE_INIT in its native endianness then no byte-swapping is
> necessary.  If it sees FUSE_INIT in the opposite endianness then
> byte-swapping is necessary on the device side.


You are right.  Pls ignore the comment.  We need to reserve the
byte-swapped FUSE_INIT to make sure future versions of fuse don't try to
send that though.  I sent a patch to that effect, let's see whether it
gets accepted.


-- 
MST

Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

[Vivek Goyal]

On Tue, Sep 03, 2019 at 09:55:49AM -0400, Michael S. Tsirkin wrote:
[..]
> What's with all of the TODOs? Some of these are really scary,
> looks like they need to be figured out before this is merged.

Hi Michael,

One of the issue I noticed is races w.r.t device removal and super
block initialization. I am about to post a set of patches which
take care of these races and also get rid of some of the scary
TODOs. Other TODOs like suspend/restore, multiqueue support etc
are improvements which we can do over a period of time.

[..]
> > +/* Per-virtqueue state */
> > +struct virtio_fs_vq {
> > +	spinlock_t lock;
> > +	struct virtqueue *vq;     /* protected by ->lock */
> > +	struct work_struct done_work;
> > +	struct list_head queued_reqs;
> > +	struct delayed_work dispatch_work;
> > +	struct fuse_dev *fud;
> > +	bool connected;
> > +	long in_flight;
> > +	char name[24];
> 
> I'd keep names somewhere separate as they are not used on data path.

Ok, this sounds like a nice to have. Will take care of this once base
patch gets merged.

[..]
> > +struct virtio_fs_forget {
> > +	struct fuse_in_header ih;
> > +	struct fuse_forget_in arg;
> 
> These structures are all native endian.
> 
> Passing them to host will make cross-endian setups painful to support,
> and hardware implementations impossible.
> 
> How about converting everything to LE?

So looks like endianness issue is now resolved (going by the other
emails). So I will not worry about it.

[..]
> > +/* Add a new instance to the list or return -EEXIST if tag name exists*/
> > +static int virtio_fs_add_instance(struct virtio_fs *fs)
> > +{
> > +	struct virtio_fs *fs2;
> > +	bool duplicate = false;
> > +
> > +	mutex_lock(&virtio_fs_mutex);
> > +
> > +	list_for_each_entry(fs2, &virtio_fs_instances, list) {
> > +		if (strcmp(fs->tag, fs2->tag) == 0)
> > +			duplicate = true;
> > +	}
> > +
> > +	if (!duplicate)
> > +		list_add_tail(&fs->list, &virtio_fs_instances);
> 
> 
> This is O(N^2) as it's presumably called for each istance.
> Doesn't scale - please switch to a tree or such.

This is O(N) and not O(N^2) right? Addition of device is O(N), search
during mount is O(N).

This is not a frequent event at all and number of virtiofs instances
per guest are expected to be fairly small (say less than 10). So I 
really don't think there is any value in converting this into a tree
(instead of a list).

[..]
> > +static void virtio_fs_free_devs(struct virtio_fs *fs)
> > +{
> > +	unsigned int i;
> > +
> > +	/* TODO lock */
> 
> Doesn't inspire confidence, does it?

Agreed. Getting rid of this in set of fixes I am about to post.

> 
> > +
> > +	for (i = 0; i < fs->nvqs; i++) {
> > +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> > +
> > +		if (!fsvq->fud)
> > +			continue;
> > +
> > +		flush_work(&fsvq->done_work);
> > +		flush_delayed_work(&fsvq->dispatch_work);
> > +
> > +		/* TODO need to quiesce/end_requests/decrement dev_count */
> 
> Indeed. Won't this crash if we don't?

Took care of this as well.

[..]
> > +static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
> > +{
> > +	struct virtio_fs_forget *forget;
> > +	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> > +						 dispatch_work.work);
> > +	struct virtqueue *vq = fsvq->vq;
> > +	struct scatterlist sg;
> > +	struct scatterlist *sgs[] = {&sg};
> > +	bool notify;
> > +	int ret;
> > +
> > +	pr_debug("virtio-fs: worker %s called.\n", __func__);
> > +	while (1) {
> > +		spin_lock(&fsvq->lock);
> > +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> > +					struct virtio_fs_forget, list);
> > +		if (!forget) {
> > +			spin_unlock(&fsvq->lock);
> > +			return;
> > +		}
> > +
> > +		list_del(&forget->list);
> > +		if (!fsvq->connected) {
> > +			spin_unlock(&fsvq->lock);
> > +			kfree(forget);
> > +			continue;
> > +		}
> > +
> > +		sg_init_one(&sg, forget, sizeof(*forget));
> 
> This passes to host a structure including "struct list_head list";
> 
> Not a good idea.

Ok, host does not have to see "struct list_head list". Its needed for
guest. Will look into getting rid of this.

> 
> 
> > +
> > +		/* Enqueue the request */
> > +		dev_dbg(&vq->vdev->dev, "%s\n", __func__);
> > +		ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
> 
> 
> This is easier as add_outbuf.

Will look into it.

> 
> Also - why GFP_ATOMIC?

Hmm..., may be it can be GFP_KERNEL. I don't see atomic context here. Will
look into it.

> 
> > +		if (ret < 0) {
> > +			if (ret == -ENOMEM || ret == -ENOSPC) {
> > +				pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
> > +					 ret);
> > +				list_add_tail(&forget->list,
> > +						&fsvq->queued_reqs);
> > +				schedule_delayed_work(&fsvq->dispatch_work,
> > +						msecs_to_jiffies(1));
> 
> Can't we we requeue after some buffers get consumed?

That's what dispatch work is doing. It tries to requeue the request after
a while.

[..]
> > +static int virtio_fs_probe(struct virtio_device *vdev)
> > +{
> > +	struct virtio_fs *fs;
> > +	int ret;
> > +
> > +	fs = devm_kzalloc(&vdev->dev, sizeof(*fs), GFP_KERNEL);
> > +	if (!fs)
> > +		return -ENOMEM;
> > +	vdev->priv = fs;
> > +
> > +	ret = virtio_fs_read_tag(vdev, fs);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = virtio_fs_setup_vqs(vdev, fs);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	/* TODO vq affinity */
> > +	/* TODO populate notifications vq */
> 
> what's notifications vq?

It has not been implemented yet. At some point of time we want to have
a notion of notification queue so that host can send notifications to
guest. Will get rid of this comment for now.

[..]
> > +#ifdef CONFIG_PM_SLEEP
> > +static int virtio_fs_freeze(struct virtio_device *vdev)
> > +{
> > +	return 0; /* TODO */
> > +}
> > +
> > +static int virtio_fs_restore(struct virtio_device *vdev)
> > +{
> > +	return 0; /* TODO */
> > +}
> 
> Is this really a good idea? I'd rather it was implemented,
> but if not possible at all disabling PM seems better than just
> keep going.

I agree. Will look into disabling it.

> 
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +const static struct virtio_device_id id_table[] = {
> > +	{ VIRTIO_ID_FS, VIRTIO_DEV_ANY_ID },
> > +	{},
> > +};
> > +
> > +const static unsigned int feature_table[] = {};
> > +
> > +static struct virtio_driver virtio_fs_driver = {
> > +	.driver.name		= KBUILD_MODNAME,
> > +	.driver.owner		= THIS_MODULE,
> > +	.id_table		= id_table,
> > +	.feature_table		= feature_table,
> > +	.feature_table_size	= ARRAY_SIZE(feature_table),
> > +	/* TODO validate config_get != NULL */
> 
> Why?

Don't know. Stefan, do you remember why did you put this comment? If not,
I will get rid of it.

> 
> > +	.probe			= virtio_fs_probe,
> > +	.remove			= virtio_fs_remove,
> > +#ifdef CONFIG_PM_SLEEP
> > +	.freeze			= virtio_fs_freeze,
> > +	.restore		= virtio_fs_restore,
> > +#endif
> > +};
> > +
> > +static void virtio_fs_wake_forget_and_unlock(struct fuse_iqueue *fiq)
> > +__releases(fiq->waitq.lock)
> > +{
> > +	struct fuse_forget_link *link;
> > +	struct virtio_fs_forget *forget;
> > +	struct scatterlist sg;
> > +	struct scatterlist *sgs[] = {&sg};
> > +	struct virtio_fs *fs;
> > +	struct virtqueue *vq;
> > +	struct virtio_fs_vq *fsvq;
> > +	bool notify;
> > +	u64 unique;
> > +	int ret;
> > +
> > +	link = fuse_dequeue_forget(fiq, 1, NULL);
> > +	unique = fuse_get_unique(fiq);
> > +
> > +	fs = fiq->priv;
> > +	fsvq = &fs->vqs[VQ_HIPRIO];
> > +	spin_unlock(&fiq->waitq.lock);
> > +
> > +	/* Allocate a buffer for the request */
> > +	forget = kmalloc(sizeof(*forget), GFP_NOFS | __GFP_NOFAIL);
> > +
> > +	forget->ih = (struct fuse_in_header){
> > +		.opcode = FUSE_FORGET,
> > +		.nodeid = link->forget_one.nodeid,
> > +		.unique = unique,
> > +		.len = sizeof(*forget),
> > +	};
> > +	forget->arg = (struct fuse_forget_in){
> > +		.nlookup = link->forget_one.nlookup,
> > +	};
> > +
> > +	sg_init_one(&sg, forget, sizeof(*forget));
> > +
> > +	/* Enqueue the request */
> > +	vq = fsvq->vq;
> > +	dev_dbg(&vq->vdev->dev, "%s\n", __func__);
> > +	spin_lock(&fsvq->lock);
> > +
> > +	ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
> > +	if (ret < 0) {
> > +		if (ret == -ENOMEM || ret == -ENOSPC) {
> > +			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later.\n",
> > +				 ret);
> > +			list_add_tail(&forget->list, &fsvq->queued_reqs);
> > +			schedule_delayed_work(&fsvq->dispatch_work,
> > +					msecs_to_jiffies(1));
> > +		} else {
> > +			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Dropping it.\n",
> > +				 ret);
> > +			kfree(forget);
> > +		}
> > +		spin_unlock(&fsvq->lock);
> > +		goto out;
> > +	}
> 
> 
> code duplicated from virtio_fs_hiprio_dispatch_work

Will look into reusing the code.

> 
> > +
> > +	fsvq->in_flight++;
> > +	notify = virtqueue_kick_prepare(vq);
> > +
> > +	spin_unlock(&fsvq->lock);
> > +
> > +	if (notify)
> > +		virtqueue_notify(vq);
> > +out:
> > +	kfree(link);
> > +}
> > +
> > +static void virtio_fs_wake_interrupt_and_unlock(struct fuse_iqueue *fiq)
> > +__releases(fiq->waitq.lock)
> > +{
> > +	/* TODO */
> 
> what's needed?

We have not implemented this interrupt thing. It interrupts the existing
pending requests. Its not a must to have.

[..]
> > +static int virtio_fs_enqueue_req(struct virtqueue *vq, struct fuse_req *req)
> > +{
> > +	/* requests need at least 4 elements */
> > +	struct scatterlist *stack_sgs[6];
> > +	struct scatterlist stack_sg[ARRAY_SIZE(stack_sgs)];
> > +	struct scatterlist **sgs = stack_sgs;
> > +	struct scatterlist *sg = stack_sg;
> > +	struct virtio_fs_vq *fsvq;
> > +	unsigned int argbuf_used = 0;
> > +	unsigned int out_sgs = 0;
> > +	unsigned int in_sgs = 0;
> > +	unsigned int total_sgs;
> > +	unsigned int i;
> > +	int ret;
> > +	bool notify;
> > +
> > +	/* Does the sglist fit on the stack? */
> > +	total_sgs = sg_count_fuse_req(req);
> > +	if (total_sgs > ARRAY_SIZE(stack_sgs)) {
> > +		sgs = kmalloc_array(total_sgs, sizeof(sgs[0]), GFP_ATOMIC);
> > +		sg = kmalloc_array(total_sgs, sizeof(sg[0]), GFP_ATOMIC);
> > +		if (!sgs || !sg) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	/* Use a bounce buffer since stack args cannot be mapped */
> > +	ret = copy_args_to_argbuf(req);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	/* Request elements */
> > +	sg_init_one(&sg[out_sgs++], &req->in.h, sizeof(req->in.h));
> > +	out_sgs += sg_init_fuse_args(&sg[out_sgs], req,
> > +				     (struct fuse_arg *)req->in.args,
> > +				     req->in.numargs, req->in.argpages,
> > +				     req->argbuf, &argbuf_used);
> > +
> > +	/* Reply elements */
> > +	if (test_bit(FR_ISREPLY, &req->flags)) {
> > +		sg_init_one(&sg[out_sgs + in_sgs++],
> > +			    &req->out.h, sizeof(req->out.h));
> > +		in_sgs += sg_init_fuse_args(&sg[out_sgs + in_sgs], req,
> > +					    req->out.args, req->out.numargs,
> > +					    req->out.argpages,
> > +					    req->argbuf + argbuf_used, NULL);
> > +	}
> > +
> > +	WARN_ON(out_sgs + in_sgs != total_sgs);
> > +
> > +	for (i = 0; i < total_sgs; i++)
> > +		sgs[i] = &sg[i];
> > +
> > +	fsvq = vq_to_fsvq(vq);
> > +	spin_lock(&fsvq->lock);
> > +
> > +	ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
> > +	if (ret < 0) {
> > +		/* TODO handle full virtqueue */
> 
> Indeed. What prevents this?

So for forget requests (VQ_HIPRIO), I already handled this by retrying 
submitting the request with the help of a worker.

For regular requests (VQ_REQUESTS), I have never run into virt queue
being full so far. That's why never worried about it.

So nothing prevents this. But we have not noticed it yet. So its a TODO
item. It will be nice to retry if virtuqueue gets full (instead of
returning error to caller).

[..]
> > +static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
> > +__releases(fiq->waitq.lock)
> > +{
> > +	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> > +	struct virtio_fs *fs;
> > +	struct fuse_conn *fc;
> > +	struct fuse_req *req;
> > +	struct fuse_pqueue *fpq;
> > +	int ret;
> > +
> > +	WARN_ON(list_empty(&fiq->pending));
> > +	req = list_last_entry(&fiq->pending, struct fuse_req, list);
> > +	clear_bit(FR_PENDING, &req->flags);
> > +	list_del_init(&req->list);
> > +	WARN_ON(!list_empty(&fiq->pending));
> > +	spin_unlock(&fiq->waitq.lock);
> > +
> > +	fs = fiq->priv;
> > +	fc = fs->vqs[queue_id].fud->fc;
> > +
> > +	dev_dbg(&fs->vqs[queue_id].vq->vdev->dev,
> > +		"%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
> > +		__func__, req->in.h.opcode, req->in.h.unique, req->in.h.nodeid,
> > +		req->in.h.len, fuse_len_args(req->out.numargs, req->out.args));
> > +
> > +	fpq = &fs->vqs[queue_id].fud->pq;
> > +	spin_lock(&fpq->lock);
> > +	if (!fpq->connected) {
> > +		spin_unlock(&fpq->lock);
> > +		req->out.h.error = -ENODEV;
> > +		pr_err("virtio-fs: %s disconnected\n", __func__);
> > +		fuse_request_end(fc, req);
> > +		return;
> > +	}
> > +	list_add_tail(&req->list, fpq->processing);
> > +	spin_unlock(&fpq->lock);
> > +	set_bit(FR_SENT, &req->flags);
> > +	/* matches barrier in request_wait_answer() */
> > +	smp_mb__after_atomic();
> > +	/* TODO check for FR_INTERRUPTED? */
> 
> 
> ?

hmm... we don't support FR_INTERRUPTED. Stefan, do you remember why
this TODO is here. If not, I will get rid of it.

> 
> > +
> > +retry:
> > +	ret = virtio_fs_enqueue_req(fs->vqs[queue_id].vq, req);
> > +	if (ret < 0) {
> > +		if (ret == -ENOMEM || ret == -ENOSPC) {
> > +			/* Virtqueue full. Retry submission */
> > +			usleep_range(20, 30);
> 
> again, why not respond to completion?

Using usleep() was a quick fix. Will look into using completion in second
round of cleanup. In first round I am taking care of more scary TODOs
which deal with races w.r.t device removal.

> 
> > +			goto retry;
> > +		}
> > +		req->out.h.error = ret;
> > +		pr_err("virtio-fs: virtio_fs_enqueue_req() failed %d\n", ret);
> > +		fuse_request_end(fc, req);
> > +		return;
> > +	}
> > +}
> > +
> > +static void virtio_fs_flush_hiprio_queue(struct virtio_fs_vq *fsvq)
> > +{
> > +	struct virtio_fs_forget *forget;
> > +
> > +	WARN_ON(fsvq->in_flight < 0);
> > +
> > +	/* Go through pending forget requests and free them */
> > +	spin_lock(&fsvq->lock);
> > +	while (1) {
> > +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> > +					struct virtio_fs_forget, list);
> > +		if (!forget)
> > +			break;
> > +		list_del(&forget->list);
> > +		kfree(forget);
> > +	}
> > +
> > +	spin_unlock(&fsvq->lock);
> > +
> > +	/* Wait for in flight requests to finish.*/
> > +	while (1) {
> > +		spin_lock(&fsvq->lock);
> > +		if (!fsvq->in_flight) {
> > +			spin_unlock(&fsvq->lock);
> > +			break;
> > +		}
> > +		spin_unlock(&fsvq->lock);
> > +		usleep_range(1000, 2000);
> 
> Same thing here. Can we use e.g. a completion and not usleep?

Second round cleanup.

> 
> > +	}
> > +}
> > +
> > +const static struct fuse_iqueue_ops virtio_fs_fiq_ops = {
> > +	.wake_forget_and_unlock		= virtio_fs_wake_forget_and_unlock,
> > +	.wake_interrupt_and_unlock	= virtio_fs_wake_interrupt_and_unlock,
> > +	.wake_pending_and_unlock	= virtio_fs_wake_pending_and_unlock,
> > +};
> > +
> > +static int virtio_fs_fill_super(struct super_block *sb)
> > +{
> > +	struct fuse_conn *fc = get_fuse_conn_super(sb);
> > +	struct virtio_fs *fs = fc->iq.priv;
> > +	unsigned int i;
> > +	int err;
> > +	struct fuse_req *init_req;
> > +	struct fuse_fs_context ctx = {
> > +		.rootmode = S_IFDIR,
> > +		.default_permissions = 1,
> > +		.allow_other = 1,
> > +		.max_read = UINT_MAX,
> > +		.blksize = 512,
> > +		.destroy = true,
> > +		.no_control = true,
> > +		.no_force_umount = true,
> > +	};
> > +
> > +	/* TODO lock */
> 
> what does this refer to?

Took care of in first round of cleanup.

> 
> > +	if (fs->vqs[VQ_REQUEST].fud) {
> > +		pr_err("virtio-fs: device already in use\n");
> > +		err = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	err = -ENOMEM;
> > +	/* Allocate fuse_dev for hiprio and notification queues */
> > +	for (i = 0; i < VQ_REQUEST; i++) {
> > +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> > +
> > +		fsvq->fud = fuse_dev_alloc();
> > +		if (!fsvq->fud)
> > +			goto err_free_fuse_devs;
> > +	}
> > +
> > +	init_req = fuse_request_alloc(0);
> > +	if (!init_req)
> > +		goto err_free_fuse_devs;
> > +	__set_bit(FR_BACKGROUND, &init_req->flags);
> > +
> > +	ctx.fudptr = (void **)&fs->vqs[VQ_REQUEST].fud;
> > +	err = fuse_fill_super_common(sb, &ctx);
> > +	if (err < 0)
> > +		goto err_free_init_req;
> > +
> > +	fc = fs->vqs[VQ_REQUEST].fud->fc;
> > +
> > +	/* TODO take fuse_mutex around this loop? */
> 
> Don't we need to figure this kind of thing out before this
> code lands upstream?

I think we don't need this TODO. fuse_connection object and associated
super block are still being formed. And my cleanup has taken care of
the additional locking.

Thanks
Vivek

Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 9386 bytes --]

On Thu, Sep 05, 2019 at 03:15:15PM -0400, Vivek Goyal wrote:
> On Tue, Sep 03, 2019 at 09:55:49AM -0400, Michael S. Tsirkin wrote:
> [..]
> > What's with all of the TODOs? Some of these are really scary,
> > looks like they need to be figured out before this is merged.
> 
> Hi Michael,
> 
> One of the issue I noticed is races w.r.t device removal and super
> block initialization. I am about to post a set of patches which
> take care of these races and also get rid of some of the scary
> TODOs. Other TODOs like suspend/restore, multiqueue support etc
> are improvements which we can do over a period of time.
> 
> [..]
> > > +/* Per-virtqueue state */
> > > +struct virtio_fs_vq {
> > > +	spinlock_t lock;
> > > +	struct virtqueue *vq;     /* protected by ->lock */
> > > +	struct work_struct done_work;
> > > +	struct list_head queued_reqs;
> > > +	struct delayed_work dispatch_work;
> > > +	struct fuse_dev *fud;
> > > +	bool connected;
> > > +	long in_flight;
> > > +	char name[24];
> > 
> > I'd keep names somewhere separate as they are not used on data path.
> 
> Ok, this sounds like a nice to have. Will take care of this once base
> patch gets merged.
> 
> [..]
> > > +struct virtio_fs_forget {
> > > +	struct fuse_in_header ih;
> > > +	struct fuse_forget_in arg;
> > 
> > These structures are all native endian.
> > 
> > Passing them to host will make cross-endian setups painful to support,
> > and hardware implementations impossible.
> > 
> > How about converting everything to LE?
> 
> So looks like endianness issue is now resolved (going by the other
> emails). So I will not worry about it.
> 
> [..]
> > > +/* Add a new instance to the list or return -EEXIST if tag name exists*/
> > > +static int virtio_fs_add_instance(struct virtio_fs *fs)
> > > +{
> > > +	struct virtio_fs *fs2;
> > > +	bool duplicate = false;
> > > +
> > > +	mutex_lock(&virtio_fs_mutex);
> > > +
> > > +	list_for_each_entry(fs2, &virtio_fs_instances, list) {
> > > +		if (strcmp(fs->tag, fs2->tag) == 0)
> > > +			duplicate = true;
> > > +	}
> > > +
> > > +	if (!duplicate)
> > > +		list_add_tail(&fs->list, &virtio_fs_instances);
> > 
> > 
> > This is O(N^2) as it's presumably called for each istance.
> > Doesn't scale - please switch to a tree or such.
> 
> This is O(N) and not O(N^2) right? Addition of device is O(N), search
> during mount is O(N).
> 
> This is not a frequent event at all and number of virtiofs instances
> per guest are expected to be fairly small (say less than 10). So I 
> really don't think there is any value in converting this into a tree
> (instead of a list).
> 
> [..]
> > > +static void virtio_fs_free_devs(struct virtio_fs *fs)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	/* TODO lock */
> > 
> > Doesn't inspire confidence, does it?
> 
> Agreed. Getting rid of this in set of fixes I am about to post.
> 
> > 
> > > +
> > > +	for (i = 0; i < fs->nvqs; i++) {
> > > +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> > > +
> > > +		if (!fsvq->fud)
> > > +			continue;
> > > +
> > > +		flush_work(&fsvq->done_work);
> > > +		flush_delayed_work(&fsvq->dispatch_work);
> > > +
> > > +		/* TODO need to quiesce/end_requests/decrement dev_count */
> > 
> > Indeed. Won't this crash if we don't?
> 
> Took care of this as well.
> 
> [..]
> > > +static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
> > > +{
> > > +	struct virtio_fs_forget *forget;
> > > +	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> > > +						 dispatch_work.work);
> > > +	struct virtqueue *vq = fsvq->vq;
> > > +	struct scatterlist sg;
> > > +	struct scatterlist *sgs[] = {&sg};
> > > +	bool notify;
> > > +	int ret;
> > > +
> > > +	pr_debug("virtio-fs: worker %s called.\n", __func__);
> > > +	while (1) {
> > > +		spin_lock(&fsvq->lock);
> > > +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> > > +					struct virtio_fs_forget, list);
> > > +		if (!forget) {
> > > +			spin_unlock(&fsvq->lock);
> > > +			return;
> > > +		}
> > > +
> > > +		list_del(&forget->list);
> > > +		if (!fsvq->connected) {
> > > +			spin_unlock(&fsvq->lock);
> > > +			kfree(forget);
> > > +			continue;
> > > +		}
> > > +
> > > +		sg_init_one(&sg, forget, sizeof(*forget));
> > 
> > This passes to host a structure including "struct list_head list";
> > 
> > Not a good idea.
> 
> Ok, host does not have to see "struct list_head list". Its needed for
> guest. Will look into getting rid of this.
> 
> > 
> > 
> > > +
> > > +		/* Enqueue the request */
> > > +		dev_dbg(&vq->vdev->dev, "%s\n", __func__);
> > > +		ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
> > 
> > 
> > This is easier as add_outbuf.
> 
> Will look into it.
> 
> > 
> > Also - why GFP_ATOMIC?
> 
> Hmm..., may be it can be GFP_KERNEL. I don't see atomic context here. Will
> look into it.
> 
> > 
> > > +		if (ret < 0) {
> > > +			if (ret == -ENOMEM || ret == -ENOSPC) {
> > > +				pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
> > > +					 ret);
> > > +				list_add_tail(&forget->list,
> > > +						&fsvq->queued_reqs);
> > > +				schedule_delayed_work(&fsvq->dispatch_work,
> > > +						msecs_to_jiffies(1));
> > 
> > Can't we we requeue after some buffers get consumed?
> 
> That's what dispatch work is doing. It tries to requeue the request after
> a while.
> 
> [..]
> > > +static int virtio_fs_probe(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_fs *fs;
> > > +	int ret;
> > > +
> > > +	fs = devm_kzalloc(&vdev->dev, sizeof(*fs), GFP_KERNEL);
> > > +	if (!fs)
> > > +		return -ENOMEM;
> > > +	vdev->priv = fs;
> > > +
> > > +	ret = virtio_fs_read_tag(vdev, fs);
> > > +	if (ret < 0)
> > > +		goto out;
> > > +
> > > +	ret = virtio_fs_setup_vqs(vdev, fs);
> > > +	if (ret < 0)
> > > +		goto out;
> > > +
> > > +	/* TODO vq affinity */
> > > +	/* TODO populate notifications vq */
> > 
> > what's notifications vq?
> 
> It has not been implemented yet. At some point of time we want to have
> a notion of notification queue so that host can send notifications to
> guest. Will get rid of this comment for now.
> 
> [..]
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int virtio_fs_freeze(struct virtio_device *vdev)
> > > +{
> > > +	return 0; /* TODO */
> > > +}
> > > +
> > > +static int virtio_fs_restore(struct virtio_device *vdev)
> > > +{
> > > +	return 0; /* TODO */
> > > +}
> > 
> > Is this really a good idea? I'd rather it was implemented,
> > but if not possible at all disabling PM seems better than just
> > keep going.
> 
> I agree. Will look into disabling it.
> 
> > 
> > > +#endif /* CONFIG_PM_SLEEP */
> > > +
> > > +const static struct virtio_device_id id_table[] = {
> > > +	{ VIRTIO_ID_FS, VIRTIO_DEV_ANY_ID },
> > > +	{},
> > > +};
> > > +
> > > +const static unsigned int feature_table[] = {};
> > > +
> > > +static struct virtio_driver virtio_fs_driver = {
> > > +	.driver.name		= KBUILD_MODNAME,
> > > +	.driver.owner		= THIS_MODULE,
> > > +	.id_table		= id_table,
> > > +	.feature_table		= feature_table,
> > > +	.feature_table_size	= ARRAY_SIZE(feature_table),
> > > +	/* TODO validate config_get != NULL */
> > 
> > Why?
> 
> Don't know. Stefan, do you remember why did you put this comment? If not,
> I will get rid of it.

This comment can be removed.

> > > +static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
> > > +__releases(fiq->waitq.lock)
> > > +{
> > > +	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> > > +	struct virtio_fs *fs;
> > > +	struct fuse_conn *fc;
> > > +	struct fuse_req *req;
> > > +	struct fuse_pqueue *fpq;
> > > +	int ret;
> > > +
> > > +	WARN_ON(list_empty(&fiq->pending));
> > > +	req = list_last_entry(&fiq->pending, struct fuse_req, list);
> > > +	clear_bit(FR_PENDING, &req->flags);
> > > +	list_del_init(&req->list);
> > > +	WARN_ON(!list_empty(&fiq->pending));
> > > +	spin_unlock(&fiq->waitq.lock);
> > > +
> > > +	fs = fiq->priv;
> > > +	fc = fs->vqs[queue_id].fud->fc;
> > > +
> > > +	dev_dbg(&fs->vqs[queue_id].vq->vdev->dev,
> > > +		"%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
> > > +		__func__, req->in.h.opcode, req->in.h.unique, req->in.h.nodeid,
> > > +		req->in.h.len, fuse_len_args(req->out.numargs, req->out.args));
> > > +
> > > +	fpq = &fs->vqs[queue_id].fud->pq;
> > > +	spin_lock(&fpq->lock);
> > > +	if (!fpq->connected) {
> > > +		spin_unlock(&fpq->lock);
> > > +		req->out.h.error = -ENODEV;
> > > +		pr_err("virtio-fs: %s disconnected\n", __func__);
> > > +		fuse_request_end(fc, req);
> > > +		return;
> > > +	}
> > > +	list_add_tail(&req->list, fpq->processing);
> > > +	spin_unlock(&fpq->lock);
> > > +	set_bit(FR_SENT, &req->flags);
> > > +	/* matches barrier in request_wait_answer() */
> > > +	smp_mb__after_atomic();
> > > +	/* TODO check for FR_INTERRUPTED? */
> > 
> > 
> > ?
> 
> hmm... we don't support FR_INTERRUPTED. Stefan, do you remember why
> this TODO is here. If not, I will get rid of it.

We don't support FUSE_INTERRUPT yet.  The purpose of this comment is
that when we do support FUSE_INTERRUPT we'll need to follow
fuse_dev_do_read() in queuing a FUSE_INTERRUPT here.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 15/16] virtio-fs: add virtiofs filesystem

[Michael S. Tsirkin]

On Fri, Sep 06, 2019 at 11:22:09AM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 03:15:15PM -0400, Vivek Goyal wrote:
> > On Tue, Sep 03, 2019 at 09:55:49AM -0400, Michael S. Tsirkin wrote:
> > [..]
> > > What's with all of the TODOs? Some of these are really scary,
> > > looks like they need to be figured out before this is merged.
> > 
> > Hi Michael,
> > 
> > One of the issue I noticed is races w.r.t device removal and super
> > block initialization. I am about to post a set of patches which
> > take care of these races and also get rid of some of the scary
> > TODOs. Other TODOs like suspend/restore, multiqueue support etc
> > are improvements which we can do over a period of time.
> > 
> > [..]
> > > > +/* Per-virtqueue state */
> > > > +struct virtio_fs_vq {
> > > > +	spinlock_t lock;
> > > > +	struct virtqueue *vq;     /* protected by ->lock */
> > > > +	struct work_struct done_work;
> > > > +	struct list_head queued_reqs;
> > > > +	struct delayed_work dispatch_work;
> > > > +	struct fuse_dev *fud;
> > > > +	bool connected;
> > > > +	long in_flight;
> > > > +	char name[24];
> > > 
> > > I'd keep names somewhere separate as they are not used on data path.
> > 
> > Ok, this sounds like a nice to have. Will take care of this once base
> > patch gets merged.
> > 
> > [..]
> > > > +struct virtio_fs_forget {
> > > > +	struct fuse_in_header ih;
> > > > +	struct fuse_forget_in arg;
> > > 
> > > These structures are all native endian.
> > > 
> > > Passing them to host will make cross-endian setups painful to support,
> > > and hardware implementations impossible.
> > > 
> > > How about converting everything to LE?
> > 
> > So looks like endianness issue is now resolved (going by the other
> > emails). So I will not worry about it.
> > 
> > [..]
> > > > +/* Add a new instance to the list or return -EEXIST if tag name exists*/
> > > > +static int virtio_fs_add_instance(struct virtio_fs *fs)
> > > > +{
> > > > +	struct virtio_fs *fs2;
> > > > +	bool duplicate = false;
> > > > +
> > > > +	mutex_lock(&virtio_fs_mutex);
> > > > +
> > > > +	list_for_each_entry(fs2, &virtio_fs_instances, list) {
> > > > +		if (strcmp(fs->tag, fs2->tag) == 0)
> > > > +			duplicate = true;
> > > > +	}
> > > > +
> > > > +	if (!duplicate)
> > > > +		list_add_tail(&fs->list, &virtio_fs_instances);
> > > 
> > > 
> > > This is O(N^2) as it's presumably called for each istance.
> > > Doesn't scale - please switch to a tree or such.
> > 
> > This is O(N) and not O(N^2) right? Addition of device is O(N), search
> > during mount is O(N).
> > 
> > This is not a frequent event at all and number of virtiofs instances
> > per guest are expected to be fairly small (say less than 10). So I 
> > really don't think there is any value in converting this into a tree
> > (instead of a list).
> > 
> > [..]
> > > > +static void virtio_fs_free_devs(struct virtio_fs *fs)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	/* TODO lock */
> > > 
> > > Doesn't inspire confidence, does it?
> > 
> > Agreed. Getting rid of this in set of fixes I am about to post.
> > 
> > > 
> > > > +
> > > > +	for (i = 0; i < fs->nvqs; i++) {
> > > > +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> > > > +
> > > > +		if (!fsvq->fud)
> > > > +			continue;
> > > > +
> > > > +		flush_work(&fsvq->done_work);
> > > > +		flush_delayed_work(&fsvq->dispatch_work);
> > > > +
> > > > +		/* TODO need to quiesce/end_requests/decrement dev_count */
> > > 
> > > Indeed. Won't this crash if we don't?
> > 
> > Took care of this as well.
> > 
> > [..]
> > > > +static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
> > > > +{
> > > > +	struct virtio_fs_forget *forget;
> > > > +	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> > > > +						 dispatch_work.work);
> > > > +	struct virtqueue *vq = fsvq->vq;
> > > > +	struct scatterlist sg;
> > > > +	struct scatterlist *sgs[] = {&sg};
> > > > +	bool notify;
> > > > +	int ret;
> > > > +
> > > > +	pr_debug("virtio-fs: worker %s called.\n", __func__);
> > > > +	while (1) {
> > > > +		spin_lock(&fsvq->lock);
> > > > +		forget = list_first_entry_or_null(&fsvq->queued_reqs,
> > > > +					struct virtio_fs_forget, list);
> > > > +		if (!forget) {
> > > > +			spin_unlock(&fsvq->lock);
> > > > +			return;
> > > > +		}
> > > > +
> > > > +		list_del(&forget->list);
> > > > +		if (!fsvq->connected) {
> > > > +			spin_unlock(&fsvq->lock);
> > > > +			kfree(forget);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		sg_init_one(&sg, forget, sizeof(*forget));
> > > 
> > > This passes to host a structure including "struct list_head list";
> > > 
> > > Not a good idea.
> > 
> > Ok, host does not have to see "struct list_head list". Its needed for
> > guest. Will look into getting rid of this.
> > 
> > > 
> > > 
> > > > +
> > > > +		/* Enqueue the request */
> > > > +		dev_dbg(&vq->vdev->dev, "%s\n", __func__);
> > > > +		ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
> > > 
> > > 
> > > This is easier as add_outbuf.
> > 
> > Will look into it.
> > 
> > > 
> > > Also - why GFP_ATOMIC?
> > 
> > Hmm..., may be it can be GFP_KERNEL. I don't see atomic context here. Will
> > look into it.
> > 
> > > 
> > > > +		if (ret < 0) {
> > > > +			if (ret == -ENOMEM || ret == -ENOSPC) {
> > > > +				pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
> > > > +					 ret);
> > > > +				list_add_tail(&forget->list,
> > > > +						&fsvq->queued_reqs);
> > > > +				schedule_delayed_work(&fsvq->dispatch_work,
> > > > +						msecs_to_jiffies(1));
> > > 
> > > Can't we we requeue after some buffers get consumed?
> > 
> > That's what dispatch work is doing. It tries to requeue the request after
> > a while.
> > 
> > [..]
> > > > +static int virtio_fs_probe(struct virtio_device *vdev)
> > > > +{
> > > > +	struct virtio_fs *fs;
> > > > +	int ret;
> > > > +
> > > > +	fs = devm_kzalloc(&vdev->dev, sizeof(*fs), GFP_KERNEL);
> > > > +	if (!fs)
> > > > +		return -ENOMEM;
> > > > +	vdev->priv = fs;
> > > > +
> > > > +	ret = virtio_fs_read_tag(vdev, fs);
> > > > +	if (ret < 0)
> > > > +		goto out;
> > > > +
> > > > +	ret = virtio_fs_setup_vqs(vdev, fs);
> > > > +	if (ret < 0)
> > > > +		goto out;
> > > > +
> > > > +	/* TODO vq affinity */
> > > > +	/* TODO populate notifications vq */
> > > 
> > > what's notifications vq?
> > 
> > It has not been implemented yet. At some point of time we want to have
> > a notion of notification queue so that host can send notifications to
> > guest. Will get rid of this comment for now.
> > 
> > [..]
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +static int virtio_fs_freeze(struct virtio_device *vdev)
> > > > +{
> > > > +	return 0; /* TODO */
> > > > +}
> > > > +
> > > > +static int virtio_fs_restore(struct virtio_device *vdev)
> > > > +{
> > > > +	return 0; /* TODO */
> > > > +}
> > > 
> > > Is this really a good idea? I'd rather it was implemented,
> > > but if not possible at all disabling PM seems better than just
> > > keep going.
> > 
> > I agree. Will look into disabling it.
> > 
> > > 
> > > > +#endif /* CONFIG_PM_SLEEP */
> > > > +
> > > > +const static struct virtio_device_id id_table[] = {
> > > > +	{ VIRTIO_ID_FS, VIRTIO_DEV_ANY_ID },
> > > > +	{},
> > > > +};
> > > > +
> > > > +const static unsigned int feature_table[] = {};
> > > > +
> > > > +static struct virtio_driver virtio_fs_driver = {
> > > > +	.driver.name		= KBUILD_MODNAME,
> > > > +	.driver.owner		= THIS_MODULE,
> > > > +	.id_table		= id_table,
> > > > +	.feature_table		= feature_table,
> > > > +	.feature_table_size	= ARRAY_SIZE(feature_table),
> > > > +	/* TODO validate config_get != NULL */
> > > 
> > > Why?
> > 
> > Don't know. Stefan, do you remember why did you put this comment? If not,
> > I will get rid of it.
> 
> This comment can be removed.
> 
> > > > +static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
> > > > +__releases(fiq->waitq.lock)
> > > > +{
> > > > +	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> > > > +	struct virtio_fs *fs;
> > > > +	struct fuse_conn *fc;
> > > > +	struct fuse_req *req;
> > > > +	struct fuse_pqueue *fpq;
> > > > +	int ret;
> > > > +
> > > > +	WARN_ON(list_empty(&fiq->pending));
> > > > +	req = list_last_entry(&fiq->pending, struct fuse_req, list);
> > > > +	clear_bit(FR_PENDING, &req->flags);
> > > > +	list_del_init(&req->list);
> > > > +	WARN_ON(!list_empty(&fiq->pending));
> > > > +	spin_unlock(&fiq->waitq.lock);
> > > > +
> > > > +	fs = fiq->priv;
> > > > +	fc = fs->vqs[queue_id].fud->fc;
> > > > +
> > > > +	dev_dbg(&fs->vqs[queue_id].vq->vdev->dev,
> > > > +		"%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
> > > > +		__func__, req->in.h.opcode, req->in.h.unique, req->in.h.nodeid,
> > > > +		req->in.h.len, fuse_len_args(req->out.numargs, req->out.args));
> > > > +
> > > > +	fpq = &fs->vqs[queue_id].fud->pq;
> > > > +	spin_lock(&fpq->lock);
> > > > +	if (!fpq->connected) {
> > > > +		spin_unlock(&fpq->lock);
> > > > +		req->out.h.error = -ENODEV;
> > > > +		pr_err("virtio-fs: %s disconnected\n", __func__);
> > > > +		fuse_request_end(fc, req);
> > > > +		return;
> > > > +	}
> > > > +	list_add_tail(&req->list, fpq->processing);
> > > > +	spin_unlock(&fpq->lock);
> > > > +	set_bit(FR_SENT, &req->flags);
> > > > +	/* matches barrier in request_wait_answer() */
> > > > +	smp_mb__after_atomic();
> > > > +	/* TODO check for FR_INTERRUPTED? */
> > > 
> > > 
> > > ?
> > 
> > hmm... we don't support FR_INTERRUPTED. Stefan, do you remember why
> > this TODO is here. If not, I will get rid of it.
> 
> We don't support FUSE_INTERRUPT yet.  The purpose of this comment is
> that when we do support FUSE_INTERRUPT we'll need to follow
> fuse_dev_do_read() in queuing a FUSE_INTERRUPT here.
> 
> Stefan



OK so pls write this explicitly in the comment.

-- 
MST