----- Original Message -----
> From: "Pavel Shilovsky" <piastryyy@xxxxxxxxx>
> To: "Ronnie Sahlberg" <lsahlber@xxxxxxxxxx>
> Cc: "linux-cifs" <linux-cifs@xxxxxxxxxxxxxxx>
> Sent: Tuesday, 29 October, 2019 9:18:40 AM
> Subject: Re: [PATCH] cifs: move cifsFileInfo_put logic into a work-queue
>
> сб, 26 окт. 2019 г. в 14:04, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> >
> > 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.
I will update the patch. Thanks.
>
> ...
> >
> > Reported-by: Frank Sorenson <sorenson@xxxxxxxxxx>:
> > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> > ---
> > 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?
Yes, that makes it much better. Thanks!
>
> --
> Best regards,
> Pavel Shilovsky
>
