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

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 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

[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 --]