Re: [PATCH 2/4] cifs: iterate over cifs_oplock_list using list_for_each_entry_safe

On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<[email protected]> wrote:
> Removing an entry from a list while you're iterating over it can be
> problematic and racy, so use the _safe version of the list iteration
> routine in DeleteTconOplockQEntries.
>
> Also restructure the oplock_thread loop to make sure that if a
> kthread_stop races in just as the thread goes to sleep, then it won't
> just sit there for 39s.
>
> Finally, remove DeleteOplockQEntry(). It's only called from one place
> and we can avoid a function call this way.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
>  fs/cifs/cifsfs.c    |   37 +++++++++++++++++++++++--------------
>  fs/cifs/cifsproto.h |    1 -
>  fs/cifs/transport.c |   23 +++++------------------
>  3 files changed, 28 insertions(+), 33 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 0d4e0da..ab4b373 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -976,7 +976,7 @@ cifs_destroy_mids(void)
>  static int cifs_oplock_thread(void *dummyarg)
>  {
>        struct oplock_q_entry *oplock_item;
> -       struct cifsTconInfo *pTcon;
> +       struct cifsTconInfo *tcon;
>        struct inode *inode;
>        __u16  netfid;
>        int rc, waitrc = 0;
> @@ -986,20 +986,24 @@ static int cifs_oplock_thread(void *dummyarg)
>                if (try_to_freeze())
>                        continue;
>
> +               /*
> +                * can't reasonably use list_for_each macros here. It's
> +                * possible that another thread could come along and remove
> +                * some of the entries while the lock is released. It's fine
> +                * though since we're just popping one off the head on each
> +                * iteration anyway.
> +                */
>                mutex_lock(&cifs_oplock_mutex);
> -               if (list_empty(&cifs_oplock_list)) {
> -                       mutex_unlock(&cifs_oplock_mutex);
> -                       set_current_state(TASK_INTERRUPTIBLE);
> -                       schedule_timeout(39*HZ);
> -               } else {
> -                       oplock_item = list_entry(cifs_oplock_list.next,
> -                                               struct oplock_q_entry, qhead);
> +               while(!list_empty(&cifs_oplock_list)) {
>                        cFYI(1, ("found oplock item to write out"));
> -                       pTcon = oplock_item->tcon;
> +                       oplock_item = list_entry(cifs_oplock_list.next,
> +                                                struct oplock_q_entry, qhead);
> +                       tcon = oplock_item->tcon;
>                        inode = oplock_item->pinode;
>                        netfid = oplock_item->netfid;
> +                       list_del(&oplock_item->qhead);
> +                       kmem_cache_free(cifs_oplock_cachep, oplock_item);
>                        mutex_unlock(&cifs_oplock_mutex);

Is not very clear with changes and indentations, but we do take mutex
lock within the new while loop right?

> -                       DeleteOplockQEntry(oplock_item);
>                        /* can not grab inode sem here since it would
>                                deadlock when oplock received on delete
>                                since vfs_unlink holds the i_mutex across
> @@ -1034,16 +1038,21 @@ static int cifs_oplock_thread(void *dummyarg)
>                                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,
> +                       if (!tcon->need_reconnect) {
> +                               rc = CIFSSMBLock(0, tcon, netfid,
>                                                0 /* len */ , 0 /* offset */, 0,
>                                                0, LOCKING_ANDX_OPLOCK_RELEASE,
>                                                false /* wait flag */);
>                                cFYI(1, ("Oplock release rc = %d", rc));
>                        }
> -                       set_current_state(TASK_INTERRUPTIBLE);
> -                       schedule_timeout(1);  /* yield in case q were corrupt */
> +                       mutex_lock(&cifs_oplock_mutex);
>                }
> +               mutex_unlock(&cifs_oplock_mutex);
> +               set_current_state(TASK_INTERRUPTIBLE);
> +               if (kthread_should_stop())
> +                       break;
> +               /* FIXME: why 39s here? Turn this into a #define constant? */
> +               schedule_timeout(39*HZ);
>        } while (!kthread_should_stop());
>
>        return 0;
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index da8fbf5..b7554a7 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -88,7 +88,6 @@ extern int CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
>  extern __u16 GetNextMid(struct TCP_Server_Info *server);
>  extern struct oplock_q_entry *AllocOplockQEntry(struct inode *, u16,
>                                                 struct cifsTconInfo *);
> -extern void DeleteOplockQEntry(struct oplock_q_entry *);
>  extern void DeleteTconOplockQEntries(struct cifsTconInfo *);
>  extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
>  extern u64 cifs_UnixTimeToNT(struct timespec);
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 92e1538..59f0e95 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -126,28 +126,15 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
>        return temp;
>  }
>
> -void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry)
> -{
> -       mutex_lock(&cifs_oplock_mutex);
> -    /* should we check if list empty first? */
> -       list_del(&oplockEntry->qhead);
> -       mutex_unlock(&cifs_oplock_mutex);
> -       kmem_cache_free(cifs_oplock_cachep, oplockEntry);
> -}
> -
> -
>  void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
>  {
> -       struct oplock_q_entry *temp;
> -
> -       if (tcon == NULL)
> -               return;
> +       struct oplock_q_entry *entry, *next;
>
>        mutex_lock(&cifs_oplock_mutex);
> -       list_for_each_entry(temp, &cifs_oplock_list, qhead) {
> -               if ((temp->tcon) && (temp->tcon == tcon)) {
> -                       list_del(&temp->qhead);
> -                       kmem_cache_free(cifs_oplock_cachep, temp);
> +       list_for_each_entry_safe(entry, next, &cifs_oplock_list, qhead) {

Just a nitpick, since we do not use next anywhere, may be use NULL instead?

> +               if (entry->tcon && entry->tcon == tcon) {
> +                       list_del(&entry->qhead);
> +                       kmem_cache_free(cifs_oplock_cachep, entry);
>                }
>        }
>        mutex_unlock(&cifs_oplock_mutex);
> --
> 1.6.0.6
>
> _______________________________________________
> linux-cifs-client mailing list
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>