Re: [PATCH 4/5] cifs: take reference to inode for oplock breaks

On Thu, 20 Aug 2009 19:42:42 -0500
Steve French <[email protected]> wrote:

> Why don't we have a reference to this inode already?   We can't have an
> oplock break unless the file is open, if the file is open then we have a
> reference to the inode ...
> 

Well, you have a reference when the entry goes onto the list. There's
no guarantee that you'll still have one when cifs_oplock_thread goes to
take it off the list however. The file could have been closed by then
and the inode flushed out of the cache. Since the cifs_oplock_thread
happens outside of any "normal" process behavior, we really need to
take an inode reference here.


> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > ---
> >  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
> >
> >
> 
> 


-- 
Jeff Layton <[email protected]>