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

[Jeff Layton]

On Fri, 28 Aug 2009 14:53:19 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:

> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton at 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 at 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 at redhat.com>