[PATCH v1 1/2] virtio_fs: introduce virtio_fs_put_locked helper

[PATCH v1 1/2] virtio_fs: introduce virtio_fs_put_locked helper

[Max Gurtovoy]

Introduce a new helper function virtio_fs_put_locked to encapsulate the
common pattern of releasing a virtio_fs reference while holding a lock.
The existing virtio_fs_put helper will be used to release a virtio_fs
reference while not holding a lock.

Also add an assertion in case the lock is not taken when it should.

Reviewed-by: Idan Zach <izach@nvidia.com>
Reviewed-by: Shai Malin <smalin@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 fs/fuse/virtio_fs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index dd5260141615..43f7be1d7887 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -201,18 +201,25 @@ static const struct kobj_type virtio_fs_ktype = {
 };
 
 /* Make sure virtiofs_mutex is held */
-static void virtio_fs_put(struct virtio_fs *fs)
+static void virtio_fs_put_locked(struct virtio_fs *fs)
 {
+	lockdep_assert_held(&virtio_fs_mutex);
+
 	kobject_put(&fs->kobj);
 }
 
+static void virtio_fs_put(struct virtio_fs *fs)
+{
+	mutex_lock(&virtio_fs_mutex);
+	virtio_fs_put_locked(fs);
+	mutex_unlock(&virtio_fs_mutex);
+}
+
 static void virtio_fs_fiq_release(struct fuse_iqueue *fiq)
 {
 	struct virtio_fs *vfs = fiq->priv;
 
-	mutex_lock(&virtio_fs_mutex);
 	virtio_fs_put(vfs);
-	mutex_unlock(&virtio_fs_mutex);
 }
 
 static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
@@ -1052,7 +1059,7 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 
 	vdev->priv = NULL;
 	/* Put device reference on virtio_fs object */
-	virtio_fs_put(fs);
+	virtio_fs_put_locked(fs);
 	mutex_unlock(&virtio_fs_mutex);
 }
 
@@ -1596,9 +1603,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 
 out_err:
 	kfree(fc);
-	mutex_lock(&virtio_fs_mutex);
 	virtio_fs_put(fs);
-	mutex_unlock(&virtio_fs_mutex);
 	return err;
 }
 
-- 
2.18.1

[PATCH v1 2/2] virtio_fs: add sysfs entries for queue information

[Max Gurtovoy]

Introduce sysfs entries to provide visibility to the multiple queues
used by the Virtio FS device. This enhancement allows users to query
information about these queues.

Specifically, add two sysfs entries:
1. Queue name: Provides the name of each queue (e.g. hiprio/requests.8).
2. CPU list: Shows the list of CPUs that can process requests for each
queue.

The CPU list feature is inspired by similar functionality in the block
MQ layer, which provides analogous sysfs entries for block devices.

These new sysfs entries will improve observability and aid in debugging
and performance tuning of Virtio FS devices.

Reviewed-by: Idan Zach <izach@nvidia.com>
Reviewed-by: Shai Malin <smalin@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 fs/fuse/virtio_fs.c | 147 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 139 insertions(+), 8 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 43f7be1d7887..78f579463cca 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -56,12 +56,14 @@ struct virtio_fs_vq {
 	bool connected;
 	long in_flight;
 	struct completion in_flight_zero; /* No inflight requests */
+	struct kobject *kobj;
 	char name[VQ_NAME_LEN];
 } ____cacheline_aligned_in_smp;
 
 /* A virtio-fs device instance */
 struct virtio_fs {
 	struct kobject kobj;
+	struct kobject *mqs_kobj;
 	struct list_head list;    /* on virtio_fs_instances */
 	char *tag;
 	struct virtio_fs_vq *vqs;
@@ -200,6 +202,74 @@ static const struct kobj_type virtio_fs_ktype = {
 	.default_groups = virtio_fs_groups,
 };
 
+static struct virtio_fs_vq *virtio_fs_kobj_to_vq(struct virtio_fs *fs,
+		struct kobject *kobj)
+{
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		if (kobj == fs->vqs[i].kobj)
+			return &fs->vqs[i];
+	}
+	return NULL;
+}
+
+static ssize_t name_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct virtio_fs *fs = container_of(kobj->parent->parent, struct virtio_fs, kobj);
+	struct virtio_fs_vq *fsvq = virtio_fs_kobj_to_vq(fs, kobj);
+
+	if (!fsvq)
+		return -EINVAL;
+	return sysfs_emit(buf, "%s\n", fsvq->name);
+}
+
+static struct kobj_attribute virtio_fs_vq_name_attr = __ATTR_RO(name);
+
+static ssize_t cpu_list_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct virtio_fs *fs = container_of(kobj->parent->parent, struct virtio_fs, kobj);
+	struct virtio_fs_vq *fsvq = virtio_fs_kobj_to_vq(fs, kobj);
+	unsigned int cpu, qid;
+	const size_t size = PAGE_SIZE - 1;
+	bool first = true;
+	int ret = 0, pos = 0;
+
+	if (!fsvq)
+		return -EINVAL;
+
+	qid = fsvq->vq->index;
+	for (cpu = 0; cpu < nr_cpu_ids; cpu++) {
+		if (qid < VQ_REQUEST || (fs->mq_map[cpu] == qid - VQ_REQUEST)) {
+			if (first)
+				ret = snprintf(buf + pos, size - pos, "%u", cpu);
+			else
+				ret = snprintf(buf + pos, size - pos, ", %u", cpu);
+
+			if (ret >= size - pos)
+				break;
+			first = false;
+			pos += ret;
+		}
+	}
+	ret = snprintf(buf + pos, size + 1 - pos, "\n");
+	return pos + ret;
+}
+
+static struct kobj_attribute virtio_fs_vq_cpu_list_attr = __ATTR_RO(cpu_list);
+
+static struct attribute *virtio_fs_vq_attrs[] = {
+	&virtio_fs_vq_name_attr.attr,
+	&virtio_fs_vq_cpu_list_attr.attr,
+	NULL
+};
+
+static struct attribute_group virtio_fs_vq_attr_group = {
+	.attrs = virtio_fs_vq_attrs,
+};
+
 /* Make sure virtiofs_mutex is held */
 static void virtio_fs_put_locked(struct virtio_fs *fs)
 {
@@ -280,6 +350,50 @@ static void virtio_fs_start_all_queues(struct virtio_fs *fs)
 	}
 }
 
+static void virtio_fs_delete_queues_sysfs(struct virtio_fs *fs)
+{
+	struct virtio_fs_vq *fsvq;
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		fsvq = &fs->vqs[i];
+		kobject_put(fsvq->kobj);
+	}
+}
+
+static int virtio_fs_add_queues_sysfs(struct virtio_fs *fs)
+{
+	struct virtio_fs_vq *fsvq;
+	char buff[12];
+	int i, j, ret;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		fsvq = &fs->vqs[i];
+
+		sprintf(buff, "%d", i);
+		fsvq->kobj = kobject_create_and_add(buff, fs->mqs_kobj);
+		if (!fs->mqs_kobj) {
+			ret = -ENOMEM;
+			goto out_del;
+		}
+
+		ret = sysfs_create_group(fsvq->kobj, &virtio_fs_vq_attr_group);
+		if (ret) {
+			kobject_put(fsvq->kobj);
+			goto out_del;
+		}
+	}
+
+	return 0;
+
+out_del:
+	for (j = 0; j < i; j++) {
+		fsvq = &fs->vqs[j];
+		kobject_put(fsvq->kobj);
+	}
+	return ret;
+}
+
 /* Add a new instance to the list or return -EEXIST if tag name exists*/
 static int virtio_fs_add_instance(struct virtio_device *vdev,
 				  struct virtio_fs *fs)
@@ -303,17 +417,22 @@ static int virtio_fs_add_instance(struct virtio_device *vdev,
 	 */
 	fs->kobj.kset = virtio_fs_kset;
 	ret = kobject_add(&fs->kobj, NULL, "%d", vdev->index);
-	if (ret < 0) {
-		mutex_unlock(&virtio_fs_mutex);
-		return ret;
+	if (ret < 0)
+		goto out_unlock;
+
+	fs->mqs_kobj = kobject_create_and_add("mqs", &fs->kobj);
+	if (!fs->mqs_kobj) {
+		ret = -ENOMEM;
+		goto out_del;
 	}
 
 	ret = sysfs_create_link(&fs->kobj, &vdev->dev.kobj, "device");
-	if (ret < 0) {
-		kobject_del(&fs->kobj);
-		mutex_unlock(&virtio_fs_mutex);
-		return ret;
-	}
+	if (ret < 0)
+		goto out_put;
+
+	ret = virtio_fs_add_queues_sysfs(fs);
+	if (ret)
+		goto out_remove;
 
 	list_add_tail(&fs->list, &virtio_fs_instances);
 
@@ -322,6 +441,16 @@ static int virtio_fs_add_instance(struct virtio_device *vdev,
 	kobject_uevent(&fs->kobj, KOBJ_ADD);
 
 	return 0;
+
+out_remove:
+	sysfs_remove_link(&fs->kobj, "device");
+out_put:
+	kobject_put(fs->mqs_kobj);
+out_del:
+	kobject_del(&fs->kobj);
+out_unlock:
+	mutex_unlock(&virtio_fs_mutex);
+	return ret;
 }
 
 /* Return the virtio_fs with a given tag, or NULL */
@@ -1050,7 +1179,9 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 	mutex_lock(&virtio_fs_mutex);
 	/* This device is going away. No one should get new reference */
 	list_del_init(&fs->list);
+	virtio_fs_delete_queues_sysfs(fs);
 	sysfs_remove_link(&fs->kobj, "device");
+	kobject_put(fs->mqs_kobj);
 	kobject_del(&fs->kobj);
 	virtio_fs_stop_all_queues(fs);
 	virtio_fs_drain_all_queues_locked(fs);
-- 
2.18.1

Re: [PATCH v1 2/2] virtio_fs: add sysfs entries for queue information

[Michael S. Tsirkin]

Cc: "Eugenio Pérez" <eperezma@redhat.com>


On Sun, Aug 25, 2024 at 04:07:16PM +0300, Max Gurtovoy wrote:
> Introduce sysfs entries to provide visibility to the multiple queues
> used by the Virtio FS device. This enhancement allows users to query
> information about these queues.
> 
> Specifically, add two sysfs entries:
> 1. Queue name: Provides the name of each queue (e.g. hiprio/requests.8).
> 2. CPU list: Shows the list of CPUs that can process requests for each
> queue.
> 
> The CPU list feature is inspired by similar functionality in the block
> MQ layer, which provides analogous sysfs entries for block devices.
> 
> These new sysfs entries will improve observability and aid in debugging
> and performance tuning of Virtio FS devices.
> 
> Reviewed-by: Idan Zach <izach@nvidia.com>
> Reviewed-by: Shai Malin <smalin@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  fs/fuse/virtio_fs.c | 147 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 139 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 43f7be1d7887..78f579463cca 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -56,12 +56,14 @@ struct virtio_fs_vq {
>  	bool connected;
>  	long in_flight;
>  	struct completion in_flight_zero; /* No inflight requests */
> +	struct kobject *kobj;
>  	char name[VQ_NAME_LEN];
>  } ____cacheline_aligned_in_smp;
>  
>  /* A virtio-fs device instance */
>  struct virtio_fs {
>  	struct kobject kobj;
> +	struct kobject *mqs_kobj;
>  	struct list_head list;    /* on virtio_fs_instances */
>  	char *tag;
>  	struct virtio_fs_vq *vqs;
> @@ -200,6 +202,74 @@ static const struct kobj_type virtio_fs_ktype = {
>  	.default_groups = virtio_fs_groups,
>  };
>  
> +static struct virtio_fs_vq *virtio_fs_kobj_to_vq(struct virtio_fs *fs,
> +		struct kobject *kobj)
> +{
> +	int i;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		if (kobj == fs->vqs[i].kobj)
> +			return &fs->vqs[i];
> +	}
> +	return NULL;
> +}
> +
> +static ssize_t name_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct virtio_fs *fs = container_of(kobj->parent->parent, struct virtio_fs, kobj);
> +	struct virtio_fs_vq *fsvq = virtio_fs_kobj_to_vq(fs, kobj);
> +
> +	if (!fsvq)
> +		return -EINVAL;
> +	return sysfs_emit(buf, "%s\n", fsvq->name);
> +}
> +
> +static struct kobj_attribute virtio_fs_vq_name_attr = __ATTR_RO(name);
> +
> +static ssize_t cpu_list_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct virtio_fs *fs = container_of(kobj->parent->parent, struct virtio_fs, kobj);
> +	struct virtio_fs_vq *fsvq = virtio_fs_kobj_to_vq(fs, kobj);
> +	unsigned int cpu, qid;
> +	const size_t size = PAGE_SIZE - 1;
> +	bool first = true;
> +	int ret = 0, pos = 0;
> +
> +	if (!fsvq)
> +		return -EINVAL;
> +
> +	qid = fsvq->vq->index;
> +	for (cpu = 0; cpu < nr_cpu_ids; cpu++) {
> +		if (qid < VQ_REQUEST || (fs->mq_map[cpu] == qid - VQ_REQUEST)) {
> +			if (first)
> +				ret = snprintf(buf + pos, size - pos, "%u", cpu);
> +			else
> +				ret = snprintf(buf + pos, size - pos, ", %u", cpu);
> +
> +			if (ret >= size - pos)
> +				break;
> +			first = false;
> +			pos += ret;
> +		}
> +	}
> +	ret = snprintf(buf + pos, size + 1 - pos, "\n");
> +	return pos + ret;
> +}
> +
> +static struct kobj_attribute virtio_fs_vq_cpu_list_attr = __ATTR_RO(cpu_list);
> +
> +static struct attribute *virtio_fs_vq_attrs[] = {
> +	&virtio_fs_vq_name_attr.attr,
> +	&virtio_fs_vq_cpu_list_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group virtio_fs_vq_attr_group = {
> +	.attrs = virtio_fs_vq_attrs,
> +};
> +
>  /* Make sure virtiofs_mutex is held */
>  static void virtio_fs_put_locked(struct virtio_fs *fs)
>  {
> @@ -280,6 +350,50 @@ static void virtio_fs_start_all_queues(struct virtio_fs *fs)
>  	}
>  }
>  
> +static void virtio_fs_delete_queues_sysfs(struct virtio_fs *fs)
> +{
> +	struct virtio_fs_vq *fsvq;
> +	int i;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		fsvq = &fs->vqs[i];
> +		kobject_put(fsvq->kobj);
> +	}
> +}
> +
> +static int virtio_fs_add_queues_sysfs(struct virtio_fs *fs)
> +{
> +	struct virtio_fs_vq *fsvq;
> +	char buff[12];
> +	int i, j, ret;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		fsvq = &fs->vqs[i];
> +
> +		sprintf(buff, "%d", i);
> +		fsvq->kobj = kobject_create_and_add(buff, fs->mqs_kobj);
> +		if (!fs->mqs_kobj) {
> +			ret = -ENOMEM;
> +			goto out_del;
> +		}
> +
> +		ret = sysfs_create_group(fsvq->kobj, &virtio_fs_vq_attr_group);
> +		if (ret) {
> +			kobject_put(fsvq->kobj);
> +			goto out_del;
> +		}
> +	}
> +
> +	return 0;
> +
> +out_del:
> +	for (j = 0; j < i; j++) {
> +		fsvq = &fs->vqs[j];
> +		kobject_put(fsvq->kobj);
> +	}
> +	return ret;
> +}
> +
>  /* Add a new instance to the list or return -EEXIST if tag name exists*/
>  static int virtio_fs_add_instance(struct virtio_device *vdev,
>  				  struct virtio_fs *fs)
> @@ -303,17 +417,22 @@ static int virtio_fs_add_instance(struct virtio_device *vdev,
>  	 */
>  	fs->kobj.kset = virtio_fs_kset;
>  	ret = kobject_add(&fs->kobj, NULL, "%d", vdev->index);
> -	if (ret < 0) {
> -		mutex_unlock(&virtio_fs_mutex);
> -		return ret;
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	fs->mqs_kobj = kobject_create_and_add("mqs", &fs->kobj);
> +	if (!fs->mqs_kobj) {
> +		ret = -ENOMEM;
> +		goto out_del;
>  	}
>  
>  	ret = sysfs_create_link(&fs->kobj, &vdev->dev.kobj, "device");
> -	if (ret < 0) {
> -		kobject_del(&fs->kobj);
> -		mutex_unlock(&virtio_fs_mutex);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto out_put;
> +
> +	ret = virtio_fs_add_queues_sysfs(fs);
> +	if (ret)
> +		goto out_remove;
>  
>  	list_add_tail(&fs->list, &virtio_fs_instances);
>  
> @@ -322,6 +441,16 @@ static int virtio_fs_add_instance(struct virtio_device *vdev,
>  	kobject_uevent(&fs->kobj, KOBJ_ADD);
>  
>  	return 0;
> +
> +out_remove:
> +	sysfs_remove_link(&fs->kobj, "device");
> +out_put:
> +	kobject_put(fs->mqs_kobj);
> +out_del:
> +	kobject_del(&fs->kobj);
> +out_unlock:
> +	mutex_unlock(&virtio_fs_mutex);
> +	return ret;
>  }
>  
>  /* Return the virtio_fs with a given tag, or NULL */
> @@ -1050,7 +1179,9 @@ static void virtio_fs_remove(struct virtio_device *vdev)
>  	mutex_lock(&virtio_fs_mutex);
>  	/* This device is going away. No one should get new reference */
>  	list_del_init(&fs->list);
> +	virtio_fs_delete_queues_sysfs(fs);
>  	sysfs_remove_link(&fs->kobj, "device");
> +	kobject_put(fs->mqs_kobj);
>  	kobject_del(&fs->kobj);
>  	virtio_fs_stop_all_queues(fs);
>  	virtio_fs_drain_all_queues_locked(fs);
> -- 
> 2.18.1

Re: [PATCH v1 1/2] virtio_fs: introduce virtio_fs_put_locked helper

[Michael S. Tsirkin]

Cc: "Eugenio Pérez" <eperezma@redhat.com>


On Sun, Aug 25, 2024 at 04:07:15PM +0300, Max Gurtovoy wrote:
> Introduce a new helper function virtio_fs_put_locked to encapsulate the
> common pattern of releasing a virtio_fs reference while holding a lock.
> The existing virtio_fs_put helper will be used to release a virtio_fs
> reference while not holding a lock.
> 
> Also add an assertion in case the lock is not taken when it should.
> 
> Reviewed-by: Idan Zach <izach@nvidia.com>
> Reviewed-by: Shai Malin <smalin@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  fs/fuse/virtio_fs.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index dd5260141615..43f7be1d7887 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -201,18 +201,25 @@ static const struct kobj_type virtio_fs_ktype = {
>  };
>  
>  /* Make sure virtiofs_mutex is held */
> -static void virtio_fs_put(struct virtio_fs *fs)
> +static void virtio_fs_put_locked(struct virtio_fs *fs)
>  {
> +	lockdep_assert_held(&virtio_fs_mutex);
> +
>  	kobject_put(&fs->kobj);
>  }
>  
> +static void virtio_fs_put(struct virtio_fs *fs)
> +{
> +	mutex_lock(&virtio_fs_mutex);
> +	virtio_fs_put_locked(fs);
> +	mutex_unlock(&virtio_fs_mutex);
> +}
> +
>  static void virtio_fs_fiq_release(struct fuse_iqueue *fiq)
>  {
>  	struct virtio_fs *vfs = fiq->priv;
>  
> -	mutex_lock(&virtio_fs_mutex);
>  	virtio_fs_put(vfs);
> -	mutex_unlock(&virtio_fs_mutex);
>  }
>  
>  static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
> @@ -1052,7 +1059,7 @@ static void virtio_fs_remove(struct virtio_device *vdev)
>  
>  	vdev->priv = NULL;
>  	/* Put device reference on virtio_fs object */
> -	virtio_fs_put(fs);
> +	virtio_fs_put_locked(fs);
>  	mutex_unlock(&virtio_fs_mutex);
>  }
>  
> @@ -1596,9 +1603,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
>  
>  out_err:
>  	kfree(fc);
> -	mutex_lock(&virtio_fs_mutex);
>  	virtio_fs_put(fs);
> -	mutex_unlock(&virtio_fs_mutex);
>  	return err;
>  }
>  
> -- 
> 2.18.1

Re: [PATCH v1 1/2] virtio_fs: introduce virtio_fs_put_locked helper

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On Sun, Aug 25, 2024 at 04:07:15PM +0300, Max Gurtovoy wrote:
> Introduce a new helper function virtio_fs_put_locked to encapsulate the
> common pattern of releasing a virtio_fs reference while holding a lock.
> The existing virtio_fs_put helper will be used to release a virtio_fs
> reference while not holding a lock.
> 
> Also add an assertion in case the lock is not taken when it should.
> 
> Reviewed-by: Idan Zach <izach@nvidia.com>
> Reviewed-by: Shai Malin <smalin@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  fs/fuse/virtio_fs.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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