[linux-cifs-client] [PATCH 4/4] cifs: hold cifs_oplock_mutex while doing oplock release call
Jeff Layton
jlayton at redhat.com
Mon Aug 17 10:24:48 MDT 2009
On Mon, 17 Aug 2009 11:49:03 -0400
Jeff Layton <jlayton at redhat.com> wrote:
> On Mon, 17 Aug 2009 10:18:51 -0500
> Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
>
> > On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton at redhat.com> wrote:
> > > We have to ensure that the tcon isn't freed until after this call
> > > completes. After dequeuing an oplock entry, have cifsoplockd hold
> > > the cifs_oplock_mutex until the oplock release call completes.
> > >
> > > We don't want to hold the mutex indefinitely however, so release
> > > and reacquire it on each pass through the loop. That should give
> > > other tasks access to the queue.
> > >
> > > Signed-off-by: Jeff Layton <jlayton at redhat.com>
> > > ---
> > > fs/cifs/cifsfs.c | 21 ++++++++++++++-------
> > > 1 files changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index 4c724d5..745c3ba 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -1004,7 +1004,6 @@ static int cifs_oplock_thread(void *dummyarg)
> > > netfid = oplock_item->netfid;
> > > list_del(&oplock_item->qhead);
> > > kmem_cache_free(cifs_oplock_cachep, oplock_item);
> > > - mutex_unlock(&cifs_oplock_mutex);
> > >
> > > if (inode && S_ISREG(inode->i_mode)) {
> > > cifsi = CIFS_I(inode);
> > > @@ -1029,12 +1028,14 @@ static int cifs_oplock_thread(void *dummyarg)
> > > }
> > > iput(inode);
> > >
> > > - /* 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 */
> > > + /*
> > > + * 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, netfid,
> > > 0 /* len */ , 0 /* offset */, 0,
> > > @@ -1042,6 +1043,12 @@ static int cifs_oplock_thread(void *dummyarg)
> > > false /* wait flag */);
> > > cFYI(1, ("Oplock release rc = %d", rc));
> >
> > Is it OK to make an SMB call whiile holding the mutex lock?
> > Also wonder if rc has any meaning i.e. can it fail in any way and if so does
> > it have any import in this function.
> >
>
> I believe it's safe. The mutex is only intended to protect the list.
>
> That said, the locking in these codepaths is very complex, so a
> critical eye for deadlocks would be a good thing here.
>
Actually...now that I look with a fresh eye. The locking changes seem
safe enough, but there's still a potential race with this set. In
is_valid_oplock_break(), the code drops the cifs_tcp_ses_lock and then
calls AllocOplockQEntry. The problem there is that the tcon may be
invalid by the time you create the new entry.
I think I need to rework this set to just take a reference on the tcon
after all and have cifs_oplock_thread put it as necessary. Let me
respin and retest and I'll re-post in a few days.
--
Jeff Layton <jlayton at redhat.com>
More information about the linux-cifs-client
mailing list