search for: virtio_fs_free_dev

virtio_fs_free_devs() is now called from ->kill_sb(). By this time all device queues have been quiesced. I am assuming that while ->kill_sb() is in progress, another mount instance will wait for it to finish (sb->s_umount mutex provides mutual exclusion). W.r.t ->remove path, we should be fine as we are...

.....40259368a6bd 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -999,8 +999,7 @@ static int virtio_fs_fill_super(struct super_block *sb) err_free_init_req: fuse_request_free(init_req); err_free_fuse_devs: - for (i = 0; i < fs->nvqs; i++) - fuse_dev_free(fs->vqs[i].fud); + virtio_fs_free_devs(fs); err: return err; } -- 2.20.1

...tiofs queues when device is being removed virtiofs: Drain all pending requests during ->remove time virtiofs: Add an helper to start all the queues virtiofs: Do not use device managed mem for virtio_fs and virtio_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_r...

...<vgoyal at redhat.com> --- fs/fuse/virtio_fs.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 1ea0f889e804..a76bd5a04521 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -180,9 +180,6 @@ static void virtio_fs_free_devs(struct virtio_fs *fs) if (!fsvq->fud) continue; - flush_work(&fsvq->done_work); - flush_delayed_work(&fsvq->dispatch_work); - /* TODO need to quiesce/end_requests/decrement dev_count */ fuse_dev_free(fsvq->fud); fsvq->fud = NULL; @@ -994,6 +991,8 @@ stati...

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

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

...ile changed, 2 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 5df97dfee37d..f68a25ca9e9d 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -497,8 +497,6 @@ static void virtio_fs_remove(struct virtio_device *vdev) { struct virtio_fs *fs = vdev->priv; - virtio_fs_free_devs(fs); - vdev->config->reset(vdev); virtio_fs_cleanup_vqs(vdev, fs); -- 2.20.1

..._wake_interrupt_and_unlock, @@ -988,7 +1007,7 @@ static void virtio_kill_sb(struct super_block *sb) spin_lock(&fsvq->lock); fsvq->connected = false; spin_unlock(&fsvq->lock); - virtio_fs_flush_hiprio_queue(fsvq); + virtio_fs_drain_all_queues(vfs); fuse_kill_sb_anon(sb); virtio_fs_free_devs(vfs); -- 2.20.1

...Signed-off-by: Vivek Goyal <vgoyal at redhat.com> --- fs/fuse/virtio_fs.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index c483482185b6..eadaea6eb8e2 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -208,7 +208,6 @@ static void virtio_fs_free_devs(struct virtio_fs *fs) if (!fsvq->fud) continue; - /* TODO need to quiesce/end_requests/decrement dev_count */ fuse_dev_free(fsvq->fud); fsvq->fud = NULL; } @@ -1022,7 +1021,6 @@ static int virtio_fs_fill_super(struct super_block *sb) if (i == VQ_REQUEST) continue;...

...Previous unmount will stop all queues. Start these again */ virtio_fs_start_all_queues(fs); fuse_send_init(fc, init_req); + mutex_unlock(&virtio_fs_mutex); return 0; err_free_init_req: @@ -1027,6 +1036,7 @@ static int virtio_fs_fill_super(struct super_block *sb) err_free_fuse_devs: virtio_fs_free_devs(fs); err: + mutex_unlock(&virtio_fs_mutex); return err; } @@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_context *fsc) fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL); if (!fc) { + mutex_lock(&virtio_fs_mutex); virtiofs_put(fs); + mutex_unlock(&virtio...

...nt virtio_fs_fill_super(struct s /* Previous unmount will stop all queues. Start these again */ virtio_fs_start_all_queues(fs); fuse_send_init(fc, init_req); + mutex_unlock(&virtio_fs_mutex); return 0; err_free_init_req: @@ -1027,6 +1036,7 @@ err_free_init_req: err_free_fuse_devs: virtio_fs_free_devs(fs); err: + mutex_unlock(&virtio_fs_mutex); return err; } @@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_ fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL); if (!fc) { + mutex_lock(&virtio_fs_mutex); virtio_fs_put(fs); + mutex_unlock(&virtio_fs_mutex);...

...gt; > This is not a frequent event at all and number of virtiofs instances > per guest are expected to be fairly small (say less than 10). So I > really don't think there is any value in converting this into a tree > (instead of a list). > > [..] > > > +static void virtio_fs_free_devs(struct virtio_fs *fs) > > > +{ > > > + unsigned int i; > > > + > > > + /* TODO lock */ > > > > Doesn't inspire confidence, does it? > > Agreed. Getting rid of this in set of fixes I am about to post. > > > > > > + >...

...> + list_for_each_entry(fs, &virtio_fs_instances, list) { > + if (strcmp(fs->tag, tag) == 0) > + goto found; > + } > + > + fs = NULL; /* not found */ > + > +found: > + mutex_unlock(&virtio_fs_mutex); > + > + return fs; > +} > + > +static void virtio_fs_free_devs(struct virtio_fs *fs) > +{ > + unsigned int i; > + > + /* TODO lock */ Doesn't inspire confidence, does it? > + > + for (i = 0; i < fs->nvqs; i++) { > + struct virtio_fs_vq *fsvq = &fs->vqs[i]; > + > + if (!fsvq->fud) > + continue; > + &gt...

...> + list_for_each_entry(fs, &virtio_fs_instances, list) { > + if (strcmp(fs->tag, tag) == 0) > + goto found; > + } > + > + fs = NULL; /* not found */ > + > +found: > + mutex_unlock(&virtio_fs_mutex); > + > + return fs; > +} > + > +static void virtio_fs_free_devs(struct virtio_fs *fs) > +{ > + unsigned int i; > + > + /* TODO lock */ Doesn't inspire confidence, does it? > + > + for (i = 0; i < fs->nvqs; i++) { > + struct virtio_fs_vq *fsvq = &fs->vqs[i]; > + > + if (!fsvq->fud) > + continue; > + &gt...

...ces, list) { > + if (strcmp(fs->tag, tag) == 0) { > + kref_get(&fs->refcount); > + goto found; > + } > + } > + > + fs = NULL; /* not found */ > + > +found: > + mutex_unlock(&virtio_fs_mutex); > + > + return fs; > +} > + > +static void virtio_fs_free_devs(struct virtio_fs *fs) > +{ > + unsigned int i; > + > + for (i = 0; i < fs->nvqs; i++) { > + struct virtio_fs_vq *fsvq = &fs->vqs[i]; > + > + if (!fsvq->fud) > + continue; > + > + fuse_dev_free(fsvq->fud); > + fsvq->fud = NULL; > + } &...

...is O(N), search during mount is O(N). This is not a frequent event at all and number of virtiofs instances per guest are expected to be fairly small (say less than 10). So I really don't think there is any value in converting this into a tree (instead of a list). [..] > > +static void virtio_fs_free_devs(struct virtio_fs *fs) > > +{ > > + unsigned int i; > > + > > + /* TODO lock */ > > Doesn't inspire confidence, does it? Agreed. Getting rid of this in set of fixes I am about to post. > > > + > > + for (i = 0; i < fs->nvqs; i++) { > &gt...