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

[Stefan Hajnoczi <stefanha@redhat.com>]

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