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

[Shirish Pargaonkar]

On Mon, Aug 31, 2009 at 9:41 AM, Jeff Layton<jlayton at redhat.com> wrote:
> 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>
>

Acked-by: Shirish Pargaonkar <shirishpargaonkar at gmail.com>