From patchwork Mon Aug 31 14:41:24 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 32651 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.samba.org (fn.samba.org [216.83.154.106]) by bilbo.ozlabs.org (Postfix) with ESMTP id 83BE8B7B71 for ; Tue, 1 Sep 2009 00:41:35 +1000 (EST) Received: from fn.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id C1CBEAD02B; Mon, 31 Aug 2009 08:32:19 -0600 (MDT) X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on fn.samba.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.8 tests=BAYES_00,SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.2.5 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by lists.samba.org (Postfix) with ESMTP id 2DCB4ACF46 for ; Mon, 31 Aug 2009 08:32:14 -0600 (MDT) Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n7VEfRPQ005931; Mon, 31 Aug 2009 10:41:27 -0400 Received: from tlielax.poochiereds.net (vpn-11-144.rdu.redhat.com [10.11.11.144]) by int-mx08.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id n7VEfQWv020487; Mon, 31 Aug 2009 10:41:26 -0400 Date: Mon, 31 Aug 2009 10:41:24 -0400 From: Jeff Layton To: Shirish Pargaonkar Message-ID: <20090831104124.3c85203c@tlielax.poochiereds.net> In-Reply-To: <4a4634330908281253j2b22c114v8e3f32faf1b543fa@mail.gmail.com> References: <1250618822-6131-1-git-send-email-jlayton@redhat.com> <1250618822-6131-5-git-send-email-jlayton@redhat.com> <4a4634330908281253j2b22c114v8e3f32faf1b543fa@mail.gmail.com> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.21 Cc: smfrench@gmail.com, linux-cifs-client@lists.samba.org Subject: Re: [linux-cifs-client] [PATCH 4/5] cifs: take reference to inode for oplock breaks X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces@lists.samba.org Errors-To: linux-cifs-client-bounces@lists.samba.org On Fri, 28 Aug 2009 14:53:19 -0500 Shirish Pargaonkar wrote: > On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton 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 > > --- > >  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. Acked-by: Shirish Pargaonkar From f09dffdc5bf519091e57442a433c15b3569d43da Mon Sep 17 00:00:00 2001 From: Jeff Layton 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 --- 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