search for: virtio_fs_mutex

On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote: > It is possible that a mount is in progress and device is being removed at > the same time. Use virtio_fs_mutex to avoid races. > > This also takes care of bunch of races and removes some TODO items. > > Signed-off-by: Vivek Goyal <vgoyal at redhat.com> > --- > fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) Let'...

On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote: > It is possible that a mount is in progress and device is being removed at > the same time. Use virtio_fs_mutex to avoid races. > > This also takes care of bunch of races and removes some TODO items. > > Signed-off-by: Vivek Goyal <vgoyal at redhat.com> > --- > fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) Let'...

On Fri, Sep 06, 2019 at 01:05:34PM +0100, Stefan Hajnoczi wrote: > On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote: > > It is possible that a mount is in progress and device is being removed at > > the same time. Use virtio_fs_mutex to avoid races. > > > > This also takes care of bunch of races and removes some TODO items. > > > > Signed-off-by: Vivek Goyal <vgoyal at redhat.com> > > --- > > fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++---------- > > 1 file changed, 22 inse...

It is possible that a mount is in progress and device is being removed at the same time. Use virtio_fs_mutex to avoid races. This also takes care of bunch of races and removes some TODO items. Signed-off-by: Vivek Goyal <vgoyal at redhat.com> --- fs/fuse/virtio_fs.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse...

...> +} > + > +static void virtiofs_put(struct virtio_fs *fs) Why do the two function names above contain "virtiofs" instead of "virtio_fs"? I'm not sure if this is intentional and is supposed to mean something, but it's confusing. > +{ > + mutex_lock(&virtio_fs_mutex); > + kref_put(&fs->refcount, release_virtiofs_obj); > + mutex_unlock(&virtio_fs_mutex); > +} > + > +static void virtio_fs_put(struct fuse_iqueue *fiq) Minor issue: this function name is confusingly similar to virtiofs_put(). Please rename to virtio_fs_fiq_put(). -------...

...> +} > + > +static void virtiofs_put(struct virtio_fs *fs) Why do the two function names above contain "virtiofs" instead of "virtio_fs"? I'm not sure if this is intentional and is supposed to mean something, but it's confusing. > +{ > + mutex_lock(&virtio_fs_mutex); > + kref_put(&fs->refcount, release_virtiofs_obj); > + mutex_unlock(&virtio_fs_mutex); > +} > + > +static void virtio_fs_put(struct fuse_iqueue *fiq) Minor issue: this function name is confusingly similar to virtiofs_put(). Please rename to virtio_fs_fiq_put(). -------...

...rtiofs_put(struct virtio_fs *fs) > > Why do the two function names above contain "virtiofs" instead > of "virtio_fs"? I'm not sure if this is intentional and is supposed to > mean something, but it's confusing. > > > +{ > > + mutex_lock(&virtio_fs_mutex); > > + kref_put(&fs->refcount, release_virtiofs_obj); > > + mutex_unlock(&virtio_fs_mutex); > > +} > > + > > +static void virtio_fs_put(struct fuse_iqueue *fiq) > > Minor issue: this function name is confusingly similar to > virtiofs_put(). Pleas...

...ueue *vq) return &vq_to_fsvq(vq)->fud->pq; } +static void release_virtiofs_obj(struct kref *ref) +{ + struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount); + + kfree(vfs->vqs); + kfree(vfs); +} + +static void virtiofs_put(struct virtio_fs *fs) +{ + mutex_lock(&virtio_fs_mutex); + kref_put(&fs->refcount, release_virtiofs_obj); + mutex_unlock(&virtio_fs_mutex); +} + +static void virtio_fs_put(struct fuse_iqueue *fiq) +{ + struct virtio_fs *vfs = fiq->priv; + virtiofs_put(vfs); +} + static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq) { WARN_ON(fs...

...io_fs_vq virtiofs: stop and drain queues after sending DESTROY virtiofs: Use virtio_fs_free_devs() in error path virtiofs: Do not access virtqueue in request submission path virtiofs: Add a fuse_iqueue operation to put() reference virtiofs: Make virtio_fs object refcounted virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path virtiofs: Remove TODO to quiesce/end_requests virtiofs: Remove TODO item from virtio_fs_free_devs() fs/fuse/fuse_i.h | 5 + fs/fuse/inode.c | 6 + fs/fuse/virtio_fs.c | 259 ++++++++++++++++++++++++++++++++------------ 3 files changed, 198...

...+#include <linux/fs_context.h> > +#include <linux/highmem.h> > +#include "fuse_i.h" > + > +/* List of virtio-fs device instances and a lock for the list. Also provides > + * mutual exclusion in device removal and mounting path > + */ > +static DEFINE_MUTEX(virtio_fs_mutex); > +static LIST_HEAD(virtio_fs_instances); > + > +enum { > + VQ_HIPRIO, > + VQ_REQUEST > +}; > + > +/* Per-virtqueue state */ > +struct virtio_fs_vq { > + spinlock_t lock; > + struct virtqueue *vq; /* protected by ->lock */ > + struct work_struct done_wor...

...WARN_ON(fsvq->in_flight < 0); /* Wait for in flight requests to finish.*/ - while (1) { - spin_lock(&fsvq->lock); - if (!fsvq->in_flight) { - spin_unlock(&fsvq->lock); - break; - } + spin_lock(&fsvq->lock); + if (fsvq->in_flight) { + /* We are holding virtio_fs_mutex. There should not be any + * waiters waiting for completion. + */ + reinit_completion(&fsvq->in_flight_zero); + spin_unlock(&fsvq->lock); + wait_for_completion(&fsvq->in_flight_zero); + } else { spin_unlock(&fsvq->lock); - /* TODO use completion instead of tim...

...kref *ref) > > +{ > > + struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount); > > + > > + kfree(vfs->vqs); > > + kfree(vfs); > > +} > > + > > +static void virtio_fs_put(struct virtio_fs *fs) > > +{ > > + mutex_lock(&virtio_fs_mutex); > > + kref_put(&fs->refcount, release_virtiofs_obj); > > + mutex_unlock(&virtio_fs_mutex); > > +} > > + > > +static void virtio_fs_put(struct fuse_iqueue *fiq) > > +{ > > + struct virtio_fs *vfs = fiq->priv; > > + virtiofs_put(vfs); &g...

Hi Miklos, Here are few small cleanups for virtiofs for 5.5. I had received some comments from Michael Tsirkin on original virtiofs patches and these cleanups are result of these comments. Thanks Vivek Vivek Goyal (3): virtiofs: Use a common function to send forget virtiofs: Do not send forget request "struct list_head" element virtiofs: Use completions while waiting for queue

...> +#include <linux/virtio_fs.h> > +#include <linux/delay.h> > +#include <linux/fs_context.h> > +#include <linux/highmem.h> > +#include "fuse_i.h" > + > +/* List of virtio-fs device instances and a lock for the list */ > +static DEFINE_MUTEX(virtio_fs_mutex); > +static LIST_HEAD(virtio_fs_instances); > + > +enum { > + VQ_HIPRIO, > + VQ_REQUEST > +}; > + > +/* Per-virtqueue state */ > +struct virtio_fs_vq { > + spinlock_t lock; > + struct virtqueue *vq; /* protected by ->lock */ > + struct work_struct done_wor...

...> +#include <linux/virtio_fs.h> > +#include <linux/delay.h> > +#include <linux/fs_context.h> > +#include <linux/highmem.h> > +#include "fuse_i.h" > + > +/* List of virtio-fs device instances and a lock for the list */ > +static DEFINE_MUTEX(virtio_fs_mutex); > +static LIST_HEAD(virtio_fs_instances); > + > +enum { > + VQ_HIPRIO, > + VQ_REQUEST > +}; > + > +/* Per-virtqueue state */ > +struct virtio_fs_vq { > + spinlock_t lock; > + struct virtqueue *vq; /* protected by ->lock */ > + struct work_struct done_wor...

...FP_KERNEL); if (!fs) return -ENOMEM; vdev->priv = fs; @@ -552,6 +552,7 @@ static int virtio_fs_probe(struct virtio_device *vdev) out: vdev->priv = NULL; + kfree(fs); return ret; } @@ -582,6 +583,8 @@ static void virtio_fs_remove(struct virtio_device *vdev) mutex_unlock(&virtio_fs_mutex); vdev->priv = NULL; + kfree(fs->vqs); + kfree(fs); } #ifdef CONFIG_PM_SLEEP -- 2.20.1

...> +/* Add a new instance to the list or return -EEXIST if tag name exists*/ > > > +static int virtio_fs_add_instance(struct virtio_fs *fs) > > > +{ > > > + struct virtio_fs *fs2; > > > + bool duplicate = false; > > > + > > > + mutex_lock(&virtio_fs_mutex); > > > + > > > + list_for_each_entry(fs2, &virtio_fs_instances, list) { > > > + if (strcmp(fs->tag, fs2->tag) == 0) > > > + duplicate = true; > > > + } > > > + > > > + if (!duplicate) > > > + list_add_tail(&f...

...ot worry about it. [..] > > +/* Add a new instance to the list or return -EEXIST if tag name exists*/ > > +static int virtio_fs_add_instance(struct virtio_fs *fs) > > +{ > > + struct virtio_fs *fs2; > > + bool duplicate = false; > > + > > + mutex_lock(&virtio_fs_mutex); > > + > > + list_for_each_entry(fs2, &virtio_fs_instances, list) { > > + if (strcmp(fs->tag, fs2->tag) == 0) > > + duplicate = true; > > + } > > + > > + if (!duplicate) > > + list_add_tail(&fs->list, &virtio_fs_instances); &...