[PATCH 0/3] Virtiofs: Support for remote blocking posix locks

[PATCH 0/3] Virtiofs: Support for remote blocking posix locks

[Ioannis Angelakopoulos]

Adding support for remote blocking locks in virtiofs. Initially linux
only supported the fcntl(SETLK) option. Now the fcntl(SETLKW) option
is also supported.

A guest issuing a fcntl(SETLKW) system call will block if another guest
has already acquired the lock. Once the lock is available then the
blocking guest will receive a notification, through the notification
queue. Then the guest will unblock and acquire the lock.

Vivek Goyal (3):
  virtiofs: Add an index to keep track of first request queue
  virtiofs: Add a virtqueue for notifications
  virtiofs: Support blocking posix locks (fcntl(F_SETLKW))

 fs/fuse/virtio_fs.c            | 290 +++++++++++++++++++++++++++++++--
 include/uapi/linux/fuse.h      |   7 +
 include/uapi/linux/virtio_fs.h |   5 +
 3 files changed, 288 insertions(+), 14 deletions(-)

-- 
2.27.0

[Virtio-fs] [PATCH 0/3] Virtiofs: Support for remote blocking posix locks

[Ioannis Angelakopoulos]

Adding support for remote blocking locks in virtiofs. Initially linux
only supported the fcntl(SETLK) option. Now the fcntl(SETLKW) option
is also supported.

A guest issuing a fcntl(SETLKW) system call will block if another guest
has already acquired the lock. Once the lock is available then the
blocking guest will receive a notification, through the notification
queue. Then the guest will unblock and acquire the lock.

Vivek Goyal (3):
  virtiofs: Add an index to keep track of first request queue
  virtiofs: Add a virtqueue for notifications
  virtiofs: Support blocking posix locks (fcntl(F_SETLKW))

 fs/fuse/virtio_fs.c            | 290 +++++++++++++++++++++++++++++++--
 include/uapi/linux/fuse.h      |   7 +
 include/uapi/linux/virtio_fs.h |   5 +
 3 files changed, 288 insertions(+), 14 deletions(-)

-- 
2.27.0

[PATCH 1/3] virtiofs: Add an index to keep track of first request queue

[Ioannis Angelakopoulos]

From: Vivek Goyal <vgoyal@redhat.com>

We have many virtqueues and first queue which carries fuse normal requests
(except forget requests) has index pointed to by enum VQ_REQUEST. This
works fine as long as number of queues are not dynamic.

I am about to introduce one more virtqueue, called notification queue,
which will be present only if device on host supports it. That means index
of request queue will change depending on if notification queue is present
or not.

So, add a variable to keep track of that index and this will help when
notification queue is added in next patch.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
---
 fs/fuse/virtio_fs.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index bcb8a02e2d8b..a545e31cf1ae 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -61,6 +61,7 @@ struct virtio_fs {
 	unsigned int nvqs;               /* number of virtqueues */
 	unsigned int num_request_queues; /* number of request queues */
 	struct dax_device *dax_dev;
+	unsigned int first_reqq_idx;     /* First request queue idx */
 
 	/* DAX memory window where file contents are mapped */
 	void *window_kaddr;
@@ -681,7 +682,9 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	if (fs->num_request_queues == 0)
 		return -EINVAL;
 
-	fs->nvqs = VQ_REQUEST + fs->num_request_queues;
+	/* One hiprio queue and rest are request queues */
+	fs->nvqs = 1 + fs->num_request_queues;
+	fs->first_reqq_idx = 1;
 	fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
 	if (!fs->vqs)
 		return -ENOMEM;
@@ -701,10 +704,11 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
 
 	/* Initialize the requests virtqueues */
-	for (i = VQ_REQUEST; i < fs->nvqs; i++) {
+	for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
 		char vq_name[VQ_NAME_LEN];
 
-		snprintf(vq_name, VQ_NAME_LEN, "requests.%u", i - VQ_REQUEST);
+		snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
+			 i - fs->first_reqq_idx);
 		virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
 		callbacks[i] = virtio_fs_vq_done;
 		names[i] = fs->vqs[i].name;
@@ -1225,7 +1229,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
 __releases(fiq->lock)
 {
-	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
+	unsigned int queue_id;
 	struct virtio_fs *fs;
 	struct fuse_req *req;
 	struct virtio_fs_vq *fsvq;
@@ -1239,6 +1243,7 @@ __releases(fiq->lock)
 	spin_unlock(&fiq->lock);
 
 	fs = fiq->priv;
+	queue_id = fs->first_reqq_idx;
 
 	pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
 		  __func__, req->in.h.opcode, req->in.h.unique,
@@ -1316,7 +1321,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 
 	err = -ENOMEM;
 	/* Allocate fuse_dev for hiprio and notification queues */
-	for (i = 0; i < fs->nvqs; i++) {
+	for (i = 0; i < fs->first_reqq_idx; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
 		fsvq->fud = fuse_dev_alloc();
@@ -1325,7 +1330,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 	}
 
 	/* virtiofs allocates and installs its own fuse devices */
-	ctx->fudptr = NULL;
+	ctx->fudptr = (void **)&fs->vqs[fs->first_reqq_idx].fud;
 	if (ctx->dax) {
 		if (!fs->dax_dev) {
 			err = -EINVAL;
@@ -1339,9 +1344,14 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 	if (err < 0)
 		goto err_free_fuse_devs;
 
+	fc = fs->vqs[fs->first_reqq_idx].fud->fc;
+
 	for (i = 0; i < fs->nvqs; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
+		if (i == fs->first_reqq_idx)
+			continue;
+
 		fuse_dev_install(fsvq->fud, fc);
 	}
 
-- 
2.27.0

[Virtio-fs] [PATCH 1/3] virtiofs: Add an index to keep track of first request queue

[Ioannis Angelakopoulos]

From: Vivek Goyal <vgoyal@redhat.com>

We have many virtqueues and first queue which carries fuse normal requests
(except forget requests) has index pointed to by enum VQ_REQUEST. This
works fine as long as number of queues are not dynamic.

I am about to introduce one more virtqueue, called notification queue,
which will be present only if device on host supports it. That means index
of request queue will change depending on if notification queue is present
or not.

So, add a variable to keep track of that index and this will help when
notification queue is added in next patch.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
---
 fs/fuse/virtio_fs.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index bcb8a02e2d8b..a545e31cf1ae 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -61,6 +61,7 @@ struct virtio_fs {
 	unsigned int nvqs;               /* number of virtqueues */
 	unsigned int num_request_queues; /* number of request queues */
 	struct dax_device *dax_dev;
+	unsigned int first_reqq_idx;     /* First request queue idx */
 
 	/* DAX memory window where file contents are mapped */
 	void *window_kaddr;
@@ -681,7 +682,9 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	if (fs->num_request_queues == 0)
 		return -EINVAL;
 
-	fs->nvqs = VQ_REQUEST + fs->num_request_queues;
+	/* One hiprio queue and rest are request queues */
+	fs->nvqs = 1 + fs->num_request_queues;
+	fs->first_reqq_idx = 1;
 	fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
 	if (!fs->vqs)
 		return -ENOMEM;
@@ -701,10 +704,11 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
 
 	/* Initialize the requests virtqueues */
-	for (i = VQ_REQUEST; i < fs->nvqs; i++) {
+	for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
 		char vq_name[VQ_NAME_LEN];
 
-		snprintf(vq_name, VQ_NAME_LEN, "requests.%u", i - VQ_REQUEST);
+		snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
+			 i - fs->first_reqq_idx);
 		virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
 		callbacks[i] = virtio_fs_vq_done;
 		names[i] = fs->vqs[i].name;
@@ -1225,7 +1229,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
 __releases(fiq->lock)
 {
-	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
+	unsigned int queue_id;
 	struct virtio_fs *fs;
 	struct fuse_req *req;
 	struct virtio_fs_vq *fsvq;
@@ -1239,6 +1243,7 @@ __releases(fiq->lock)
 	spin_unlock(&fiq->lock);
 
 	fs = fiq->priv;
+	queue_id = fs->first_reqq_idx;
 
 	pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
 		  __func__, req->in.h.opcode, req->in.h.unique,
@@ -1316,7 +1321,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 
 	err = -ENOMEM;
 	/* Allocate fuse_dev for hiprio and notification queues */
-	for (i = 0; i < fs->nvqs; i++) {
+	for (i = 0; i < fs->first_reqq_idx; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
 		fsvq->fud = fuse_dev_alloc();
@@ -1325,7 +1330,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 	}
 
 	/* virtiofs allocates and installs its own fuse devices */
-	ctx->fudptr = NULL;
+	ctx->fudptr = (void **)&fs->vqs[fs->first_reqq_idx].fud;
 	if (ctx->dax) {
 		if (!fs->dax_dev) {
 			err = -EINVAL;
@@ -1339,9 +1344,14 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 	if (err < 0)
 		goto err_free_fuse_devs;
 
+	fc = fs->vqs[fs->first_reqq_idx].fud->fc;
+
 	for (i = 0; i < fs->nvqs; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
+		if (i == fs->first_reqq_idx)
+			continue;
+
 		fuse_dev_install(fsvq->fud, fc);
 	}
 
-- 
2.27.0

[PATCH 2/3] virtiofs: Add a virtqueue for notifications

[Ioannis Angelakopoulos]

From: Vivek Goyal <vgoyal@redhat.com>

Add a new virtqueue for notifications. This will allow the device to send
notifications to guest. This queue is created only if the device supports
it. This is negotiated using feature bit VIRTIO_FS_F_NOTIFICATION.

Given the architecture of virtqueue, one needs to queue up pre-allocated
elements in the notification queue and the device can pop these elements
and fill the notification info and send it back. The size of the
notification buffer is negotiable and is specified by the device through
config space. This will allow us to add and support more notification
types without having to change the spec.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
---
 fs/fuse/virtio_fs.c            | 199 +++++++++++++++++++++++++++++++--
 include/uapi/linux/virtio_fs.h |   5 +
 2 files changed, 193 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index a545e31cf1ae..f9a6a7252218 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -32,10 +32,12 @@ static LIST_HEAD(virtio_fs_instances);
 
 enum {
 	VQ_HIPRIO,
+	VQ_NOTIFY,
 	VQ_REQUEST
 };
 
 #define VQ_NAME_LEN	24
+#define VQ_NOTIFY_ELEMS 16	/* Number of notification elements */
 
 /* Per-virtqueue state */
 struct virtio_fs_vq {
@@ -44,6 +46,8 @@ struct virtio_fs_vq {
 	struct work_struct done_work;
 	struct list_head queued_reqs;
 	struct list_head end_reqs;	/* End these requests */
+	struct virtio_fs_notify_node *notify_nodes;
+	struct list_head notify_reqs;	/* List for queuing notify requests */
 	struct delayed_work dispatch_work;
 	struct fuse_dev *fud;
 	bool connected;
@@ -62,6 +66,8 @@ struct virtio_fs {
 	unsigned int num_request_queues; /* number of request queues */
 	struct dax_device *dax_dev;
 	unsigned int first_reqq_idx;     /* First request queue idx */
+	bool notify_enabled;
+	unsigned int notify_buf_size;    /* Size of notification buffer */
 
 	/* DAX memory window where file contents are mapped */
 	void *window_kaddr;
@@ -89,6 +95,19 @@ struct virtio_fs_req_work {
 static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 				 struct fuse_req *req, bool in_flight);
 
+/* Size of virtio_fs_notify specified by fs->notify_buf_size. */
+struct virtio_fs_notify {
+	struct fuse_out_header out_hdr;
+	char outarg[];
+};
+
+struct virtio_fs_notify_node {
+	struct list_head list;
+	struct virtio_fs_notify notify;
+};
+
+static int virtio_fs_enqueue_all_notify(struct virtio_fs_vq *fsvq);
+
 enum {
 	OPT_DAX,
 };
@@ -134,6 +153,11 @@ static inline struct virtio_fs_vq *vq_to_fsvq(struct virtqueue *vq)
 	return &fs->vqs[vq->index];
 }
 
+static inline struct virtio_fs *fsvq_to_fs(struct virtio_fs_vq *fsvq)
+{
+	return (struct virtio_fs *)fsvq->vq->vdev->priv;
+}
+
 /* Should be called with fsvq->lock held. */
 static inline void inc_in_flight_req(struct virtio_fs_vq *fsvq)
 {
@@ -149,10 +173,17 @@ static inline void dec_in_flight_req(struct virtio_fs_vq *fsvq)
 		complete(&fsvq->in_flight_zero);
 }
 
+static void virtio_fs_free_notify_nodes(struct virtio_fs *fs)
+{
+	if (fs->notify_enabled && fs->vqs)
+		kfree(fs->vqs[VQ_NOTIFY].notify_nodes);
+}
+
 static void release_virtio_fs_obj(struct kref *ref)
 {
 	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
 
+	virtio_fs_free_notify_nodes(vfs);
 	kfree(vfs->vqs);
 	kfree(vfs);
 }
@@ -199,6 +230,13 @@ static void virtio_fs_drain_all_queues_locked(struct virtio_fs *fs)
 	int i;
 
 	for (i = 0; i < fs->nvqs; i++) {
+		/*
+		 * Can't wait to drain notification queue as it always
+		 * had pending requests so that server can use those
+		 * to send notifications
+		 */
+		if (fs->notify_enabled && (i == VQ_NOTIFY))
+			continue;
 		fsvq = &fs->vqs[i];
 		virtio_fs_drain_queue(fsvq);
 	}
@@ -227,6 +265,8 @@ static void virtio_fs_start_all_queues(struct virtio_fs *fs)
 		spin_lock(&fsvq->lock);
 		fsvq->connected = true;
 		spin_unlock(&fsvq->lock);
+		if (fs->notify_enabled && (i == VQ_NOTIFY))
+			virtio_fs_enqueue_all_notify(fsvq);
 	}
 }
 
@@ -475,6 +515,98 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
 	}
 }
 
+static int virtio_fs_init_notify_vq(struct virtio_fs *fs,
+				    struct virtio_fs_vq *fsvq)
+{
+	struct virtio_fs_notify_node *notify;
+	unsigned int notify_node_sz = sizeof(struct list_head) +
+				  fs->notify_buf_size;
+	int i;
+
+	fsvq->notify_nodes = kcalloc(VQ_NOTIFY_ELEMS, notify_node_sz,
+				     GFP_KERNEL);
+	if (!fsvq->notify_nodes)
+		return -ENOMEM;
+
+	for (i = 0; i < VQ_NOTIFY_ELEMS; i++) {
+		notify = (void *)fsvq->notify_nodes + (i * notify_node_sz);
+		list_add_tail(&notify->list, &fsvq->notify_reqs);
+	}
+
+	return 0;
+}
+
+static int virtio_fs_enqueue_all_notify(struct virtio_fs_vq *fsvq)
+{
+	struct scatterlist sg[1];
+	int ret;
+	bool kick;
+	struct virtio_fs *fs = fsvq_to_fs(fsvq);
+	struct virtio_fs_notify_node *notify, *next;
+	unsigned int notify_sz;
+
+	notify_sz = fs->notify_buf_size;
+	spin_lock(&fsvq->lock);
+	list_for_each_entry_safe(notify, next, &fsvq->notify_reqs, list) {
+		list_del_init(&notify->list);
+		sg_init_one(sg, &notify->notify, notify_sz);
+		ret = virtqueue_add_inbuf(fsvq->vq, sg, 1, notify, GFP_ATOMIC);
+		if (ret) {
+			list_add_tail(&notify->list, &fsvq->notify_reqs);
+			spin_unlock(&fsvq->lock);
+			return ret;
+		}
+		inc_in_flight_req(fsvq);
+	}
+
+	kick = virtqueue_kick_prepare(fsvq->vq);
+	spin_unlock(&fsvq->lock);
+	if (kick)
+		virtqueue_notify(fsvq->vq);
+	return 0;
+}
+
+static void virtio_fs_notify_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;
+	LIST_HEAD(reqs);
+	struct virtio_fs_notify_node *notify, *next;
+
+	spin_lock(&fsvq->lock);
+	do {
+		unsigned int len;
+
+		virtqueue_disable_cb(vq);
+
+		while ((notify = virtqueue_get_buf(vq, &len)) != NULL)
+			list_add_tail(&notify->list, &reqs);
+
+	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
+	spin_unlock(&fsvq->lock);
+
+	/* Process notify */
+	list_for_each_entry_safe(notify, next, &reqs, list) {
+		spin_lock(&fsvq->lock);
+		dec_in_flight_req(fsvq);
+		list_del_init(&notify->list);
+		list_add_tail(&notify->list, &fsvq->notify_reqs);
+		spin_unlock(&fsvq->lock);
+	}
+
+	/*
+	 * If queue is connected, queue notifications again. If not,
+	 * these will be queued again when virtuqueue is restarted.
+	 */
+	if (fsvq->connected)
+		virtio_fs_enqueue_all_notify(fsvq);
+}
+
+static void virtio_fs_notify_dispatch_work(struct work_struct *work)
+{
+}
+
 /* Allocate and copy args into req->argbuf */
 static int copy_args_to_argbuf(struct fuse_req *req)
 {
@@ -647,24 +779,34 @@ static void virtio_fs_vq_done(struct virtqueue *vq)
 	schedule_work(&fsvq->done_work);
 }
 
-static void virtio_fs_init_vq(struct virtio_fs_vq *fsvq, char *name,
-			      int vq_type)
+static int virtio_fs_init_vq(struct virtio_fs *fs, struct virtio_fs_vq *fsvq,
+			      char *name, int vq_type)
 {
+	int ret = 0;
+
 	strncpy(fsvq->name, name, VQ_NAME_LEN);
 	spin_lock_init(&fsvq->lock);
 	INIT_LIST_HEAD(&fsvq->queued_reqs);
 	INIT_LIST_HEAD(&fsvq->end_reqs);
+	INIT_LIST_HEAD(&fsvq->notify_reqs);
 	init_completion(&fsvq->in_flight_zero);
 
 	if (vq_type == VQ_REQUEST) {
 		INIT_WORK(&fsvq->done_work, virtio_fs_requests_done_work);
 		INIT_DELAYED_WORK(&fsvq->dispatch_work,
 				  virtio_fs_request_dispatch_work);
+	} else if (vq_type == VQ_NOTIFY) {
+		INIT_WORK(&fsvq->done_work, virtio_fs_notify_done_work);
+		INIT_DELAYED_WORK(&fsvq->dispatch_work,
+				  virtio_fs_notify_dispatch_work);
+		ret = virtio_fs_init_notify_vq(fs, fsvq);
 	} else {
 		INIT_WORK(&fsvq->done_work, virtio_fs_hiprio_done_work);
 		INIT_DELAYED_WORK(&fsvq->dispatch_work,
 				  virtio_fs_hiprio_dispatch_work);
 	}
+
+	return ret;
 }
 
 /* Initialize virtqueues */
@@ -682,9 +824,28 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	if (fs->num_request_queues == 0)
 		return -EINVAL;
 
-	/* One hiprio queue and rest are request queues */
-	fs->nvqs = 1 + fs->num_request_queues;
-	fs->first_reqq_idx = 1;
+	if (virtio_has_feature(vdev, VIRTIO_FS_F_NOTIFICATION)) {
+		fs->notify_enabled = true;
+		virtio_cread(vdev, struct virtio_fs_config, notify_buf_size,
+			     &fs->notify_buf_size);
+		if (fs->notify_buf_size <= sizeof(struct fuse_out_header)) {
+			pr_err("virtio-fs: Invalid value %d of notification"
+					" buffer size\n", fs->notify_buf_size);
+			return -EINVAL;
+		}
+		pr_info("virtio-fs: device supports notification."
+				" Notification_buf_size=%u\n", fs->notify_buf_size);
+	}
+
+	if (fs->notify_enabled) {
+		/* One additional queue for hiprio and one for notifications */
+		fs->nvqs = 2 + fs->num_request_queues;
+		fs->first_reqq_idx = 2;
+	} else {
+		fs->nvqs = 1 + fs->num_request_queues;
+		fs->first_reqq_idx = 1;
+	}
+
 	fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
 	if (!fs->vqs)
 		return -ENOMEM;
@@ -700,16 +861,31 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 
 	/* Initialize the hiprio/forget request virtqueue */
 	callbacks[VQ_HIPRIO] = virtio_fs_vq_done;
-	virtio_fs_init_vq(&fs->vqs[VQ_HIPRIO], "hiprio", VQ_HIPRIO);
+	ret = virtio_fs_init_vq(fs, &fs->vqs[VQ_HIPRIO], "hiprio", VQ_HIPRIO);
+	if (ret < 0)
+		goto out;
 	names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
 
+	/* Initialize notification queue */
+	if (fs->notify_enabled) {
+		callbacks[VQ_NOTIFY] = virtio_fs_vq_done;
+		ret = virtio_fs_init_vq(fs, &fs->vqs[VQ_NOTIFY], "notification",
+					VQ_NOTIFY);
+		if (ret < 0)
+			goto out;
+		names[VQ_NOTIFY] = fs->vqs[VQ_NOTIFY].name;
+	}
+
+
 	/* Initialize the requests virtqueues */
 	for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
 		char vq_name[VQ_NAME_LEN];
 
 		snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
 			 i - fs->first_reqq_idx);
-		virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
+		ret = virtio_fs_init_vq(fs, &fs->vqs[i], vq_name, VQ_REQUEST);
+		if (ret < 0)
+			goto out;
 		callbacks[i] = virtio_fs_vq_done;
 		names[i] = fs->vqs[i].name;
 	}
@@ -720,14 +896,14 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 
 	for (i = 0; i < fs->nvqs; i++)
 		fs->vqs[i].vq = vqs[i];
-
-	virtio_fs_start_all_queues(fs);
 out:
 	kfree(names);
 	kfree(callbacks);
 	kfree(vqs);
-	if (ret)
+	if (ret) {
+		virtio_fs_free_notify_nodes(fs);
 		kfree(fs->vqs);
+	}
 	return ret;
 }
 
@@ -891,6 +1067,7 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 	 * requests need to be sent before we return.
 	 */
 	virtio_device_ready(vdev);
+	virtio_fs_start_all_queues(fs);
 
 	ret = virtio_fs_add_instance(fs);
 	if (ret < 0)
@@ -960,7 +1137,7 @@ static const struct virtio_device_id id_table[] = {
 	{},
 };
 
-static const unsigned int feature_table[] = {};
+static const unsigned int feature_table[] = {VIRTIO_FS_F_NOTIFICATION};
 
 static struct virtio_driver virtio_fs_driver = {
 	.driver.name		= KBUILD_MODNAME,
diff --git a/include/uapi/linux/virtio_fs.h b/include/uapi/linux/virtio_fs.h
index bea38291421b..3a9bbccf4115 100644
--- a/include/uapi/linux/virtio_fs.h
+++ b/include/uapi/linux/virtio_fs.h
@@ -8,12 +8,17 @@
 #include <linux/virtio_config.h>
 #include <linux/virtio_types.h>
 
+/* Feature bits */
+#define VIRTIO_FS_F_NOTIFICATION 0	/* Notification queue supported */
+
 struct virtio_fs_config {
 	/* Filesystem name (UTF-8, not NUL-terminated, padded with NULs) */
 	__u8 tag[36];
 
 	/* Number of request queues */
 	__le32 num_request_queues;
+	/* Size of notification buffer */
+	__u32 notify_buf_size;
 } __attribute__((packed));
 
 /* For the id field in virtio_pci_shm_cap */
-- 
2.27.0

[Virtio-fs] [PATCH 2/3] virtiofs: Add a virtqueue for notifications

[Ioannis Angelakopoulos]

From: Vivek Goyal <vgoyal@redhat.com>

Add a new virtqueue for notifications. This will allow the device to send
notifications to guest. This queue is created only if the device supports
it. This is negotiated using feature bit VIRTIO_FS_F_NOTIFICATION.

Given the architecture of virtqueue, one needs to queue up pre-allocated
elements in the notification queue and the device can pop these elements
and fill the notification info and send it back. The size of the
notification buffer is negotiable and is specified by the device through
config space. This will allow us to add and support more notification
types without having to change the spec.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
---
 fs/fuse/virtio_fs.c            | 199 +++++++++++++++++++++++++++++++--
 include/uapi/linux/virtio_fs.h |   5 +
 2 files changed, 193 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index a545e31cf1ae..f9a6a7252218 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -32,10 +32,12 @@ static LIST_HEAD(virtio_fs_instances);
 
 enum {
 	VQ_HIPRIO,
+	VQ_NOTIFY,
 	VQ_REQUEST
 };
 
 #define VQ_NAME_LEN	24
+#define VQ_NOTIFY_ELEMS 16	/* Number of notification elements */
 
 /* Per-virtqueue state */
 struct virtio_fs_vq {
@@ -44,6 +46,8 @@ struct virtio_fs_vq {
 	struct work_struct done_work;
 	struct list_head queued_reqs;
 	struct list_head end_reqs;	/* End these requests */
+	struct virtio_fs_notify_node *notify_nodes;
+	struct list_head notify_reqs;	/* List for queuing notify requests */
 	struct delayed_work dispatch_work;
 	struct fuse_dev *fud;
 	bool connected;
@@ -62,6 +66,8 @@ struct virtio_fs {
 	unsigned int num_request_queues; /* number of request queues */
 	struct dax_device *dax_dev;
 	unsigned int first_reqq_idx;     /* First request queue idx */
+	bool notify_enabled;
+	unsigned int notify_buf_size;    /* Size of notification buffer */
 
 	/* DAX memory window where file contents are mapped */
 	void *window_kaddr;
@@ -89,6 +95,19 @@ struct virtio_fs_req_work {
 static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 				 struct fuse_req *req, bool in_flight);
 
+/* Size of virtio_fs_notify specified by fs->notify_buf_size. */
+struct virtio_fs_notify {
+	struct fuse_out_header out_hdr;
+	char outarg[];
+};
+
+struct virtio_fs_notify_node {
+	struct list_head list;
+	struct virtio_fs_notify notify;
+};
+
+static int virtio_fs_enqueue_all_notify(struct virtio_fs_vq *fsvq);
+
 enum {
 	OPT_DAX,
 };
@@ -134,6 +153,11 @@ static inline struct virtio_fs_vq *vq_to_fsvq(struct virtqueue *vq)
 	return &fs->vqs[vq->index];
 }
 
+static inline struct virtio_fs *fsvq_to_fs(struct virtio_fs_vq *fsvq)
+{
+	return (struct virtio_fs *)fsvq->vq->vdev->priv;
+}
+
 /* Should be called with fsvq->lock held. */
 static inline void inc_in_flight_req(struct virtio_fs_vq *fsvq)
 {
@@ -149,10 +173,17 @@ static inline void dec_in_flight_req(struct virtio_fs_vq *fsvq)
 		complete(&fsvq->in_flight_zero);
 }
 
+static void virtio_fs_free_notify_nodes(struct virtio_fs *fs)
+{
+	if (fs->notify_enabled && fs->vqs)
+		kfree(fs->vqs[VQ_NOTIFY].notify_nodes);
+}
+
 static void release_virtio_fs_obj(struct kref *ref)
 {
 	struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount);
 
+	virtio_fs_free_notify_nodes(vfs);
 	kfree(vfs->vqs);
 	kfree(vfs);
 }
@@ -199,6 +230,13 @@ static void virtio_fs_drain_all_queues_locked(struct virtio_fs *fs)
 	int i;
 
 	for (i = 0; i < fs->nvqs; i++) {
+		/*
+		 * Can't wait to drain notification queue as it always
+		 * had pending requests so that server can use those
+		 * to send notifications
+		 */
+		if (fs->notify_enabled && (i == VQ_NOTIFY))
+			continue;
 		fsvq = &fs->vqs[i];
 		virtio_fs_drain_queue(fsvq);
 	}
@@ -227,6 +265,8 @@ static void virtio_fs_start_all_queues(struct virtio_fs *fs)
 		spin_lock(&fsvq->lock);
 		fsvq->connected = true;
 		spin_unlock(&fsvq->lock);
+		if (fs->notify_enabled && (i == VQ_NOTIFY))
+			virtio_fs_enqueue_all_notify(fsvq);
 	}
 }
 
@@ -475,6 +515,98 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
 	}
 }
 
+static int virtio_fs_init_notify_vq(struct virtio_fs *fs,
+				    struct virtio_fs_vq *fsvq)
+{
+	struct virtio_fs_notify_node *notify;
+	unsigned int notify_node_sz = sizeof(struct list_head) +
+				  fs->notify_buf_size;
+	int i;
+
+	fsvq->notify_nodes = kcalloc(VQ_NOTIFY_ELEMS, notify_node_sz,
+				     GFP_KERNEL);
+	if (!fsvq->notify_nodes)
+		return -ENOMEM;
+
+	for (i = 0; i < VQ_NOTIFY_ELEMS; i++) {
+		notify = (void *)fsvq->notify_nodes + (i * notify_node_sz);
+		list_add_tail(&notify->list, &fsvq->notify_reqs);
+	}
+
+	return 0;
+}
+
+static int virtio_fs_enqueue_all_notify(struct virtio_fs_vq *fsvq)
+{
+	struct scatterlist sg[1];
+	int ret;
+	bool kick;
+	struct virtio_fs *fs = fsvq_to_fs(fsvq);
+	struct virtio_fs_notify_node *notify, *next;
+	unsigned int notify_sz;
+
+	notify_sz = fs->notify_buf_size;
+	spin_lock(&fsvq->lock);
+	list_for_each_entry_safe(notify, next, &fsvq->notify_reqs, list) {
+		list_del_init(&notify->list);
+		sg_init_one(sg, &notify->notify, notify_sz);
+		ret = virtqueue_add_inbuf(fsvq->vq, sg, 1, notify, GFP_ATOMIC);
+		if (ret) {
+			list_add_tail(&notify->list, &fsvq->notify_reqs);
+			spin_unlock(&fsvq->lock);
+			return ret;
+		}
+		inc_in_flight_req(fsvq);
+	}
+
+	kick = virtqueue_kick_prepare(fsvq->vq);
+	spin_unlock(&fsvq->lock);
+	if (kick)
+		virtqueue_notify(fsvq->vq);
+	return 0;
+}
+
+static void virtio_fs_notify_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;
+	LIST_HEAD(reqs);
+	struct virtio_fs_notify_node *notify, *next;
+
+	spin_lock(&fsvq->lock);
+	do {
+		unsigned int len;
+
+		virtqueue_disable_cb(vq);
+
+		while ((notify = virtqueue_get_buf(vq, &len)) != NULL)
+			list_add_tail(&notify->list, &reqs);
+
+	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
+	spin_unlock(&fsvq->lock);
+
+	/* Process notify */
+	list_for_each_entry_safe(notify, next, &reqs, list) {
+		spin_lock(&fsvq->lock);
+		dec_in_flight_req(fsvq);
+		list_del_init(&notify->list);
+		list_add_tail(&notify->list, &fsvq->notify_reqs);
+		spin_unlock(&fsvq->lock);
+	}
+
+	/*
+	 * If queue is connected, queue notifications again. If not,
+	 * these will be queued again when virtuqueue is restarted.
+	 */
+	if (fsvq->connected)
+		virtio_fs_enqueue_all_notify(fsvq);
+}
+
+static void virtio_fs_notify_dispatch_work(struct work_struct *work)
+{
+}
+
 /* Allocate and copy args into req->argbuf */
 static int copy_args_to_argbuf(struct fuse_req *req)
 {
@@ -647,24 +779,34 @@ static void virtio_fs_vq_done(struct virtqueue *vq)
 	schedule_work(&fsvq->done_work);
 }
 
-static void virtio_fs_init_vq(struct virtio_fs_vq *fsvq, char *name,
-			      int vq_type)
+static int virtio_fs_init_vq(struct virtio_fs *fs, struct virtio_fs_vq *fsvq,
+			      char *name, int vq_type)
 {
+	int ret = 0;
+
 	strncpy(fsvq->name, name, VQ_NAME_LEN);
 	spin_lock_init(&fsvq->lock);
 	INIT_LIST_HEAD(&fsvq->queued_reqs);
 	INIT_LIST_HEAD(&fsvq->end_reqs);
+	INIT_LIST_HEAD(&fsvq->notify_reqs);
 	init_completion(&fsvq->in_flight_zero);
 
 	if (vq_type == VQ_REQUEST) {
 		INIT_WORK(&fsvq->done_work, virtio_fs_requests_done_work);
 		INIT_DELAYED_WORK(&fsvq->dispatch_work,
 				  virtio_fs_request_dispatch_work);
+	} else if (vq_type == VQ_NOTIFY) {
+		INIT_WORK(&fsvq->done_work, virtio_fs_notify_done_work);
+		INIT_DELAYED_WORK(&fsvq->dispatch_work,
+				  virtio_fs_notify_dispatch_work);
+		ret = virtio_fs_init_notify_vq(fs, fsvq);
 	} else {
 		INIT_WORK(&fsvq->done_work, virtio_fs_hiprio_done_work);
 		INIT_DELAYED_WORK(&fsvq->dispatch_work,
 				  virtio_fs_hiprio_dispatch_work);
 	}
+
+	return ret;
 }
 
 /* Initialize virtqueues */
@@ -682,9 +824,28 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 	if (fs->num_request_queues == 0)
 		return -EINVAL;
 
-	/* One hiprio queue and rest are request queues */
-	fs->nvqs = 1 + fs->num_request_queues;
-	fs->first_reqq_idx = 1;
+	if (virtio_has_feature(vdev, VIRTIO_FS_F_NOTIFICATION)) {
+		fs->notify_enabled = true;
+		virtio_cread(vdev, struct virtio_fs_config, notify_buf_size,
+			     &fs->notify_buf_size);
+		if (fs->notify_buf_size <= sizeof(struct fuse_out_header)) {
+			pr_err("virtio-fs: Invalid value %d of notification"
+					" buffer size\n", fs->notify_buf_size);
+			return -EINVAL;
+		}
+		pr_info("virtio-fs: device supports notification."
+				" Notification_buf_size=%u\n", fs->notify_buf_size);
+	}
+
+	if (fs->notify_enabled) {
+		/* One additional queue for hiprio and one for notifications */
+		fs->nvqs = 2 + fs->num_request_queues;
+		fs->first_reqq_idx = 2;
+	} else {
+		fs->nvqs = 1 + fs->num_request_queues;
+		fs->first_reqq_idx = 1;
+	}
+
 	fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
 	if (!fs->vqs)
 		return -ENOMEM;
@@ -700,16 +861,31 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 
 	/* Initialize the hiprio/forget request virtqueue */
 	callbacks[VQ_HIPRIO] = virtio_fs_vq_done;
-	virtio_fs_init_vq(&fs->vqs[VQ_HIPRIO], "hiprio", VQ_HIPRIO);
+	ret = virtio_fs_init_vq(fs, &fs->vqs[VQ_HIPRIO], "hiprio", VQ_HIPRIO);
+	if (ret < 0)
+		goto out;
 	names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
 
+	/* Initialize notification queue */
+	if (fs->notify_enabled) {
+		callbacks[VQ_NOTIFY] = virtio_fs_vq_done;
+		ret = virtio_fs_init_vq(fs, &fs->vqs[VQ_NOTIFY], "notification",
+					VQ_NOTIFY);
+		if (ret < 0)
+			goto out;
+		names[VQ_NOTIFY] = fs->vqs[VQ_NOTIFY].name;
+	}
+
+
 	/* Initialize the requests virtqueues */
 	for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
 		char vq_name[VQ_NAME_LEN];
 
 		snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
 			 i - fs->first_reqq_idx);
-		virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
+		ret = virtio_fs_init_vq(fs, &fs->vqs[i], vq_name, VQ_REQUEST);
+		if (ret < 0)
+			goto out;
 		callbacks[i] = virtio_fs_vq_done;
 		names[i] = fs->vqs[i].name;
 	}
@@ -720,14 +896,14 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
 
 	for (i = 0; i < fs->nvqs; i++)
 		fs->vqs[i].vq = vqs[i];
-
-	virtio_fs_start_all_queues(fs);
 out:
 	kfree(names);
 	kfree(callbacks);
 	kfree(vqs);
-	if (ret)
+	if (ret) {
+		virtio_fs_free_notify_nodes(fs);
 		kfree(fs->vqs);
+	}
 	return ret;
 }
 
@@ -891,6 +1067,7 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 	 * requests need to be sent before we return.
 	 */
 	virtio_device_ready(vdev);
+	virtio_fs_start_all_queues(fs);
 
 	ret = virtio_fs_add_instance(fs);
 	if (ret < 0)
@@ -960,7 +1137,7 @@ static const struct virtio_device_id id_table[] = {
 	{},
 };
 
-static const unsigned int feature_table[] = {};
+static const unsigned int feature_table[] = {VIRTIO_FS_F_NOTIFICATION};
 
 static struct virtio_driver virtio_fs_driver = {
 	.driver.name		= KBUILD_MODNAME,
diff --git a/include/uapi/linux/virtio_fs.h b/include/uapi/linux/virtio_fs.h
index bea38291421b..3a9bbccf4115 100644
--- a/include/uapi/linux/virtio_fs.h
+++ b/include/uapi/linux/virtio_fs.h
@@ -8,12 +8,17 @@
 #include <linux/virtio_config.h>
 #include <linux/virtio_types.h>
 
+/* Feature bits */
+#define VIRTIO_FS_F_NOTIFICATION 0	/* Notification queue supported */
+
 struct virtio_fs_config {
 	/* Filesystem name (UTF-8, not NUL-terminated, padded with NULs) */
 	__u8 tag[36];
 
 	/* Number of request queues */
 	__le32 num_request_queues;
+	/* Size of notification buffer */
+	__u32 notify_buf_size;
 } __attribute__((packed));
 
 /* For the id field in virtio_pci_shm_cap */
-- 
2.27.0

[PATCH 3/3] virtiofs: Support blocking posix locks (fcntl(F_SETLKW))

[Ioannis Angelakopoulos]

From: Vivek Goyal <vgoyal@redhat.com>

As of now we don't support blocking variant of posix locks and daemon
returns -EOPNOTSUPP. Reason being that it can lead to deadlocks.
Virtqueue size is limited and it is possible we fill virtqueue with
all the requests of fcntl(F_SETLKW) and wait for reply. And later a
subsequent unlock request can't make progress because virtqueue is full.
And that means F_SETLKW can't make progress and we are deadlocked.

Use notification queue to solve this problem. After submitting lock
request device will send a reply asking requester to wait. Once lock is
available, requester will get a notification saying locking is available.
That way we don't keep the request virtueue busy while we are waiting for
lock and further unlock requests can make progress.

When we get a reply in response to lock request, we need a way to know
if we need to wait for notification or not. I have overloaded the
fuse_out_header->error field. If value is ->error is 1, that's a signal
to caller to wait for lock notification.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
---
 fs/fuse/virtio_fs.c       | 75 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/fuse.h |  7 ++++
 2 files changed, 82 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index f9a6a7252218..c85334543a29 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -45,6 +45,7 @@ struct virtio_fs_vq {
 	struct virtqueue *vq;     /* protected by ->lock */
 	struct work_struct done_work;
 	struct list_head queued_reqs;
+	struct list_head wait_reqs;     /* Requests waiting for notification  */
 	struct list_head end_reqs;	/* End these requests */
 	struct virtio_fs_notify_node *notify_nodes;
 	struct list_head notify_reqs;	/* List for queuing notify requests */
@@ -566,13 +567,74 @@ static int virtio_fs_enqueue_all_notify(struct virtio_fs_vq *fsvq)
 	return 0;
 }
 
+static int notify_complete_waiting_req(struct virtio_fs *vfs,
+				       struct fuse_notify_lock_out *out_args)
+{
+	struct virtio_fs_vq *fsvq = &vfs->vqs[VQ_REQUEST];
+	struct fuse_req *req, *next;
+	bool found = false;
+
+	/* Find waiting request with the unique number and end it */
+	spin_lock(&fsvq->lock);
+		list_for_each_entry_safe(req, next, &fsvq->wait_reqs, list) {
+			if (req->in.h.unique == out_args->unique) {
+				list_del_init(&req->list);
+				clear_bit(FR_SENT, &req->flags);
+				/* Transfer error code from notify */
+				req->out.h.error = out_args->error;
+				found = true;
+				break;
+			}
+		}
+	spin_unlock(&fsvq->lock);
+
+	/*
+	 * TODO: It is possible that some re-ordering happens in notify
+	 * comes before request is complete. Deal with it.
+	 */
+	if (found) {
+		fuse_request_end(req);
+		spin_lock(&fsvq->lock);
+		dec_in_flight_req(fsvq);
+		spin_unlock(&fsvq->lock);
+	} else
+		pr_debug("virtio-fs: Did not find waiting request"
+				" with unique=0x%llx\n", out_args->unique);
+
+	return 0;
+}
+
+static int virtio_fs_handle_notify(struct virtio_fs *vfs,
+				   struct virtio_fs_notify *notify)
+{
+	int ret = 0;
+	struct fuse_out_header *oh = &notify->out_hdr;
+	struct fuse_notify_lock_out *lo;
+
+	/*
+	 * For notifications, oh.unique is 0 and oh->error contains code
+	 * for which notification as arrived.
+	 */
+	switch (oh->error) {
+	case FUSE_NOTIFY_LOCK:
+		lo = (struct fuse_notify_lock_out *) &notify->outarg;
+		notify_complete_waiting_req(vfs, lo);
+		break;
+	default:
+		pr_err("virtio-fs: Unexpected notification %d\n", oh->error);
+	}
+	return ret;
+}
+
 static void virtio_fs_notify_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;
+	struct virtio_fs *vfs = vq->vdev->priv;
 	LIST_HEAD(reqs);
 	struct virtio_fs_notify_node *notify, *next;
+	struct fuse_out_header *oh;
 
 	spin_lock(&fsvq->lock);
 	do {
@@ -588,6 +650,10 @@ static void virtio_fs_notify_done_work(struct work_struct *work)
 
 	/* Process notify */
 	list_for_each_entry_safe(notify, next, &reqs, list) {
+		oh = &notify->notify.out_hdr;
+		WARN_ON(oh->unique);
+		/* Handle notification */
+		virtio_fs_handle_notify(vfs, &notify->notify);
 		spin_lock(&fsvq->lock);
 		dec_in_flight_req(fsvq);
 		list_del_init(&notify->list);
@@ -688,6 +754,14 @@ static void virtio_fs_request_complete(struct fuse_req *req,
 	 * TODO verify that server properly follows FUSE protocol
 	 * (oh.uniq, oh.len)
 	 */
+	if (req->out.h.error == 1) {
+		/* Wait for notification to complete request */
+		spin_lock(&fsvq->lock);
+		list_add_tail(&req->list, &fsvq->wait_reqs);
+		spin_unlock(&fsvq->lock);
+		return;
+	}
+
 	args = req->args;
 	copy_args_from_argbuf(args, req);
 
@@ -787,6 +861,7 @@ static int virtio_fs_init_vq(struct virtio_fs *fs, struct virtio_fs_vq *fsvq,
 	strncpy(fsvq->name, name, VQ_NAME_LEN);
 	spin_lock_init(&fsvq->lock);
 	INIT_LIST_HEAD(&fsvq->queued_reqs);
+	INIT_LIST_HEAD(&fsvq->wait_reqs);
 	INIT_LIST_HEAD(&fsvq->end_reqs);
 	INIT_LIST_HEAD(&fsvq->notify_reqs);
 	init_completion(&fsvq->in_flight_zero);
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 271ae90a9bb7..ae6b3fcd1fa7 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -525,6 +525,7 @@ enum fuse_notify_code {
 	FUSE_NOTIFY_STORE = 4,
 	FUSE_NOTIFY_RETRIEVE = 5,
 	FUSE_NOTIFY_DELETE = 6,
+	FUSE_NOTIFY_LOCK = 7,
 	FUSE_NOTIFY_CODE_MAX,
 };
 
@@ -916,6 +917,12 @@ struct fuse_notify_retrieve_in {
 	uint64_t	dummy4;
 };
 
+struct fuse_notify_lock_out {
+	uint64_t	unique;
+	int32_t		error;
+	int32_t		padding;
+};
+
 /* Device ioctls: */
 #define FUSE_DEV_IOC_MAGIC		229
 #define FUSE_DEV_IOC_CLONE		_IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t)
-- 
2.27.0

[Virtio-fs] [PATCH 3/3] virtiofs: Support blocking posix locks (fcntl(F_SETLKW))

[Ioannis Angelakopoulos]

From: Vivek Goyal <vgoyal@redhat.com>

As of now we don't support blocking variant of posix locks and daemon
returns -EOPNOTSUPP. Reason being that it can lead to deadlocks.
Virtqueue size is limited and it is possible we fill virtqueue with
all the requests of fcntl(F_SETLKW) and wait for reply. And later a
subsequent unlock request can't make progress because virtqueue is full.
And that means F_SETLKW can't make progress and we are deadlocked.

Use notification queue to solve this problem. After submitting lock
request device will send a reply asking requester to wait. Once lock is
available, requester will get a notification saying locking is available.
That way we don't keep the request virtueue busy while we are waiting for
lock and further unlock requests can make progress.

When we get a reply in response to lock request, we need a way to know
if we need to wait for notification or not. I have overloaded the
fuse_out_header->error field. If value is ->error is 1, that's a signal
to caller to wait for lock notification.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
---
 fs/fuse/virtio_fs.c       | 75 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/fuse.h |  7 ++++
 2 files changed, 82 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index f9a6a7252218..c85334543a29 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -45,6 +45,7 @@ struct virtio_fs_vq {
 	struct virtqueue *vq;     /* protected by ->lock */
 	struct work_struct done_work;
 	struct list_head queued_reqs;
+	struct list_head wait_reqs;     /* Requests waiting for notification  */
 	struct list_head end_reqs;	/* End these requests */
 	struct virtio_fs_notify_node *notify_nodes;
 	struct list_head notify_reqs;	/* List for queuing notify requests */
@@ -566,13 +567,74 @@ static int virtio_fs_enqueue_all_notify(struct virtio_fs_vq *fsvq)
 	return 0;
 }
 
+static int notify_complete_waiting_req(struct virtio_fs *vfs,
+				       struct fuse_notify_lock_out *out_args)
+{
+	struct virtio_fs_vq *fsvq = &vfs->vqs[VQ_REQUEST];
+	struct fuse_req *req, *next;
+	bool found = false;
+
+	/* Find waiting request with the unique number and end it */
+	spin_lock(&fsvq->lock);
+		list_for_each_entry_safe(req, next, &fsvq->wait_reqs, list) {
+			if (req->in.h.unique == out_args->unique) {
+				list_del_init(&req->list);
+				clear_bit(FR_SENT, &req->flags);
+				/* Transfer error code from notify */
+				req->out.h.error = out_args->error;
+				found = true;
+				break;
+			}
+		}
+	spin_unlock(&fsvq->lock);
+
+	/*
+	 * TODO: It is possible that some re-ordering happens in notify
+	 * comes before request is complete. Deal with it.
+	 */
+	if (found) {
+		fuse_request_end(req);
+		spin_lock(&fsvq->lock);
+		dec_in_flight_req(fsvq);
+		spin_unlock(&fsvq->lock);
+	} else
+		pr_debug("virtio-fs: Did not find waiting request"
+				" with unique=0x%llx\n", out_args->unique);
+
+	return 0;
+}
+
+static int virtio_fs_handle_notify(struct virtio_fs *vfs,
+				   struct virtio_fs_notify *notify)
+{
+	int ret = 0;
+	struct fuse_out_header *oh = &notify->out_hdr;
+	struct fuse_notify_lock_out *lo;
+
+	/*
+	 * For notifications, oh.unique is 0 and oh->error contains code
+	 * for which notification as arrived.
+	 */
+	switch (oh->error) {
+	case FUSE_NOTIFY_LOCK:
+		lo = (struct fuse_notify_lock_out *) &notify->outarg;
+		notify_complete_waiting_req(vfs, lo);
+		break;
+	default:
+		pr_err("virtio-fs: Unexpected notification %d\n", oh->error);
+	}
+	return ret;
+}
+
 static void virtio_fs_notify_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;
+	struct virtio_fs *vfs = vq->vdev->priv;
 	LIST_HEAD(reqs);
 	struct virtio_fs_notify_node *notify, *next;
+	struct fuse_out_header *oh;
 
 	spin_lock(&fsvq->lock);
 	do {
@@ -588,6 +650,10 @@ static void virtio_fs_notify_done_work(struct work_struct *work)
 
 	/* Process notify */
 	list_for_each_entry_safe(notify, next, &reqs, list) {
+		oh = &notify->notify.out_hdr;
+		WARN_ON(oh->unique);
+		/* Handle notification */
+		virtio_fs_handle_notify(vfs, &notify->notify);
 		spin_lock(&fsvq->lock);
 		dec_in_flight_req(fsvq);
 		list_del_init(&notify->list);
@@ -688,6 +754,14 @@ static void virtio_fs_request_complete(struct fuse_req *req,
 	 * TODO verify that server properly follows FUSE protocol
 	 * (oh.uniq, oh.len)
 	 */
+	if (req->out.h.error == 1) {
+		/* Wait for notification to complete request */
+		spin_lock(&fsvq->lock);
+		list_add_tail(&req->list, &fsvq->wait_reqs);
+		spin_unlock(&fsvq->lock);
+		return;
+	}
+
 	args = req->args;
 	copy_args_from_argbuf(args, req);
 
@@ -787,6 +861,7 @@ static int virtio_fs_init_vq(struct virtio_fs *fs, struct virtio_fs_vq *fsvq,
 	strncpy(fsvq->name, name, VQ_NAME_LEN);
 	spin_lock_init(&fsvq->lock);
 	INIT_LIST_HEAD(&fsvq->queued_reqs);
+	INIT_LIST_HEAD(&fsvq->wait_reqs);
 	INIT_LIST_HEAD(&fsvq->end_reqs);
 	INIT_LIST_HEAD(&fsvq->notify_reqs);
 	init_completion(&fsvq->in_flight_zero);
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 271ae90a9bb7..ae6b3fcd1fa7 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -525,6 +525,7 @@ enum fuse_notify_code {
 	FUSE_NOTIFY_STORE = 4,
 	FUSE_NOTIFY_RETRIEVE = 5,
 	FUSE_NOTIFY_DELETE = 6,
+	FUSE_NOTIFY_LOCK = 7,
 	FUSE_NOTIFY_CODE_MAX,
 };
 
@@ -916,6 +917,12 @@ struct fuse_notify_retrieve_in {
 	uint64_t	dummy4;
 };
 
+struct fuse_notify_lock_out {
+	uint64_t	unique;
+	int32_t		error;
+	int32_t		padding;
+};
+
 /* Device ioctls: */
 #define FUSE_DEV_IOC_MAGIC		229
 #define FUSE_DEV_IOC_CLONE		_IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t)
-- 
2.27.0

Re: [PATCH 1/3] virtiofs: Add an index to keep track of first request queue

[Miklos Szeredi]

On Wed, 16 Jun 2021 at 18:09, Ioannis Angelakopoulos
<iangelak@redhat.com> wrote:
>
> From: Vivek Goyal <vgoyal@redhat.com>
>
> We have many virtqueues and first queue which carries fuse normal requests
> (except forget requests) has index pointed to by enum VQ_REQUEST. This
> works fine as long as number of queues are not dynamic.
>
> I am about to introduce one more virtqueue, called notification queue,
> which will be present only if device on host supports it. That means index
> of request queue will change depending on if notification queue is present
> or not.
>
> So, add a variable to keep track of that index and this will help when
> notification queue is added in next patch.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index bcb8a02e2d8b..a545e31cf1ae 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -61,6 +61,7 @@ struct virtio_fs {
>         unsigned int nvqs;               /* number of virtqueues */
>         unsigned int num_request_queues; /* number of request queues */
>         struct dax_device *dax_dev;
> +       unsigned int first_reqq_idx;     /* First request queue idx */
>
>         /* DAX memory window where file contents are mapped */
>         void *window_kaddr;
> @@ -681,7 +682,9 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
>         if (fs->num_request_queues == 0)
>                 return -EINVAL;
>
> -       fs->nvqs = VQ_REQUEST + fs->num_request_queues;

Okay, so VQ_REQUEST now completely lost it's meaning as an index into
fs->vqs[] array, but VQ_HIPRIO is still used that way.  This looks
confusing.  Let's just get rid of VQ_REQUEST/VQ_HIPRIO completely, and
add "#define VQ_HIPRIO_IDX 0".

> +       /* One hiprio queue and rest are request queues */
> +       fs->nvqs = 1 + fs->num_request_queues;
> +       fs->first_reqq_idx = 1;
>         fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
>         if (!fs->vqs)
>                 return -ENOMEM;
> @@ -701,10 +704,11 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
>         names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
>
>         /* Initialize the requests virtqueues */
> -       for (i = VQ_REQUEST; i < fs->nvqs; i++) {
> +       for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
>                 char vq_name[VQ_NAME_LEN];
>
> -               snprintf(vq_name, VQ_NAME_LEN, "requests.%u", i - VQ_REQUEST);
> +               snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
> +                        i - fs->first_reqq_idx);
>                 virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
>                 callbacks[i] = virtio_fs_vq_done;
>                 names[i] = fs->vqs[i].name;
> @@ -1225,7 +1229,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>  static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
>  __releases(fiq->lock)
>  {
> -       unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> +       unsigned int queue_id;
>         struct virtio_fs *fs;
>         struct fuse_req *req;
>         struct virtio_fs_vq *fsvq;
> @@ -1239,6 +1243,7 @@ __releases(fiq->lock)
>         spin_unlock(&fiq->lock);
>
>         fs = fiq->priv;
> +       queue_id = fs->first_reqq_idx;
>
>         pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
>                   __func__, req->in.h.opcode, req->in.h.unique,
> @@ -1316,7 +1321,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>
>         err = -ENOMEM;
>         /* Allocate fuse_dev for hiprio and notification queues */
> -       for (i = 0; i < fs->nvqs; i++) {
> +       for (i = 0; i < fs->first_reqq_idx; i++) {

Previous code didn't seem to do what comment said, while new code
does.  So while the change seems correct, it should go into a separate
patch with explanation.

>                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
>
>                 fsvq->fud = fuse_dev_alloc();
> @@ -1325,7 +1330,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>         }
>
>         /* virtiofs allocates and installs its own fuse devices */
> -       ctx->fudptr = NULL;
> +       ctx->fudptr = (void **)&fs->vqs[fs->first_reqq_idx].fud;

I don't understand this.

>         if (ctx->dax) {
>                 if (!fs->dax_dev) {
>                         err = -EINVAL;
> @@ -1339,9 +1344,14 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>         if (err < 0)
>                 goto err_free_fuse_devs;
>
> +       fc = fs->vqs[fs->first_reqq_idx].fud->fc;
> +

Nor this.

>         for (i = 0; i < fs->nvqs; i++) {
>                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
>
> +               if (i == fs->first_reqq_idx)
> +                       continue;
> +

Nor this.    There's something subtle going on here, that's not
mentioned in the patch header.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 1/3] virtiofs: Add an index to keep track of first request queue

[Miklos Szeredi]

On Wed, 16 Jun 2021 at 18:09, Ioannis Angelakopoulos
<iangelak@redhat.com> wrote:
>
> From: Vivek Goyal <vgoyal@redhat.com>
>
> We have many virtqueues and first queue which carries fuse normal requests
> (except forget requests) has index pointed to by enum VQ_REQUEST. This
> works fine as long as number of queues are not dynamic.
>
> I am about to introduce one more virtqueue, called notification queue,
> which will be present only if device on host supports it. That means index
> of request queue will change depending on if notification queue is present
> or not.
>
> So, add a variable to keep track of that index and this will help when
> notification queue is added in next patch.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index bcb8a02e2d8b..a545e31cf1ae 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -61,6 +61,7 @@ struct virtio_fs {
>         unsigned int nvqs;               /* number of virtqueues */
>         unsigned int num_request_queues; /* number of request queues */
>         struct dax_device *dax_dev;
> +       unsigned int first_reqq_idx;     /* First request queue idx */
>
>         /* DAX memory window where file contents are mapped */
>         void *window_kaddr;
> @@ -681,7 +682,9 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
>         if (fs->num_request_queues == 0)
>                 return -EINVAL;
>
> -       fs->nvqs = VQ_REQUEST + fs->num_request_queues;

Okay, so VQ_REQUEST now completely lost it's meaning as an index into
fs->vqs[] array, but VQ_HIPRIO is still used that way.  This looks
confusing.  Let's just get rid of VQ_REQUEST/VQ_HIPRIO completely, and
add "#define VQ_HIPRIO_IDX 0".

> +       /* One hiprio queue and rest are request queues */
> +       fs->nvqs = 1 + fs->num_request_queues;
> +       fs->first_reqq_idx = 1;
>         fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
>         if (!fs->vqs)
>                 return -ENOMEM;
> @@ -701,10 +704,11 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
>         names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
>
>         /* Initialize the requests virtqueues */
> -       for (i = VQ_REQUEST; i < fs->nvqs; i++) {
> +       for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
>                 char vq_name[VQ_NAME_LEN];
>
> -               snprintf(vq_name, VQ_NAME_LEN, "requests.%u", i - VQ_REQUEST);
> +               snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
> +                        i - fs->first_reqq_idx);
>                 virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
>                 callbacks[i] = virtio_fs_vq_done;
>                 names[i] = fs->vqs[i].name;
> @@ -1225,7 +1229,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>  static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
>  __releases(fiq->lock)
>  {
> -       unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> +       unsigned int queue_id;
>         struct virtio_fs *fs;
>         struct fuse_req *req;
>         struct virtio_fs_vq *fsvq;
> @@ -1239,6 +1243,7 @@ __releases(fiq->lock)
>         spin_unlock(&fiq->lock);
>
>         fs = fiq->priv;
> +       queue_id = fs->first_reqq_idx;
>
>         pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
>                   __func__, req->in.h.opcode, req->in.h.unique,
> @@ -1316,7 +1321,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>
>         err = -ENOMEM;
>         /* Allocate fuse_dev for hiprio and notification queues */
> -       for (i = 0; i < fs->nvqs; i++) {
> +       for (i = 0; i < fs->first_reqq_idx; i++) {

Previous code didn't seem to do what comment said, while new code
does.  So while the change seems correct, it should go into a separate
patch with explanation.

>                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
>
>                 fsvq->fud = fuse_dev_alloc();
> @@ -1325,7 +1330,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>         }
>
>         /* virtiofs allocates and installs its own fuse devices */
> -       ctx->fudptr = NULL;
> +       ctx->fudptr = (void **)&fs->vqs[fs->first_reqq_idx].fud;

I don't understand this.

>         if (ctx->dax) {
>                 if (!fs->dax_dev) {
>                         err = -EINVAL;
> @@ -1339,9 +1344,14 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>         if (err < 0)
>                 goto err_free_fuse_devs;
>
> +       fc = fs->vqs[fs->first_reqq_idx].fud->fc;
> +

Nor this.

>         for (i = 0; i < fs->nvqs; i++) {
>                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
>
> +               if (i == fs->first_reqq_idx)
> +                       continue;
> +

Nor this.    There's something subtle going on here, that's not
mentioned in the patch header.

Thanks,
Miklos

Re: [PATCH 1/3] virtiofs: Add an index to keep track of first request queue

[Vivek Goyal]

On Fri, Jun 18, 2021 at 09:43:36AM +0200, Miklos Szeredi wrote:
> On Wed, 16 Jun 2021 at 18:09, Ioannis Angelakopoulos
> <iangelak@redhat.com> wrote:
> >
> > From: Vivek Goyal <vgoyal@redhat.com>
> >
> > We have many virtqueues and first queue which carries fuse normal requests
> > (except forget requests) has index pointed to by enum VQ_REQUEST. This
> > works fine as long as number of queues are not dynamic.
> >
> > I am about to introduce one more virtqueue, called notification queue,
> > which will be present only if device on host supports it. That means index
> > of request queue will change depending on if notification queue is present
> > or not.
> >
> > So, add a variable to keep track of that index and this will help when
> > notification queue is added in next patch.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index bcb8a02e2d8b..a545e31cf1ae 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -61,6 +61,7 @@ struct virtio_fs {
> >         unsigned int nvqs;               /* number of virtqueues */
> >         unsigned int num_request_queues; /* number of request queues */
> >         struct dax_device *dax_dev;
> > +       unsigned int first_reqq_idx;     /* First request queue idx */
> >
> >         /* DAX memory window where file contents are mapped */
> >         void *window_kaddr;
> > @@ -681,7 +682,9 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
> >         if (fs->num_request_queues == 0)
> >                 return -EINVAL;
> >
> > -       fs->nvqs = VQ_REQUEST + fs->num_request_queues;
> 
> Okay, so VQ_REQUEST now completely lost it's meaning as an index into
> fs->vqs[] array, but VQ_HIPRIO is still used that way.  This looks
> confusing.  Let's just get rid of VQ_REQUEST/VQ_HIPRIO completely, and
> add "#define VQ_HIPRIO_IDX 0".

Hi Miklos,

Will do.

> 
> > +       /* One hiprio queue and rest are request queues */
> > +       fs->nvqs = 1 + fs->num_request_queues;
> > +       fs->first_reqq_idx = 1;
> >         fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
> >         if (!fs->vqs)
> >                 return -ENOMEM;
> > @@ -701,10 +704,11 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
> >         names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
> >
> >         /* Initialize the requests virtqueues */
> > -       for (i = VQ_REQUEST; i < fs->nvqs; i++) {
> > +       for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
> >                 char vq_name[VQ_NAME_LEN];
> >
> > -               snprintf(vq_name, VQ_NAME_LEN, "requests.%u", i - VQ_REQUEST);
> > +               snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
> > +                        i - fs->first_reqq_idx);
> >                 virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
> >                 callbacks[i] = virtio_fs_vq_done;
> >                 names[i] = fs->vqs[i].name;
> > @@ -1225,7 +1229,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> >  static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
> >  __releases(fiq->lock)
> >  {
> > -       unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> > +       unsigned int queue_id;
> >         struct virtio_fs *fs;
> >         struct fuse_req *req;
> >         struct virtio_fs_vq *fsvq;
> > @@ -1239,6 +1243,7 @@ __releases(fiq->lock)
> >         spin_unlock(&fiq->lock);
> >
> >         fs = fiq->priv;
> > +       queue_id = fs->first_reqq_idx;
> >
> >         pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
> >                   __func__, req->in.h.opcode, req->in.h.unique,
> > @@ -1316,7 +1321,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
> >
> >         err = -ENOMEM;
> >         /* Allocate fuse_dev for hiprio and notification queues */
> > -       for (i = 0; i < fs->nvqs; i++) {
> > +       for (i = 0; i < fs->first_reqq_idx; i++) {
> 
> Previous code didn't seem to do what comment said, while new code
> does.  So while the change seems correct, it should go into a separate
> patch with explanation.

I think this patch needs to be updated. It was correct when I had posted
it back then. But since then we have changed virtiofs code.

Initially we used to allocate fuse device only for hiprio queue and
for request queue fuse_fill_super_common() used to allocate the device.

It was confusing, so I added one patch so that for all virtiofs queues,
virtiofs will allocate fuse device and fuse common code will not allocate
fuse device.

commit 7fd3abfa8dd7c08ecacd25b2f9f9e1d3fb642440
Author: Vivek Goyal <vgoyal@redhat.com>
Date:   Mon May 4 14:33:15 2020 -0400

    virtiofs: do not use fuse_fill_super_common() for device installation

So this patch now needs to be udpated so that it works with current
code. I will fix it.

> 
> >                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
> >
> >                 fsvq->fud = fuse_dev_alloc();
> > @@ -1325,7 +1330,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
> >         }
> >
> >         /* virtiofs allocates and installs its own fuse devices */
> > -       ctx->fudptr = NULL;
> > +       ctx->fudptr = (void **)&fs->vqs[fs->first_reqq_idx].fud;
> 
> I don't understand this.

This is also vestige of old code. We used to pass pointer to the location
where fuse common code should install fuse device.

ctx.fudptr = (void **)&fs->vqs[VQ_REQUEST].fud;

Now this code should not be needed. I will clean this up.

> 
> >         if (ctx->dax) {
> >                 if (!fs->dax_dev) {
> >                         err = -EINVAL;
> > @@ -1339,9 +1344,14 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
> >         if (err < 0)
> >                 goto err_free_fuse_devs;
> >
> > +       fc = fs->vqs[fs->first_reqq_idx].fud->fc;
> > +
> 
> Nor this.
> 
> >         for (i = 0; i < fs->nvqs; i++) {
> >                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
> >
> > +               if (i == fs->first_reqq_idx)
> > +                       continue;
> > +
> 
> Nor this.    There's something subtle going on here, that's not
> mentioned in the patch header.

Will fix all this. I think all this is due to old code we had about
fuse device handling.

Thanks
Vivek

Re: [Virtio-fs] [PATCH 1/3] virtiofs: Add an index to keep track of first request queue

[Vivek Goyal]

On Fri, Jun 18, 2021 at 09:43:36AM +0200, Miklos Szeredi wrote:
> On Wed, 16 Jun 2021 at 18:09, Ioannis Angelakopoulos
> <iangelak@redhat.com> wrote:
> >
> > From: Vivek Goyal <vgoyal@redhat.com>
> >
> > We have many virtqueues and first queue which carries fuse normal requests
> > (except forget requests) has index pointed to by enum VQ_REQUEST. This
> > works fine as long as number of queues are not dynamic.
> >
> > I am about to introduce one more virtqueue, called notification queue,
> > which will be present only if device on host supports it. That means index
> > of request queue will change depending on if notification queue is present
> > or not.
> >
> > So, add a variable to keep track of that index and this will help when
> > notification queue is added in next patch.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index bcb8a02e2d8b..a545e31cf1ae 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -61,6 +61,7 @@ struct virtio_fs {
> >         unsigned int nvqs;               /* number of virtqueues */
> >         unsigned int num_request_queues; /* number of request queues */
> >         struct dax_device *dax_dev;
> > +       unsigned int first_reqq_idx;     /* First request queue idx */
> >
> >         /* DAX memory window where file contents are mapped */
> >         void *window_kaddr;
> > @@ -681,7 +682,9 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
> >         if (fs->num_request_queues == 0)
> >                 return -EINVAL;
> >
> > -       fs->nvqs = VQ_REQUEST + fs->num_request_queues;
> 
> Okay, so VQ_REQUEST now completely lost it's meaning as an index into
> fs->vqs[] array, but VQ_HIPRIO is still used that way.  This looks
> confusing.  Let's just get rid of VQ_REQUEST/VQ_HIPRIO completely, and
> add "#define VQ_HIPRIO_IDX 0".

Hi Miklos,

Will do.

> 
> > +       /* One hiprio queue and rest are request queues */
> > +       fs->nvqs = 1 + fs->num_request_queues;
> > +       fs->first_reqq_idx = 1;
> >         fs->vqs = kcalloc(fs->nvqs, sizeof(fs->vqs[VQ_HIPRIO]), GFP_KERNEL);
> >         if (!fs->vqs)
> >                 return -ENOMEM;
> > @@ -701,10 +704,11 @@ static int virtio_fs_setup_vqs(struct virtio_device *vdev,
> >         names[VQ_HIPRIO] = fs->vqs[VQ_HIPRIO].name;
> >
> >         /* Initialize the requests virtqueues */
> > -       for (i = VQ_REQUEST; i < fs->nvqs; i++) {
> > +       for (i = fs->first_reqq_idx; i < fs->nvqs; i++) {
> >                 char vq_name[VQ_NAME_LEN];
> >
> > -               snprintf(vq_name, VQ_NAME_LEN, "requests.%u", i - VQ_REQUEST);
> > +               snprintf(vq_name, VQ_NAME_LEN, "requests.%u",
> > +                        i - fs->first_reqq_idx);
> >                 virtio_fs_init_vq(&fs->vqs[i], vq_name, VQ_REQUEST);
> >                 callbacks[i] = virtio_fs_vq_done;
> >                 names[i] = fs->vqs[i].name;
> > @@ -1225,7 +1229,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> >  static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
> >  __releases(fiq->lock)
> >  {
> > -       unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
> > +       unsigned int queue_id;
> >         struct virtio_fs *fs;
> >         struct fuse_req *req;
> >         struct virtio_fs_vq *fsvq;
> > @@ -1239,6 +1243,7 @@ __releases(fiq->lock)
> >         spin_unlock(&fiq->lock);
> >
> >         fs = fiq->priv;
> > +       queue_id = fs->first_reqq_idx;
> >
> >         pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
> >                   __func__, req->in.h.opcode, req->in.h.unique,
> > @@ -1316,7 +1321,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
> >
> >         err = -ENOMEM;
> >         /* Allocate fuse_dev for hiprio and notification queues */
> > -       for (i = 0; i < fs->nvqs; i++) {
> > +       for (i = 0; i < fs->first_reqq_idx; i++) {
> 
> Previous code didn't seem to do what comment said, while new code
> does.  So while the change seems correct, it should go into a separate
> patch with explanation.

I think this patch needs to be updated. It was correct when I had posted
it back then. But since then we have changed virtiofs code.

Initially we used to allocate fuse device only for hiprio queue and
for request queue fuse_fill_super_common() used to allocate the device.

It was confusing, so I added one patch so that for all virtiofs queues,
virtiofs will allocate fuse device and fuse common code will not allocate
fuse device.

commit 7fd3abfa8dd7c08ecacd25b2f9f9e1d3fb642440
Author: Vivek Goyal <vgoyal@redhat.com>
Date:   Mon May 4 14:33:15 2020 -0400

    virtiofs: do not use fuse_fill_super_common() for device installation

So this patch now needs to be udpated so that it works with current
code. I will fix it.

> 
> >                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
> >
> >                 fsvq->fud = fuse_dev_alloc();
> > @@ -1325,7 +1330,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
> >         }
> >
> >         /* virtiofs allocates and installs its own fuse devices */
> > -       ctx->fudptr = NULL;
> > +       ctx->fudptr = (void **)&fs->vqs[fs->first_reqq_idx].fud;
> 
> I don't understand this.

This is also vestige of old code. We used to pass pointer to the location
where fuse common code should install fuse device.

ctx.fudptr = (void **)&fs->vqs[VQ_REQUEST].fud;

Now this code should not be needed. I will clean this up.

> 
> >         if (ctx->dax) {
> >                 if (!fs->dax_dev) {
> >                         err = -EINVAL;
> > @@ -1339,9 +1344,14 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
> >         if (err < 0)
> >                 goto err_free_fuse_devs;
> >
> > +       fc = fs->vqs[fs->first_reqq_idx].fud->fc;
> > +
> 
> Nor this.
> 
> >         for (i = 0; i < fs->nvqs; i++) {
> >                 struct virtio_fs_vq *fsvq = &fs->vqs[i];
> >
> > +               if (i == fs->first_reqq_idx)
> > +                       continue;
> > +
> 
> Nor this.    There's something subtle going on here, that's not
> mentioned in the patch header.

Will fix all this. I think all this is due to old code we had about
fuse device handling.

Thanks
Vivek