[linux-cifs-client] [PATCH 2/4] cifs: iterate over cifs_oplock_list using list_for_each_entry_safe
Jeff Layton
jlayton at redhat.com
Mon Aug 17 09:46:19 MDT 2009
On Mon, 17 Aug 2009 10:09:56 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
> On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton at 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 at 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 at lists.samba.org
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >
--
Jeff Layton <jlayton at redhat.com>
More information about the linux-cifs-client
mailing list