[PATCH v2] virtiofs: Export filesystem tags through sysfs

[PATCH v2] virtiofs: Export filesystem tags through sysfs

[Vivek Goyal]

virtiofs filesystem is mounted using a "tag" which is exported by the
virtiofs device. virtiofs driver knows about all the available tags but
these are not exported to user space.

People have asked these tags to be exported to user space. Most recently
Lennart Poettering has asked for it as he wants to scan the tags and mount
virtiofs automatically in certain cases.

https://gitlab.com/virtio-fs/virtiofsd/-/issues/128

This patch exports tags through sysfs. One tag is associated with each
virtiofs device. A new "tag" file appears under virtiofs device dir.
Actual filesystem tag can be obtained by reading this "tag" file.

For example, if a virtiofs device exports tag "myfs", a new file "tag"
will show up here. Tag has a newline char at the end.

/sys/bus/virtio/devices/virtio<N>/tag

# cat /sys/bus/virtio/devices/virtio<N>/tag
myfs

Note, tag is available at KOBJ_BIND time and not at KOBJ_ADD event time.

v2:
- Add a newline char at the end in tag file. (Alyssa Ross)
- Add a line in commit logs about tag file being available at KOBJ_BIND
  time and not KOBJ_ADD time.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 fs/fuse/virtio_fs.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 5f1be1da92ce..9f76c9697e6f 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -107,6 +107,21 @@ static const struct fs_parameter_spec virtio_fs_parameters[] = {
 	{}
 };
 
+/* Forward Declarations */
+static void virtio_fs_stop_all_queues(struct virtio_fs *fs);
+
+/* sysfs related */
+static ssize_t tag_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct virtio_device *vdev = container_of(dev, struct virtio_device,
+						  dev);
+	struct virtio_fs *fs = vdev->priv;
+
+	return sysfs_emit(buf, "%s\n", fs->tag);
+}
+static DEVICE_ATTR_RO(tag);
+
 static int virtio_fs_parse_param(struct fs_context *fsc,
 				 struct fs_parameter *param)
 {
@@ -265,6 +280,15 @@ static int virtio_fs_add_instance(struct virtio_fs *fs)
 	return 0;
 }
 
+static void virtio_fs_remove_instance(struct virtio_fs *fs)
+{
+	mutex_lock(&virtio_fs_mutex);
+	list_del_init(&fs->list);
+	virtio_fs_stop_all_queues(fs);
+	virtio_fs_drain_all_queues_locked(fs);
+	mutex_unlock(&virtio_fs_mutex);
+}
+
 /* Return the virtio_fs with a given tag, or NULL */
 static struct virtio_fs *virtio_fs_find_instance(const char *tag)
 {
@@ -891,8 +915,15 @@ static int virtio_fs_probe(struct virtio_device *vdev)
 	if (ret < 0)
 		goto out_vqs;
 
+	/* Export tag through sysfs */
+	ret = device_create_file(&vdev->dev, &dev_attr_tag);
+	if (ret < 0)
+		goto out_sysfs_attr;
+
 	return 0;
 
+out_sysfs_attr:
+	virtio_fs_remove_instance(fs);
 out_vqs:
 	virtio_reset_device(vdev);
 	virtio_fs_cleanup_vqs(vdev);
@@ -922,6 +953,9 @@ static void virtio_fs_remove(struct virtio_device *vdev)
 	struct virtio_fs *fs = vdev->priv;
 
 	mutex_lock(&virtio_fs_mutex);
+	/* Remove tag attr from sysfs */
+	device_remove_file(&vdev->dev, &dev_attr_tag);
+
 	/* This device is going away. No one should get new reference */
 	list_del_init(&fs->list);
 	virtio_fs_stop_all_queues(fs);
-- 
2.41.0

Re: [PATCH v2] virtiofs: Export filesystem tags through sysfs

[Stefan Hajnoczi]

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

On Wed, Nov 08, 2023 at 04:33:33PM -0500, Vivek Goyal wrote:
> virtiofs filesystem is mounted using a "tag" which is exported by the
> virtiofs device. virtiofs driver knows about all the available tags but
> these are not exported to user space.
> 
> People have asked these tags to be exported to user space. Most recently
> Lennart Poettering has asked for it as he wants to scan the tags and mount
> virtiofs automatically in certain cases.
> 
> https://gitlab.com/virtio-fs/virtiofsd/-/issues/128
> 
> This patch exports tags through sysfs. One tag is associated with each
> virtiofs device. A new "tag" file appears under virtiofs device dir.
> Actual filesystem tag can be obtained by reading this "tag" file.
> 
> For example, if a virtiofs device exports tag "myfs", a new file "tag"
> will show up here. Tag has a newline char at the end.
> 
> /sys/bus/virtio/devices/virtio<N>/tag
> 
> # cat /sys/bus/virtio/devices/virtio<N>/tag
> myfs
> 
> Note, tag is available at KOBJ_BIND time and not at KOBJ_ADD event time.
> 
> v2:
> - Add a newline char at the end in tag file. (Alyssa Ross)
> - Add a line in commit logs about tag file being available at KOBJ_BIND
>   time and not KOBJ_ADD time.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 5f1be1da92ce..9f76c9697e6f 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -107,6 +107,21 @@ static const struct fs_parameter_spec virtio_fs_parameters[] = {
>  	{}
>  };
>  
> +/* Forward Declarations */
> +static void virtio_fs_stop_all_queues(struct virtio_fs *fs);
> +
> +/* sysfs related */
> +static ssize_t tag_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct virtio_device *vdev = container_of(dev, struct virtio_device,
> +						  dev);
> +	struct virtio_fs *fs = vdev->priv;
> +
> +	return sysfs_emit(buf, "%s\n", fs->tag);
> +}
> +static DEVICE_ATTR_RO(tag);

Is there a race between tag_show() and virtio_fs_remove()?
virtio_fs_mutex is not held. I'm thinking of the case where userspace
opens the sysfs file and invokes read(2) on one CPU while
virtio_fs_remove() runs on another CPU.

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

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

Re: [PATCH v2] virtiofs: Export filesystem tags through sysfs

[Greg KH]

On Wed, Nov 08, 2023 at 04:33:33PM -0500, Vivek Goyal wrote:
> virtiofs filesystem is mounted using a "tag" which is exported by the
> virtiofs device. virtiofs driver knows about all the available tags but
> these are not exported to user space.
> 
> People have asked these tags to be exported to user space. Most recently
> Lennart Poettering has asked for it as he wants to scan the tags and mount
> virtiofs automatically in certain cases.
> 
> https://gitlab.com/virtio-fs/virtiofsd/-/issues/128
> 
> This patch exports tags through sysfs. One tag is associated with each
> virtiofs device. A new "tag" file appears under virtiofs device dir.
> Actual filesystem tag can be obtained by reading this "tag" file.
> 
> For example, if a virtiofs device exports tag "myfs", a new file "tag"
> will show up here. Tag has a newline char at the end.
> 
> /sys/bus/virtio/devices/virtio<N>/tag
> 
> # cat /sys/bus/virtio/devices/virtio<N>/tag
> myfs
> 
> Note, tag is available at KOBJ_BIND time and not at KOBJ_ADD event time.
> 
> v2:
> - Add a newline char at the end in tag file. (Alyssa Ross)
> - Add a line in commit logs about tag file being available at KOBJ_BIND
>   time and not KOBJ_ADD time.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

No documentation for your new sysfs file?  :(

Re: [PATCH v2] virtiofs: Export filesystem tags through sysfs

[Vivek Goyal]

On Sat, Nov 11, 2023 at 06:55:18AM -0500, Greg KH wrote:
> On Wed, Nov 08, 2023 at 04:33:33PM -0500, Vivek Goyal wrote:
> > virtiofs filesystem is mounted using a "tag" which is exported by the
> > virtiofs device. virtiofs driver knows about all the available tags but
> > these are not exported to user space.
> > 
> > People have asked these tags to be exported to user space. Most recently
> > Lennart Poettering has asked for it as he wants to scan the tags and mount
> > virtiofs automatically in certain cases.
> > 
> > https://gitlab.com/virtio-fs/virtiofsd/-/issues/128
> > 
> > This patch exports tags through sysfs. One tag is associated with each
> > virtiofs device. A new "tag" file appears under virtiofs device dir.
> > Actual filesystem tag can be obtained by reading this "tag" file.
> > 
> > For example, if a virtiofs device exports tag "myfs", a new file "tag"
> > will show up here. Tag has a newline char at the end.
> > 
> > /sys/bus/virtio/devices/virtio<N>/tag
> > 
> > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > myfs
> > 
> > Note, tag is available at KOBJ_BIND time and not at KOBJ_ADD event time.
> > 
> > v2:
> > - Add a newline char at the end in tag file. (Alyssa Ross)
> > - Add a line in commit logs about tag file being available at KOBJ_BIND
> >   time and not KOBJ_ADD time.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> 
> No documentation for your new sysfs file?  :(

Hi Greg,

My bad. I forgot about it while posting V2.

As per your comment in another email, I will include some documentation
in Documentation/ABI/. Not very sure what file name to choose. I will
probably start with "sysfs-bus-virtio-devices-virtiofs".

Thanks
Vivek

Re: [PATCH v2] virtiofs: Export filesystem tags through sysfs

[Vivek Goyal]

On Thu, Nov 09, 2023 at 09:28:25AM +0800, Stefan Hajnoczi wrote:
> On Wed, Nov 08, 2023 at 04:33:33PM -0500, Vivek Goyal wrote:
> > virtiofs filesystem is mounted using a "tag" which is exported by the
> > virtiofs device. virtiofs driver knows about all the available tags but
> > these are not exported to user space.
> > 
> > People have asked these tags to be exported to user space. Most recently
> > Lennart Poettering has asked for it as he wants to scan the tags and mount
> > virtiofs automatically in certain cases.
> > 
> > https://gitlab.com/virtio-fs/virtiofsd/-/issues/128
> > 
> > This patch exports tags through sysfs. One tag is associated with each
> > virtiofs device. A new "tag" file appears under virtiofs device dir.
> > Actual filesystem tag can be obtained by reading this "tag" file.
> > 
> > For example, if a virtiofs device exports tag "myfs", a new file "tag"
> > will show up here. Tag has a newline char at the end.
> > 
> > /sys/bus/virtio/devices/virtio<N>/tag
> > 
> > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > myfs
> > 
> > Note, tag is available at KOBJ_BIND time and not at KOBJ_ADD event time.
> > 
> > v2:
> > - Add a newline char at the end in tag file. (Alyssa Ross)
> > - Add a line in commit logs about tag file being available at KOBJ_BIND
> >   time and not KOBJ_ADD time.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 5f1be1da92ce..9f76c9697e6f 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -107,6 +107,21 @@ static const struct fs_parameter_spec virtio_fs_parameters[] = {
> >  	{}
> >  };
> >  
> > +/* Forward Declarations */
> > +static void virtio_fs_stop_all_queues(struct virtio_fs *fs);
> > +
> > +/* sysfs related */
> > +static ssize_t tag_show(struct device *dev, struct device_attribute *attr,
> > +			char *buf)
> > +{
> > +	struct virtio_device *vdev = container_of(dev, struct virtio_device,
> > +						  dev);
> > +	struct virtio_fs *fs = vdev->priv;
> > +
> > +	return sysfs_emit(buf, "%s\n", fs->tag);
> > +}
> > +static DEVICE_ATTR_RO(tag);
> 
> Is there a race between tag_show() and virtio_fs_remove()?
> virtio_fs_mutex is not held. I'm thinking of the case where userspace
> opens the sysfs file and invokes read(2) on one CPU while
> virtio_fs_remove() runs on another CPU.

Hi Stefan,

Good point. I started testing it and realized that something else
is providing mutual exclusion and race does not occur. I added
an artifial msleep(10 seconds) in tag_show() and removed the device
and let tag_show() continue, hoping kernel will crash. But that
did not happen. 

Further investation revealed that device_remove_file() call in 
virtio_fs_remove() blocks till tag_show() has finished.

I have not looked too deep but my guess is that is is probably
kernfs_node->kernfs_rwsem which is providing mutual exclusion
and eliminating this race.

So I don't think we need to take virtio_fs_mutex in tag_show().

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

Re: [PATCH v2] virtiofs: Export filesystem tags through sysfs

[Stefan Hajnoczi]

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

On Mon, Nov 13, 2023 at 06:19:39PM -0500, Vivek Goyal wrote:
> On Thu, Nov 09, 2023 at 09:28:25AM +0800, Stefan Hajnoczi wrote:
> > On Wed, Nov 08, 2023 at 04:33:33PM -0500, Vivek Goyal wrote:
> > > virtiofs filesystem is mounted using a "tag" which is exported by the
> > > virtiofs device. virtiofs driver knows about all the available tags but
> > > these are not exported to user space.
> > > 
> > > People have asked these tags to be exported to user space. Most recently
> > > Lennart Poettering has asked for it as he wants to scan the tags and mount
> > > virtiofs automatically in certain cases.
> > > 
> > > https://gitlab.com/virtio-fs/virtiofsd/-/issues/128
> > > 
> > > This patch exports tags through sysfs. One tag is associated with each
> > > virtiofs device. A new "tag" file appears under virtiofs device dir.
> > > Actual filesystem tag can be obtained by reading this "tag" file.
> > > 
> > > For example, if a virtiofs device exports tag "myfs", a new file "tag"
> > > will show up here. Tag has a newline char at the end.
> > > 
> > > /sys/bus/virtio/devices/virtio<N>/tag
> > > 
> > > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > > myfs
> > > 
> > > Note, tag is available at KOBJ_BIND time and not at KOBJ_ADD event time.
> > > 
> > > v2:
> > > - Add a newline char at the end in tag file. (Alyssa Ross)
> > > - Add a line in commit logs about tag file being available at KOBJ_BIND
> > >   time and not KOBJ_ADD time.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  fs/fuse/virtio_fs.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > > index 5f1be1da92ce..9f76c9697e6f 100644
> > > --- a/fs/fuse/virtio_fs.c
> > > +++ b/fs/fuse/virtio_fs.c
> > > @@ -107,6 +107,21 @@ static const struct fs_parameter_spec virtio_fs_parameters[] = {
> > >  	{}
> > >  };
> > >  
> > > +/* Forward Declarations */
> > > +static void virtio_fs_stop_all_queues(struct virtio_fs *fs);
> > > +
> > > +/* sysfs related */
> > > +static ssize_t tag_show(struct device *dev, struct device_attribute *attr,
> > > +			char *buf)
> > > +{
> > > +	struct virtio_device *vdev = container_of(dev, struct virtio_device,
> > > +						  dev);
> > > +	struct virtio_fs *fs = vdev->priv;
> > > +
> > > +	return sysfs_emit(buf, "%s\n", fs->tag);
> > > +}
> > > +static DEVICE_ATTR_RO(tag);
> > 
> > Is there a race between tag_show() and virtio_fs_remove()?
> > virtio_fs_mutex is not held. I'm thinking of the case where userspace
> > opens the sysfs file and invokes read(2) on one CPU while
> > virtio_fs_remove() runs on another CPU.
> 
> Hi Stefan,
> 
> Good point. I started testing it and realized that something else
> is providing mutual exclusion and race does not occur. I added
> an artifial msleep(10 seconds) in tag_show() and removed the device
> and let tag_show() continue, hoping kernel will crash. But that
> did not happen. 
> 
> Further investation revealed that device_remove_file() call in 
> virtio_fs_remove() blocks till tag_show() has finished.
> 
> I have not looked too deep but my guess is that is is probably
> kernfs_node->kernfs_rwsem which is providing mutual exclusion
> and eliminating this race.
> 
> So I don't think we need to take virtio_fs_mutex in tag_show().

Nice. That explains why other ->show() functions in the kernel also
don't have much in the way of explicit synchronization.

Thank you!

Stefan

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