Re: [PATCH] cifs: move cifsFileInfo_put logic into a work-queue

[Pavel Shilovsky <piastryyy@gmail.com>]

сб, 26 окт. 2019 г. в 14:04, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> This patch moves the _put logic to be processed in a separate thread that
> holds no other locks to prevent deadlocks like below from happening
>

Ronnie,

Thanks for creating the patch! Please find my comments below.

...
>
> Reported-by: Frank Sorenson <sorenson@redhat.com>:
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/cifsfs.c   | 13 ++++++++-
>  fs/cifs/cifsglob.h |  1 +
>  fs/cifs/file.c     | 79 +++++++++++++++++++++++++++++++++++++-----------------
>  3 files changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index e4e3b573d20c..f8e201c45ccb 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -119,6 +119,7 @@ extern mempool_t *cifs_mid_poolp;
>
>  struct workqueue_struct        *cifsiod_wq;
>  struct workqueue_struct        *decrypt_wq;
> +struct workqueue_struct        *fileinfo_put_wq;
>  struct workqueue_struct        *cifsoplockd_wq;
>  __u32 cifs_lock_secret;
>
> @@ -1557,11 +1558,18 @@ init_cifs(void)
>                 goto out_destroy_cifsiod_wq;
>         }
>
> +       fileinfo_put_wq = alloc_workqueue("cifsfileinfoput",
> +                                    WQ_UNBOUND|WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
> +       if (!fileinfo_put_wq) {
> +               rc = -ENOMEM;
> +               goto out_destroy_decrypt_wq;
> +       }
> +
>         cifsoplockd_wq = alloc_workqueue("cifsoplockd",
>                                          WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
>         if (!cifsoplockd_wq) {
>                 rc = -ENOMEM;
> -               goto out_destroy_decrypt_wq;
> +               goto out_destroy_fileinfo_put_wq;
>         }
>
>         rc = cifs_fscache_register();
> @@ -1627,6 +1635,8 @@ init_cifs(void)
>         cifs_fscache_unregister();
>  out_destroy_cifsoplockd_wq:
>         destroy_workqueue(cifsoplockd_wq);
> +out_destroy_fileinfo_put_wq:
> +       destroy_workqueue(fileinfo_put_wq);
>  out_destroy_decrypt_wq:
>         destroy_workqueue(decrypt_wq);
>  out_destroy_cifsiod_wq:
> @@ -1656,6 +1666,7 @@ exit_cifs(void)
>         cifs_fscache_unregister();
>         destroy_workqueue(cifsoplockd_wq);
>         destroy_workqueue(decrypt_wq);
> +       destroy_workqueue(fileinfo_put_wq);
>         destroy_workqueue(cifsiod_wq);
>         cifs_proc_clean();
>  }
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 50dfd9049370..8361e30adcd9 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1902,6 +1902,7 @@ void cifs_queue_oplock_break(struct cifsFileInfo *cfile);
>  extern const struct slow_work_ops cifs_oplock_break_ops;
>  extern struct workqueue_struct *cifsiod_wq;
>  extern struct workqueue_struct *decrypt_wq;
> +extern struct workqueue_struct *fileinfo_put_wq;
>  extern struct workqueue_struct *cifsoplockd_wq;
>  extern __u32 cifs_lock_secret;
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 0e0217641de1..e222cf04b325 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -368,30 +368,8 @@ cifsFileInfo_get(struct cifsFileInfo *cifs_file)
>         return cifs_file;
>  }
>
> -/**
> - * cifsFileInfo_put - release a reference of file priv data
> - *
> - * Always potentially wait for oplock handler. See _cifsFileInfo_put().
> - */
> -void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> -{
> -       _cifsFileInfo_put(cifs_file, true);
> -}
> -
> -/**
> - * _cifsFileInfo_put - release a reference of file priv data
> - *
> - * This may involve closing the filehandle @cifs_file out on the
> - * server. Must be called without holding tcon->open_file_lock and
> - * cifs_file->file_info_lock.
> - *
> - * If @wait_for_oplock_handler is true and we are releasing the last
> - * reference, wait for any running oplock break handler of the file
> - * and cancel any pending one. If calling this function from the
> - * oplock break handler, you need to pass false.
> - *
> - */
> -void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
> +static void
> +_cifsFileInfo_put_work(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
>  {
>         struct inode *inode = d_inode(cifs_file->dentry);
>         struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink);
> @@ -480,6 +458,59 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
>         kfree(cifs_file);
>  }
>
> +struct cifsFileInfo_w {
> +       struct work_struct put;
> +       struct cifsFileInfo *cifs_file;
> +       bool wait_oplock_handler;
> +};
> +
> +static void cifsFileInfo_put_work(struct work_struct *work)
> +{
> +       struct cifsFileInfo_w *cfiw = container_of(work,
> +                                                  struct cifsFileInfo_w, put);
> +       _cifsFileInfo_put_work(cfiw->cifs_file, cfiw->wait_oplock_handler);
> +       kfree(cfiw);
> +}
> +
> +/**
> + * cifsFileInfo_put - release a reference of file priv data
> + *
> + * Always potentially wait for oplock handler. See _cifsFileInfo_put().
> + */
> +void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> +{
> +       _cifsFileInfo_put(cifs_file, true);
> +}
> +
> +/**
> + * _cifsFileInfo_put - release a reference of file priv data
> + *
> + * This may involve closing the filehandle @cifs_file out on the
> + * server. Must be called without holding tcon->open_file_lock and
> + * cifs_file->file_info_lock.

cinode->open_file_lock should be mentioned here as well.

> + *
> + * If @wait_for_oplock_handler is true and we are releasing the last
> + * reference, wait for any running oplock break handler of the file
> + * and cancel any pending one. If calling this function from the
> + * oplock break handler, you need to pass false.
> + *
> + */
> +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
> +{
> +       struct cifsFileInfo_w *work;
> +
> +       work = kmalloc(sizeof(struct cifsFileInfo_w), GFP_KERNEL);

I would suggest to make this a part of cifsFileInfo structure (like
oplock handling work) to avoid unnecessary memory allocations every
time we put/close the handle.

> +       if (work == NULL) {
> +               _cifsFileInfo_put_work(cifs_file, wait_oplock_handler);
> +               return;
> +       }
> +
> +       INIT_WORK(&work->put, cifsFileInfo_put_work);
> +       work->cifs_file = cifs_file;
> +       work->wait_oplock_handler = wait_oplock_handler;
> +       queue_work(fileinfo_put_wq, &work->put);

I don't think we should offload the work to another thread
unconditionally here - e.g. cifs_close() can safely do it in the same
thread. Since there are more places where we want to offload, so let's
do the similar thing as we have to the "wait_oplock_handler" argument:

cifsFileInfo_put(cifs_file) -> _cifsFileInfo_put(cifs_file, true /*
wait_oplock_handler */, true /* offload */);

and convert cifs_close() to call _cifsFileInfo_put(cifs_file, true /*
wait_oplock_handler */, false /* offload */);

Note, that we do not need to conditionally cancel the worker thread as
we do for the oplock handler because once we reach zero references
there should be only one thread (we) that has access to the file info
structure, thus no need to cancel anything. We may add a Warning if
the work is scheduled but we reached zero references to highlight a
potential bug.

Also, as an optimization, we may use the caller thread to check the
number of references and only offload if both conditions meet:
"offload" argument is true and the current number of references is 1
(putting the last reference). This optimization may be done in the
same patch or as a separate patch.

Thoughts?

--
Best regards,
Pavel Shilovsky