diff mbox

[linux-cifs-client,4/4] cifs: hold cifs_oplock_mutex while doing oplock release call

Message ID 1250511368-9812-5-git-send-email-jlayton@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton Aug. 17, 2009, 12:16 p.m. UTC 
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@redhat.com>
---
 fs/cifs/cifsfs.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

Comments

Shirish Pargaonkar Aug. 17, 2009, 3:18 p.m. UTC | #1 
On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@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@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.

>                        }
> +
> +                       /*
> +                        * 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
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>
Jeff Layton Aug. 17, 2009, 3:49 p.m. UTC | #2 
On Mon, 17 Aug 2009 10:18:51 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@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@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.

> >                        }
> > +
> > +                       /*
> > +                        * 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
> > linux-cifs-client@lists.samba.org
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >
Jeff Layton Aug. 17, 2009, 4:24 p.m. UTC | #3 
On Mon, 17 Aug 2009 11:49:03 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Mon, 17 Aug 2009 10:18:51 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> 
> > On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@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@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.
diff mbox

Patch

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));
 			}
+
+			/*
+			 * 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);