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

[Vivek Goyal <vgoyal@redhat.com>]

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>