diff mbox

[linux-cifs-client,4/5] cifs: take reference to inode for oplock breaks

Message ID 20090831104124.3c85203c@tlielax.poochiereds.net (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton Aug. 31, 2009, 2:41 p.m. UTC 
On Fri, 28 Aug 2009 14:53:19 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@redhat.com> wrote:
> > When an oplock break comes in, cifs needs to do things like write out
> > dirty data for it. It doesn't hold a reference to that inode in this
> > case however. Get an active reference to the inode when an oplock break
> > comes in. If we don't get a reference, we still need to create an oplock
> > queue entry so that the oplock release call gets done, but we'll want to
> > skip writeback in that case.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/cifsfs.c |   26 ++++++++++----------------
> >  fs/cifs/misc.c   |    4 +++-
> >  2 files changed, 13 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 2fcc722..647c5bc 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -993,13 +993,8 @@ static int cifs_oplock_thread(void *dummyarg)
> >                        cFYI(1, ("found oplock item to write out"));
> >                        tcon = oplock->tcon;
> >                        inode = oplock->pinode;
> > -                       /* can not grab inode sem here since it would
> > -                               deadlock when oplock received on delete
> > -                               since vfs_unlink holds the i_mutex across
> > -                               the call */
> > -                       /* mutex_lock(&inode->i_mutex);*/
> > -                       cifsi = CIFS_I(inode);
> > -                       if (S_ISREG(inode->i_mode)) {
> > +                       if (inode && S_ISREG(inode->i_mode)) {
> > +                               cifsi = CIFS_I(inode);
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> >                                if (cifsi->clientCanCacheAll == 0)
> >                                        break_lease(inode, FMODE_READ);
> > @@ -1010,17 +1005,16 @@ static int cifs_oplock_thread(void *dummyarg)
> >                                if (cifsi->clientCanCacheRead == 0) {
> >                                        waitrc = filemap_fdatawait(
> >                                                              inode->i_mapping);
> > +                                       if (rc == 0)
> > +                                               rc = waitrc;
> >                                        invalidate_remote_inode(inode);
> >                                }
> > -                               if (rc == 0)
> > -                                       rc = waitrc;
> > -                       } else
> > -                               rc = 0;
> > -                       /* mutex_unlock(&inode->i_mutex);*/
> > -                       if (rc)
> > -                               cifsi->write_behind_rc = rc;
> > -                       cFYI(1, ("Oplock flush inode %p rc %d",
> > -                               inode, rc));
> > +                               if (rc)
> > +                                       cifsi->write_behind_rc = rc;
> > +                               cFYI(1, ("Oplock flush inode %p rc %d",
> > +                                        inode, rc));
> > +                       }
> > +                       iput(inode);
> >
> >                        /*
> >                         * releasing stale oplock after recent reconnect
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index 7221af9..3bf3a52 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -502,6 +502,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
> >        struct cifsInodeInfo *pCifsInode;
> >        struct cifsFileInfo *netfile;
> >        struct oplock_q_entry *oplock;
> > +       struct inode *inode;
> >
> >        cFYI(1, ("Checking for oplock break or dnotify response"));
> >        if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
> > @@ -600,6 +601,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
> >                                pCifsInode->clientCanCacheAll = false;
> >                                if (pSMB->OplockLevel == 0)
> >                                        pCifsInode->clientCanCacheRead = false;
> > +                               inode = igrab(netfile->pInode);
> >
> >                                if (!oplock) {
> >                                        cERROR(1, ("Unable to allocate "
> > @@ -608,7 +610,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
> >                                }
> >
> >                                oplock->tcon = tcon;
> > -                               oplock->pinode = netfile->pInode;
> > +                               oplock->pinode = inode;
> >                                oplock->netfid = netfile->netfid;
> >                                spin_lock(&cifs_oplock_lock);
> >                                list_add_tail(&oplock->qhead,
> > --
> > 1.6.0.6
> >

Just found a bug with this patch. It takes an inode reference even if
an oplock queue entry couldn't be allocated. The fix is simple (just
move the igrab to later in the function). Replacement patch is attached.

Comments

Shirish Pargaonkar Aug. 31, 2009, 3:17 p.m. UTC | #1 
On Mon, Aug 31, 2009 at 9:41 AM, Jeff Layton<jlayton@redhat.com> wrote:
> On Fri, 28 Aug 2009 14:53:19 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>
>> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@redhat.com> wrote:
>> > When an oplock break comes in, cifs needs to do things like write out
>> > dirty data for it. It doesn't hold a reference to that inode in this
>> > case however. Get an active reference to the inode when an oplock break
>> > comes in. If we don't get a reference, we still need to create an oplock
>> > queue entry so that the oplock release call gets done, but we'll want to
>> > skip writeback in that case.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> > ---
>> >  fs/cifs/cifsfs.c |   26 ++++++++++----------------
>> >  fs/cifs/misc.c   |    4 +++-
>> >  2 files changed, 13 insertions(+), 17 deletions(-)
>> >
>> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> > index 2fcc722..647c5bc 100644
>> > --- a/fs/cifs/cifsfs.c
>> > +++ b/fs/cifs/cifsfs.c
>> > @@ -993,13 +993,8 @@ static int cifs_oplock_thread(void *dummyarg)
>> >                        cFYI(1, ("found oplock item to write out"));
>> >                        tcon = oplock->tcon;
>> >                        inode = oplock->pinode;
>> > -                       /* can not grab inode sem here since it would
>> > -                               deadlock when oplock received on delete
>> > -                               since vfs_unlink holds the i_mutex across
>> > -                               the call */
>> > -                       /* mutex_lock(&inode->i_mutex);*/
>> > -                       cifsi = CIFS_I(inode);
>> > -                       if (S_ISREG(inode->i_mode)) {
>> > +                       if (inode && S_ISREG(inode->i_mode)) {
>> > +                               cifsi = CIFS_I(inode);
>> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
>> >                                if (cifsi->clientCanCacheAll == 0)
>> >                                        break_lease(inode, FMODE_READ);
>> > @@ -1010,17 +1005,16 @@ static int cifs_oplock_thread(void *dummyarg)
>> >                                if (cifsi->clientCanCacheRead == 0) {
>> >                                        waitrc = filemap_fdatawait(
>> >                                                              inode->i_mapping);
>> > +                                       if (rc == 0)
>> > +                                               rc = waitrc;
>> >                                        invalidate_remote_inode(inode);
>> >                                }
>> > -                               if (rc == 0)
>> > -                                       rc = waitrc;
>> > -                       } else
>> > -                               rc = 0;
>> > -                       /* mutex_unlock(&inode->i_mutex);*/
>> > -                       if (rc)
>> > -                               cifsi->write_behind_rc = rc;
>> > -                       cFYI(1, ("Oplock flush inode %p rc %d",
>> > -                               inode, rc));
>> > +                               if (rc)
>> > +                                       cifsi->write_behind_rc = rc;
>> > +                               cFYI(1, ("Oplock flush inode %p rc %d",
>> > +                                        inode, rc));
>> > +                       }
>> > +                       iput(inode);
>> >
>> >                        /*
>> >                         * releasing stale oplock after recent reconnect
>> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
>> > index 7221af9..3bf3a52 100644
>> > --- a/fs/cifs/misc.c
>> > +++ b/fs/cifs/misc.c
>> > @@ -502,6 +502,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
>> >        struct cifsInodeInfo *pCifsInode;
>> >        struct cifsFileInfo *netfile;
>> >        struct oplock_q_entry *oplock;
>> > +       struct inode *inode;
>> >
>> >        cFYI(1, ("Checking for oplock break or dnotify response"));
>> >        if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
>> > @@ -600,6 +601,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
>> >                                pCifsInode->clientCanCacheAll = false;
>> >                                if (pSMB->OplockLevel == 0)
>> >                                        pCifsInode->clientCanCacheRead = false;
>> > +                               inode = igrab(netfile->pInode);
>> >
>> >                                if (!oplock) {
>> >                                        cERROR(1, ("Unable to allocate "
>> > @@ -608,7 +610,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
>> >                                }
>> >
>> >                                oplock->tcon = tcon;
>> > -                               oplock->pinode = netfile->pInode;
>> > +                               oplock->pinode = inode;
>> >                                oplock->netfid = netfile->netfid;
>> >                                spin_lock(&cifs_oplock_lock);
>> >                                list_add_tail(&oplock->qhead,
>> > --
>> > 1.6.0.6
>> >
>
> Just found a bug with this patch. It takes an inode reference even if
> an oplock queue entry couldn't be allocated. The fix is simple (just
> move the igrab to later in the function). Replacement patch is attached.
>
> --
> Jeff Layton <jlayton@redhat.com>
>

Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
diff mbox

Patch

From f09dffdc5bf519091e57442a433c15b3569d43da Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Mon, 31 Aug 2009 07:07:11 -0400
Subject: [PATCH] cifs: take reference to inode for oplock breaks

When an oplock break comes in, cifs needs to do things like write out
dirty data for it. It doesn't hold a reference to that inode in this
case however. Get an active reference to the inode when an oplock break
comes in. If we don't get a reference, we still need to create an oplock
queue entry so that the oplock release call gets done, but we'll want to
skip writeback in that case.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsfs.c |   26 ++++++++++----------------
 fs/cifs/misc.c   |    2 +-
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 2fcc722..647c5bc 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -993,13 +993,8 @@  static int cifs_oplock_thread(void *dummyarg)
 			cFYI(1, ("found oplock item to write out"));
 			tcon = oplock->tcon;
 			inode = oplock->pinode;
-			/* can not grab inode sem here since it would
-				deadlock when oplock received on delete
-				since vfs_unlink holds the i_mutex across
-				the call */
-			/* mutex_lock(&inode->i_mutex);*/
-			cifsi = CIFS_I(inode);
-			if (S_ISREG(inode->i_mode)) {
+			if (inode && S_ISREG(inode->i_mode)) {
+				cifsi = CIFS_I(inode);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
 				if (cifsi->clientCanCacheAll == 0)
 					break_lease(inode, FMODE_READ);
@@ -1010,17 +1005,16 @@  static int cifs_oplock_thread(void *dummyarg)
 				if (cifsi->clientCanCacheRead == 0) {
 					waitrc = filemap_fdatawait(
 							      inode->i_mapping);
+					if (rc == 0)
+						rc = waitrc;
 					invalidate_remote_inode(inode);
 				}
-				if (rc == 0)
-					rc = waitrc;
-			} else
-				rc = 0;
-			/* mutex_unlock(&inode->i_mutex);*/
-			if (rc)
-				cifsi->write_behind_rc = rc;
-			cFYI(1, ("Oplock flush inode %p rc %d",
-				inode, rc));
+				if (rc)
+					cifsi->write_behind_rc = rc;
+				cFYI(1, ("Oplock flush inode %p rc %d",
+					 inode, rc));
+			}
+			iput(inode);
 
 			/*
 			 * releasing stale oplock after recent reconnect
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index acdd9fa..ec98910 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -608,7 +608,7 @@  is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
 				}
 
 				oplock->tcon = tcon;
-				oplock->pinode = netfile->pInode;
+				oplock->pinode = igrab(netfile->pInode);
 				oplock->netfid = netfile->netfid;
 				spin_lock(&cifs_oplock_lock);
 				list_add_tail(&oplock->qhead,
-- 
1.6.0.6