[PATCH] virtiofs: Export filesystem tags through sysfs

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

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

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

Signed-off-by: Vivek Goyal <vgoyal@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..a5b11e18f331 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", 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] virtiofs: Export filesystem tags through sysfs

[Miklos Szeredi]

On Thu, 5 Oct 2023 at 22:30, Vivek Goyal <vgoyal@redhat.com> 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.
>
> /sys/bus/virtio/devices/virtio<N>/tag
>
> # cat /sys/bus/virtio/devices/virtio<N>/tag
> myfs
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Hi Vivek,

This needs something under Documentation/.

While the interface looks good to me, I think we need an ack on that
from the virtio maintainer.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH] virtiofs: Export filesystem tags through sysfs

[Vivek Goyal]

On Mon, Oct 09, 2023 at 11:53:42AM +0200, Miklos Szeredi wrote:
> On Thu, 5 Oct 2023 at 22:30, Vivek Goyal <vgoyal@redhat.com> 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.
> >
> > /sys/bus/virtio/devices/virtio<N>/tag
> >
> > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > myfs
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Hi Vivek,
> 
> This needs something under Documentation/.

Hi Miklos,

Hmm.., I can easily update the virtiofs documentation.

Initially I was thinking to put something in Documentation/ABI/testing/
as well. But I don't see any virtio related. In fact can't find any
files related to "virtio" at all.

So I will just update the Documentation/filesystems/virtiofs.rst for now.

> 
> While the interface looks good to me, I think we need an ack on that
> from the virtio maintainer.

I am assuming Stefan should be able to provide an ACK. But I will also
add Michael and Paolo in the V2 of the patch and hoping we should be
able to get atleast 1 ACK.

Thanks
Vivek

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

[Stefan Hajnoczi]

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

On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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.
> 
> /sys/bus/virtio/devices/virtio<N>/tag
> 
> # cat /sys/bus/virtio/devices/virtio<N>/tag
> myfs

If you respin this series, please mention that the tag is available at
KOBJ_BIND time, but not KOBJ_ADD. Just a sentence or two is enough to
help someone trying to figure out how to use this new sysfs attr with
udev.

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

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

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

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

[German Maglione]

On Tue, Oct 10, 2023 at 8:38 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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.
> >
> > /sys/bus/virtio/devices/virtio<N>/tag
> >
> > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > myfs
>
> If you respin this series, please mention that the tag is available at
> KOBJ_BIND time, but not KOBJ_ADD. Just a sentence or two is enough to
> help someone trying to figure out how to use this new sysfs attr with
> udev.

Maybe it's also worth mention, that the tag file is created after a successful
probe, so the tag should be a valid one: non-empty and unique

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



-- 
German

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

[Alyssa Ross]

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

On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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

Hi, I was one of those people. :)

> 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.
>
> /sys/bus/virtio/devices/virtio<N>/tag
>
> # cat /sys/bus/virtio/devices/virtio<N>/tag
> myfs
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Are you still thinking about exposing this in the uevent as well?
That would be much more convenient for me, because with this approach
by the time the "remove" uevent arrives, it's no longer possible to
check what tag was associated with the device — you have to store it
somewhere when the device appears, so you can look it up again when the
device is removed.  (Not everybody uses udev.)

Regardless,

Tested-by: Alyssa Ross <hi@alyssa.is>

… and a review comment below.

> ---
>  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..a5b11e18f331 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", fs->tag);

All of the other files in the device directory end with trailing
newlines.  Should this one be an exception?

> +}
> +static DEVICE_ATTR_RO(tag);

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

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

[Vivek Goyal]

On Sat, Oct 21, 2023 at 04:10:21PM +0000, Alyssa Ross wrote:
> On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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
> 
> Hi, I was one of those people. :)

Hi Alyssa,

Aha... Thanks. I was not able to remember who had requested this.

> 
> > 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.
> >
> > /sys/bus/virtio/devices/virtio<N>/tag
> >
> > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > myfs
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Are you still thinking about exposing this in the uevent as well?
> That would be much more convenient for me, because with this approach
> by the time the "remove" uevent arrives, it's no longer possible to
> check what tag was associated with the device — you have to store it
> somewhere when the device appears, so you can look it up again when the
> device is removed.  (Not everybody uses udev.)

Looks like systemd + udev combination can already take care of it. I just
had to specify "StopWhenUnneeded=true" in my systemd .mount unit file. And
that made sure that when device goes away, virtiofs is unmounted and
service is deactivated.

Following is my mount unit file.

$ cat /etc/systemd/system/mnt-virtiofs.mount

[Unit]
Description=Virtiofs mount myfs
DefaultDependencies=no
ConditionPathExists=/mnt/virtiofs
ConditionCapability=CAP_SYS_ADMIN
Before=sysinit.target
StopWhenUnneeded=true 

[Mount]
What=myfs
Where=/mnt/virtiofs
Type=virtiofs

And following is the udev rule I used.

$ cat /etc/udev/rules.d/80-local.rules
SUBSYSTEM=="virtio", DRIVER=="virtiofs", ATTR{tag}=="myfs", TAG+="systemd", ENV{SYSTEMD_WANTS}+="mnt-virtiofs.mount"

And a combination of above two seems to work. virtiofs is automatically
mounted when device is hotplugged and it is unmounted when device is
hot unplugged.

> 
> Regardless,
> 
> Tested-by: Alyssa Ross <hi@alyssa.is>
> 
> … and a review comment below.
> 
> > ---
> >  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..a5b11e18f331 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", fs->tag);
> 
> All of the other files in the device directory end with trailing
> newlines.  Should this one be an exception?

I did not think much about it but you are right that other files seem
to have a newline at the end. I will add one as well.

Thanks
Vivek

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

[Alyssa Ross]

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

Vivek Goyal <vgoyal@redhat.com> writes:

> On Sat, Oct 21, 2023 at 04:10:21PM +0000, Alyssa Ross wrote:
>> Are you still thinking about exposing this in the uevent as well?
>> That would be much more convenient for me, because with this approach
>> by the time the "remove" uevent arrives, it's no longer possible to
>> check what tag was associated with the device — you have to store it
>> somewhere when the device appears, so you can look it up again when the
>> device is removed.  (Not everybody uses udev.)
>
> Looks like systemd + udev combination can already take care of it. I just
> had to specify "StopWhenUnneeded=true" in my systemd .mount unit file. And
> that made sure that when device goes away, virtiofs is unmounted and
> service is deactivated.

My point is that, if it's not exposed in the uevent, the tag information
has to be tracked somehow.  systemd/udev may do that already, but every
other system people might be using (mine uses mdevd) also has to track
that state.  Whereas if the uevent did contain that information,
userspace would be able to do the unmount directly, without needing to
look up some information it has previously saved.

Relying on tracking state from previous events also introduces potential
reliability problems — it's possible to miss uevents if the netlink
queue fills up.  Suppose I have a system where virtiofs filesystems
should always be unmounted when the device goes away.  I miss the uevent
for the device being added, the user mounts the filesystem anyway, and
then when the device removal uevent comes in, unless that uevent
contains the filesystem tag, I'm not going to know which filesystem to
unmount.

> Following is my mount unit file.
>
> $ cat /etc/systemd/system/mnt-virtiofs.mount
>
> [Unit]
> Description=Virtiofs mount myfs
> DefaultDependencies=no
> ConditionPathExists=/mnt/virtiofs
> ConditionCapability=CAP_SYS_ADMIN
> Before=sysinit.target
> StopWhenUnneeded=true 
>
> [Mount]
> What=myfs
> Where=/mnt/virtiofs
> Type=virtiofs
>
> And following is the udev rule I used.
>
> $ cat /etc/udev/rules.d/80-local.rules
> SUBSYSTEM=="virtio", DRIVER=="virtiofs", ATTR{tag}=="myfs", TAG+="systemd", ENV{SYSTEMD_WANTS}+="mnt-virtiofs.mount"
>
> And a combination of above two seems to work. virtiofs is automatically
> mounted when device is hotplugged and it is unmounted when device is
> hot unplugged.

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

Re: [Virtio-fs] [PATCH] virtiofs: Export filesystem tags through sysfs

[Greg KH]

On Mon, Oct 09, 2023 at 04:21:46PM -0400, Vivek Goyal wrote:
> On Mon, Oct 09, 2023 at 11:53:42AM +0200, Miklos Szeredi wrote:
> > On Thu, 5 Oct 2023 at 22:30, Vivek Goyal <vgoyal@redhat.com> 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.
> > >
> > > /sys/bus/virtio/devices/virtio<N>/tag
> > >
> > > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > > myfs
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Hi Vivek,
> > 
> > This needs something under Documentation/.
> 
> Hi Miklos,
> 
> Hmm.., I can easily update the virtiofs documentation.
> 
> Initially I was thinking to put something in Documentation/ABI/testing/
> as well. But I don't see any virtio related. In fact can't find any
> files related to "virtio" at all.
> 
> So I will just update the Documentation/filesystems/virtiofs.rst for now.

No, please put it in Documentation/ABI/ that is where sysfs file
information goes, and where our tools actually look for it (and test for
it!)

thanks,

greg k-h

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

[Greg KH]

On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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.
> 
> /sys/bus/virtio/devices/virtio<N>/tag
> 
> # cat /sys/bus/virtio/devices/virtio<N>/tag
> myfs
> 
> Signed-off-by: Vivek Goyal <vgoyal@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..a5b11e18f331 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", 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;

You just raced with userspace and lost :(

Please use default groups for your device/bus, that is what they are
there for.

thanks,

greg k-h

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

[Stefan Hajnoczi]

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

On Thu, Nov 09, 2023 at 04:57:01PM +0100, Alyssa Ross wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > On Sat, Oct 21, 2023 at 04:10:21PM +0000, Alyssa Ross wrote:
> >> Are you still thinking about exposing this in the uevent as well?
> >> That would be much more convenient for me, because with this approach
> >> by the time the "remove" uevent arrives, it's no longer possible to
> >> check what tag was associated with the device — you have to store it
> >> somewhere when the device appears, so you can look it up again when the
> >> device is removed.  (Not everybody uses udev.)
> >
> > Looks like systemd + udev combination can already take care of it. I just
> > had to specify "StopWhenUnneeded=true" in my systemd .mount unit file. And
> > that made sure that when device goes away, virtiofs is unmounted and
> > service is deactivated.
> 
> My point is that, if it's not exposed in the uevent, the tag information
> has to be tracked somehow.  systemd/udev may do that already, but every
> other system people might be using (mine uses mdevd) also has to track
> that state.  Whereas if the uevent did contain that information,
> userspace would be able to do the unmount directly, without needing to
> look up some information it has previously saved.
> 
> Relying on tracking state from previous events also introduces potential
> reliability problems — it's possible to miss uevents if the netlink
> queue fills up.  Suppose I have a system where virtiofs filesystems
> should always be unmounted when the device goes away.  I miss the uevent
> for the device being added, the user mounts the filesystem anyway, and
> then when the device removal uevent comes in, unless that uevent
> contains the filesystem tag, I'm not going to know which filesystem to
> unmount.

I agree that it's hard for userspace to react when a virtiofs device is
removed. Looking at the Linux device remove code, it appears the sysfs
attr is already gone when the UNBIND event is emitted.

My naive idea as a sysfs newbie is that the driver's ->remove() should
be able to produce an envp[] argument to the later kobject_uevent_env()
call. This would allow the driver to pass additional information to
userspace. Then the virtiofs driver could include the tag string in the
UNBIND event.

I'm a bit surprised that this functionality doesn't already exist. Maybe
there is another way of solving this?

(A different approach that feels wrong is to leave the "tag" attr on the
virtio device and just make sure it gets cleaned up when the virtio bus
driver removes the device kobj. That way unbind udev rules could
continue to use the sysfs attr.)

Stefan

> 
> > Following is my mount unit file.
> >
> > $ cat /etc/systemd/system/mnt-virtiofs.mount
> >
> > [Unit]
> > Description=Virtiofs mount myfs
> > DefaultDependencies=no
> > ConditionPathExists=/mnt/virtiofs
> > ConditionCapability=CAP_SYS_ADMIN
> > Before=sysinit.target
> > StopWhenUnneeded=true 
> >
> > [Mount]
> > What=myfs
> > Where=/mnt/virtiofs
> > Type=virtiofs
> >
> > And following is the udev rule I used.
> >
> > $ cat /etc/udev/rules.d/80-local.rules
> > SUBSYSTEM=="virtio", DRIVER=="virtiofs", ATTR{tag}=="myfs", TAG+="systemd", ENV{SYSTEMD_WANTS}+="mnt-virtiofs.mount"
> >
> > And a combination of above two seems to work. virtiofs is automatically
> > mounted when device is hotplugged and it is unmounted when device is
> > hot unplugged.



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

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

[Vivek Goyal]

On Sat, Nov 11, 2023 at 06:53:40AM -0500, Greg KH wrote:
> On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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.
> > 
> > /sys/bus/virtio/devices/virtio<N>/tag
> > 
> > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > myfs
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@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..a5b11e18f331 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", 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;
> 
> You just raced with userspace and lost :(
> 
> Please use default groups for your device/bus, that is what they are
> there for.

Hi Greg,

Getting back to this thread after a long time. Sorry, got busy in other
things.

Trying to understand the races with user space. IIUC, your primary concern
is that kobject has already been published to the user space and I am
trying to add/remove attributes to the object after the fact and that
makes it racy?

And suggestion is to first fully create kobject (including attrs) before
publishing it to user space. Do I understand it correctly or I missed
the point completely?

Thanks
Vivek

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

[Greg KH]

On Fri, Jan 05, 2024 at 03:44:55PM -0500, Vivek Goyal wrote:
> On Sat, Nov 11, 2023 at 06:53:40AM -0500, Greg KH wrote:
> > On Thu, Oct 05, 2023 at 04:30:30PM -0400, 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.
> > > 
> > > /sys/bus/virtio/devices/virtio<N>/tag
> > > 
> > > # cat /sys/bus/virtio/devices/virtio<N>/tag
> > > myfs
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@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..a5b11e18f331 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", 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;
> > 
> > You just raced with userspace and lost :(
> > 
> > Please use default groups for your device/bus, that is what they are
> > there for.
> 
> Hi Greg,
> 
> Getting back to this thread after a long time. Sorry, got busy in other
> things.
> 
> Trying to understand the races with user space. IIUC, your primary concern
> is that kobject has already been published to the user space and I am
> trying to add/remove attributes to the object after the fact and that
> makes it racy?

Yes.

> And suggestion is to first fully create kobject (including attrs) before
> publishing it to user space. Do I understand it correctly or I missed
> the point completely?

Use the default groups pointer in the device instead which makes the
driver core create/remove the files automatically for you.  Here's
something I wrote over a decade ago as to how to prevent this:
	http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

thanks,

greg k-h