| Message ID | 20231108213333.132599-1-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] virtiofs: Export filesystem tags through sysfs | expand |
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>
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? :(
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
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>
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
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);