Re: [PATCH 3/5] cifs: clean up cifs_oplock_thread loop

On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<[email protected]> wrote:
> Minor cleanups and optimizations mostly. Also fix it so that if a
> kthread_stop races in at the right time, it won't make the thread
> sleep unnecessarily for 39s.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
>  fs/cifs/cifsfs.c |   66 +++++++++++++++++++++++++++++------------------------
>  1 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 7ce8dcd..2fcc722 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -972,10 +972,10 @@ cifs_destroy_mids(void)
>
>  static int cifs_oplock_thread(void *dummyarg)
>  {
> -       struct oplock_q_entry *oplock_item;
> -       struct cifsTconInfo *pTcon;
> +       struct oplock_q_entry *oplock;
> +       struct cifsTconInfo *tcon;
>        struct inode *inode;
> -       __u16  netfid;
> +       struct cifsInodeInfo *cifsi;
>        int rc, waitrc = 0;
>
>        set_freezable();
> @@ -984,34 +984,30 @@ static int cifs_oplock_thread(void *dummyarg)
>                        continue;
>
>                spin_lock(&cifs_oplock_lock);
> -               if (list_empty(&cifs_oplock_list)) {
> +               while(!list_empty(&cifs_oplock_list)) {
> +                       oplock = list_entry(cifs_oplock_list.next,
> +                                           struct oplock_q_entry, qhead);
> +                       list_del(&oplock->qhead);
>                        spin_unlock(&cifs_oplock_lock);
> -                       set_current_state(TASK_INTERRUPTIBLE);
> -                       schedule_timeout(39*HZ);
> -               } else {
> -                       oplock_item = list_entry(cifs_oplock_list.next,
> -                                               struct oplock_q_entry, qhead);
> +
>                        cFYI(1, ("found oplock item to write out"));
> -                       pTcon = oplock_item->tcon;
> -                       inode = oplock_item->pinode;
> -                       netfid = oplock_item->netfid;
> -                       list_del(&oplock_item->qhead);
> -                       spin_unlock(&cifs_oplock_lock);
> -                       kmem_cache_free(cifs_oplock_cachep, oplock_item);
> +                       tcon = oplock->tcon;
> +                       inode = oplock->pinode;
>                        /* can not grab inode sem here since it would
>                                deadlock when oplock received on delete
>                                since vfs_unlink holds the i_mutex across
>                                the call */
>                        /* mutex_lock(&inode->i_mutex);*/
> +                       cifsi = CIFS_I(inode);
>                        if (S_ISREG(inode->i_mode)) {
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
> -                               if (CIFS_I(inode)->clientCanCacheAll == 0)
> +                               if (cifsi->clientCanCacheAll == 0)
>                                        break_lease(inode, FMODE_READ);
> -                               else if (CIFS_I(inode)->clientCanCacheRead == 0)
> +                               else if (cifsi->clientCanCacheRead == 0)
>                                        break_lease(inode, FMODE_WRITE);
>  #endif
>                                rc = filemap_fdatawrite(inode->i_mapping);
> -                               if (CIFS_I(inode)->clientCanCacheRead == 0) {
> +                               if (cifsi->clientCanCacheRead == 0) {
>                                        waitrc = filemap_fdatawait(
>                                                              inode->i_mapping);
>                                        invalidate_remote_inode(inode);
> @@ -1022,27 +1018,37 @@ static int cifs_oplock_thread(void *dummyarg)
>                                rc = 0;
>                        /* mutex_unlock(&inode->i_mutex);*/
>                        if (rc)
> -                               CIFS_I(inode)->write_behind_rc = rc;
> +                               cifsi->write_behind_rc = rc;
>                        cFYI(1, ("Oplock flush inode %p rc %d",
>                                inode, rc));
>
> -                               /* releasing stale oplock after recent reconnect
> -                               of smb session using a now incorrect file
> -                               handle is not a data integrity issue but do
> -                               not bother sending an oplock release if session
> -                               to server still is disconnected since oplock
> -                               already released by the server in that case */
> -                       if (!pTcon->need_reconnect) {
> -                               rc = CIFSSMBLock(0, pTcon, netfid,
> +                       /*
> +                        * releasing stale oplock after recent reconnect
> +                        * of smb session using a now incorrect file
> +                        * handle is not a data integrity issue but do
> +                        * not bother sending an oplock release if session
> +                        * to server still is disconnected since oplock
> +                        * already released by the server in that case
> +                        */
> +                       if (!tcon->need_reconnect) {
> +                               rc = CIFSSMBLock(0, tcon, oplock->netfid,
>                                                0 /* len */ , 0 /* offset */, 0,
>                                                0, LOCKING_ANDX_OPLOCK_RELEASE,
>                                                false /* wait flag */);
>                                cFYI(1, ("Oplock release rc = %d", rc));
>                        }
> -                       cifs_put_tcon(pTcon);
> -                       set_current_state(TASK_INTERRUPTIBLE);
> -                       schedule_timeout(1);  /* yield in case q were corrupt */
> +                       cifs_put_tcon(tcon);
> +                       kmem_cache_free(cifs_oplock_cachep, oplock);
> +                       spin_lock(&cifs_oplock_lock);
> +               }
> +               spin_unlock(&cifs_oplock_lock);
> +               set_current_state(TASK_INTERRUPTIBLE);
> +               if (kthread_should_stop()) {
> +                       set_current_state(TASK_RUNNING);
> +                       break;
>                }
> +               /* FIXME: Why 39s here? This should be a named constant. */
> +               schedule_timeout(39*HZ);
>        } while (!kthread_should_stop());
>
>        return 0;
> --
> 1.6.0.6
>
> _______________________________________________
> linux-cifs-client mailing list
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Acked-by: Shirish Pargaonkar <[email protected]>