| Message ID | 1250511368-9812-3-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@redhat.com> 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 <jlayton@redhat.com> > --- >  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 > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client >
On Mon, 17 Aug 2009 10:09:56 -0500 Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@redhat.com> 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 <jlayton@redhat.com> > > --- > >  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? > Right. In a later patch, I have the thread hold the mutex while the call goes out on the wire as well (in order to prevent the tcon from getting ripped out from under it). > > -            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? > No. The "next" pointer is used internally by the list_for_each_entry_safe macro. It's not safe to use list_for_each_entry if you're removing the entry from the list and freeing it within the loop. The _safe versions of those routines use dereference the "next" pointer first, so that it doesn't have to touch the original entry on the next pass. > > +        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 > > linux-cifs-client@lists.samba.org > > https://lists.samba.org/mailman/listinfo/linux-cifs-client > >
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); - 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) { + if (entry->tcon && entry->tcon == tcon) { + list_del(&entry->qhead); + kmem_cache_free(cifs_oplock_cachep, entry); } } mutex_unlock(&cifs_oplock_mutex);
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 <jlayton@redhat.com> --- fs/cifs/cifsfs.c | 37 +++++++++++++++++++++++-------------- fs/cifs/cifsproto.h | 1 - fs/cifs/transport.c | 23 +++++------------------ 3 files changed, 28 insertions(+), 33 deletions(-)