Re: [PATCH 4/4] cifs: hold cifs_oplock_mutex while doing oplock release call

On Mon, 17 Aug 2009 10:18:51 -0500
Shirish Pargaonkar <[email protected]> wrote:

> On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<[email protected]> 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 <[email protected]>
> > ---
> >  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.

> >                        }
> > +
> > +                       /*
> > +                        * release and reacquire the oplock mutex in order to
> > +                        * allow tasks waiting on it to have a chance.
> > +                        */
> > +                       mutex_unlock(&cifs_oplock_mutex);
> >                        mutex_lock(&cifs_oplock_mutex);
> >                }
> >                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
> >


-- 
Jeff Layton <[email protected]>