From patchwork Tue Aug 18 18:06:59 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 42399 Received: from lists.samba.org (fn.samba.org [216.83.154.106]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n7II7FTw005697 for ; Tue, 18 Aug 2009 18:07:15 GMT Received: from fn.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id 86D50ACFC8; Tue, 18 Aug 2009 12:00:41 -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=-3.3 required=3.8 tests=AWL, BAYES_00, NO_MORE_FUNN, SPF_PASS autolearn=no version=3.2.5 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from cdptpa-omtalb.mail.rr.com (cdptpa-omtalb.mail.rr.com [75.180.132.122]) by lists.samba.org (Postfix) with ESMTP id A1E21ACFB0 for ; Tue, 18 Aug 2009 12:00:30 -0600 (MDT) Received: from mail.poochiereds.net ([71.70.153.3]) by cdptpa-omta04.mail.rr.com with ESMTP id <20090818180703416.FUZD6077@cdptpa-omta04.mail.rr.com>; Tue, 18 Aug 2009 18:07:03 +0000 Received: by mail.poochiereds.net (Postfix, from userid 4447) id 01CA35814C; Tue, 18 Aug 2009 14:07:03 -0400 (EDT) From: Jeff Layton To: smfrench@gmail.com Date: Tue, 18 Aug 2009 14:06:59 -0400 Message-Id: <1250618822-6131-3-git-send-email-jlayton@redhat.com> X-Mailer: git-send-email 1.6.0.6 In-Reply-To: <1250618822-6131-1-git-send-email-jlayton@redhat.com> References: <1250618822-6131-1-git-send-email-jlayton@redhat.com> Cc: linux-cifs-client@lists.samba.org Subject: [linux-cifs-client] [PATCH 2/5] cifs: take tcon reference 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: , MIME-Version: 1.0 Sender: linux-cifs-client-bounces@lists.samba.org Errors-To: linux-cifs-client-bounces@lists.samba.org When an oplock break comes in, we search for a tcon that matches it and make an oplock queue entry with a pointer to that tcon. There's a problem here, however. That tcon may disappear before the oplock thread can get around to using it. Take a reference to the tcon when we find the matching one and have the cifs_oplock_thread put it when it's finished with the tcon. There's a problem though, we can't *put* the last reference to a tcon within the context of cifsd since that would deadlock. So we need to take some extra precautions to only take a reference if we can actually use it. To do this, we get rid of all of the existing tcon allocation and deletion routines. The way that they handle the locking is racy, and in many cases they only have one caller anyway. Signed-off-by: Jeff Layton Acked-by: Shirish Pargaonkar --- fs/cifs/cifsfs.c | 7 ++--- fs/cifs/cifsglob.h | 3 +- fs/cifs/cifsproto.h | 8 ++---- fs/cifs/connect.c | 10 ++++++-- fs/cifs/misc.c | 53 +++++++++++++++++++++++++++++++++++++++++++------- fs/cifs/transport.c | 51 ------------------------------------------------- 6 files changed, 60 insertions(+), 72 deletions(-) diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 3610e99..7ce8dcd 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -89,8 +89,6 @@ extern mempool_t *cifs_sm_req_poolp; extern mempool_t *cifs_req_poolp; extern mempool_t *cifs_mid_poolp; -extern struct kmem_cache *cifs_oplock_cachep; - static int cifs_read_super(struct super_block *sb, void *data, const char *devname, int silent) @@ -294,7 +292,6 @@ static int cifs_permission(struct inode *inode, int mask) static struct kmem_cache *cifs_inode_cachep; static struct kmem_cache *cifs_req_cachep; static struct kmem_cache *cifs_mid_cachep; -struct kmem_cache *cifs_oplock_cachep; static struct kmem_cache *cifs_sm_req_cachep; mempool_t *cifs_sm_req_poolp; mempool_t *cifs_req_poolp; @@ -998,8 +995,9 @@ static int cifs_oplock_thread(void *dummyarg) pTcon = oplock_item->tcon; inode = oplock_item->pinode; netfid = oplock_item->netfid; + list_del(&oplock_item->qhead); spin_unlock(&cifs_oplock_lock); - DeleteOplockQEntry(oplock_item); + kmem_cache_free(cifs_oplock_cachep, oplock_item); /* can not grab inode sem here since it would deadlock when oplock received on delete since vfs_unlink holds the i_mutex across @@ -1041,6 +1039,7 @@ static int cifs_oplock_thread(void *dummyarg) false /* wait flag */); cFYI(1, ("Oplock release rc = %d", rc)); } + cifs_put_tcon(pTcon); set_current_state(TASK_INTERRUPTIBLE); schedule_timeout(1); /* yield in case q were corrupt */ } diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index f100399..363dbcf 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -369,7 +369,6 @@ struct cifsInodeInfo { unsigned long time; /* jiffies of last update/check of inode */ bool clientCanCacheRead:1; /* read oplock */ bool clientCanCacheAll:1; /* read and writebehind oplock */ - bool oplockPending:1; bool delete_pending:1; /* DELETE_ON_CLOSE is set */ u64 server_eof; /* current file size on server */ u64 uniqueid; /* server inode number */ @@ -656,6 +655,8 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock; */ GLOBAL_EXTERN rwlock_t GlobalSMBSeslock; +GLOBAL_EXTERN struct kmem_cache *cifs_oplock_cachep; + /* Global list of oplocks */ GLOBAL_EXTERN struct list_head cifs_oplock_list; diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index da8fbf5..623d928 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -64,7 +64,8 @@ extern int SendReceiveBlockingLock(const unsigned int xid, int *bytes_returned); extern int checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length); extern bool is_valid_oplock_break(struct smb_hdr *smb, - struct TCP_Server_Info *); + struct TCP_Server_Info *, + struct oplock_q_entry **); extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof); extern struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *); #ifdef CONFIG_CIFS_EXPERIMENTAL @@ -86,10 +87,6 @@ extern int CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses, const int stage, const struct nls_table *nls_cp); extern __u16 GetNextMid(struct TCP_Server_Info *server); -extern struct oplock_q_entry *AllocOplockQEntry(struct inode *, u16, - struct cifsTconInfo *); -extern void DeleteOplockQEntry(struct oplock_q_entry *); -extern void DeleteTconOplockQEntries(struct cifsTconInfo *); extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601); extern u64 cifs_UnixTimeToNT(struct timespec); extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, @@ -117,6 +114,7 @@ extern void cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, const char *path, const __u16 *pfid); extern int mode_to_acl(struct inode *inode, const char *path, __u64); +void cifs_put_tcon(struct cifsTconInfo *tcon); extern int cifs_mount(struct super_block *, struct cifs_sb_info *, char *, const char *); extern int cifs_umount(struct super_block *, struct cifs_sb_info *); diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index edf8765..f49304d 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -331,6 +331,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server) struct cifsSesInfo *ses; struct task_struct *task_to_wake = NULL; struct mid_q_entry *mid_entry; + struct oplock_q_entry *oplock_entry = NULL; char temp; bool isLargeBuf = false; bool isMultiRsp; @@ -626,7 +627,8 @@ multi_t2_fnd: smallbuf = NULL; } wake_up_process(task_to_wake); - } else if (!is_valid_oplock_break(smb_buffer, server) && + } else if (!is_valid_oplock_break(smb_buffer, server, + &oplock_entry) && !isMultiRsp) { cERROR(1, ("No task to wake, unknown frame received! " "NumMids %d", midCount.counter)); @@ -640,6 +642,9 @@ multi_t2_fnd: } } /* end while !EXITING */ + if (oplock_entry) + kmem_cache_free(cifs_oplock_cachep, oplock_entry); + /* take it off the list, if it's not already */ write_lock(&cifs_tcp_ses_lock); list_del_init(&server->tcp_ses_list); @@ -1624,7 +1629,7 @@ cifs_find_tcon(struct cifsSesInfo *ses, const char *unc) return NULL; } -static void +void cifs_put_tcon(struct cifsTconInfo *tcon) { int xid; @@ -1643,7 +1648,6 @@ cifs_put_tcon(struct cifsTconInfo *tcon) CIFSSMBTDis(xid, tcon); _FreeXid(xid); - DeleteTconOplockQEntries(tcon); tconInfoFree(tcon); cifs_put_smb_ses(ses); } diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index e079a91..7221af9 100644 --- a/fs/cifs/misc.c +++ b/fs/cifs/misc.c @@ -492,7 +492,8 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length) } bool -is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) +is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv, + struct oplock_q_entry **oplock_entry) { struct smb_com_lock_req *pSMB = (struct smb_com_lock_req *)buf; struct list_head *tmp, *tmp1, *tmp2; @@ -500,6 +501,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) struct cifsTconInfo *tcon; struct cifsInodeInfo *pCifsInode; struct cifsFileInfo *netfile; + struct oplock_q_entry *oplock; cFYI(1, ("Checking for oplock break or dnotify response")); if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) && @@ -552,8 +554,23 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) if (!(pSMB->LockType & LOCKING_ANDX_OPLOCK_RELEASE)) return false; + /* + * If we succeed in finding a matching tcon and fid, then we'll need + * an allocated oplock queue entry. But at that point we'll have an + * active tcon reference. If the allocation fails, we cannot put + * the reference within the context of cifs_demultiplex_thread. + * + * So, we must instead pre-allocate this entry in case it's needed. + * If it isn't however, then we can just hold on to it until one is. + */ + if (!*oplock_entry) + *oplock_entry = (struct oplock_q_entry *) + kmem_cache_alloc(cifs_oplock_cachep, + GFP_KERNEL); + oplock = *oplock_entry; + /* look up tcon based on tid & uid */ - read_lock(&cifs_tcp_ses_lock); + write_lock(&cifs_tcp_ses_lock); list_for_each(tmp, &srv->smb_ses_list) { ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list); list_for_each(tmp1, &ses->tcon_list) { @@ -569,16 +586,36 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) if (pSMB->Fid != netfile->netfid) continue; + /* + * can't put reference later if we don't have + * an oplock entry. So only take a reference + * if we had a successful allocation. + */ + if (oplock) + ++tcon->tc_count; write_unlock(&GlobalSMBSeslock); - read_unlock(&cifs_tcp_ses_lock); + write_unlock(&cifs_tcp_ses_lock); cFYI(1, ("file id match, oplock break")); pCifsInode = CIFS_I(netfile->pInode); pCifsInode->clientCanCacheAll = false; if (pSMB->OplockLevel == 0) pCifsInode->clientCanCacheRead = false; - pCifsInode->oplockPending = true; - AllocOplockQEntry(netfile->pInode, - netfile->netfid, tcon); + + if (!oplock) { + cERROR(1, ("Unable to allocate " + "queue entry!")); + return true; + } + + oplock->tcon = tcon; + oplock->pinode = netfile->pInode; + oplock->netfid = netfile->netfid; + spin_lock(&cifs_oplock_lock); + list_add_tail(&oplock->qhead, + &cifs_oplock_list); + spin_unlock(&cifs_oplock_lock); + *oplock_entry = NULL; + cFYI(1, ("about to wake up oplock thread")); if (oplockThread) wake_up_process(oplockThread); @@ -586,12 +623,12 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv) return true; } write_unlock(&GlobalSMBSeslock); - read_unlock(&cifs_tcp_ses_lock); + write_unlock(&cifs_tcp_ses_lock); cFYI(1, ("No matching file for oplock break")); return true; } } - read_unlock(&cifs_tcp_ses_lock); + write_unlock(&cifs_tcp_ses_lock); cFYI(1, ("Can not process oplock break for non-existent connection")); return true; } diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 1da4ab2..ae22ff2 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -34,7 +34,6 @@ #include "cifs_debug.h" extern mempool_t *cifs_mid_poolp; -extern struct kmem_cache *cifs_oplock_cachep; static struct mid_q_entry * AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server) @@ -103,56 +102,6 @@ DeleteMidQEntry(struct mid_q_entry *midEntry) mempool_free(midEntry, cifs_mid_poolp); } -struct oplock_q_entry * -AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon) -{ - struct oplock_q_entry *temp; - if ((pinode == NULL) || (tcon == NULL)) { - cERROR(1, ("Null parms passed to AllocOplockQEntry")); - return NULL; - } - temp = (struct oplock_q_entry *) kmem_cache_alloc(cifs_oplock_cachep, - GFP_KERNEL); - if (temp == NULL) - return temp; - else { - temp->pinode = pinode; - temp->tcon = tcon; - temp->netfid = fid; - spin_lock(&cifs_oplock_lock); - list_add_tail(&temp->qhead, &cifs_oplock_list); - spin_unlock(&cifs_oplock_lock); - } - return temp; -} - -void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry) -{ - spin_lock(&cifs_oplock_lock); - /* should we check if list empty first? */ - list_del(&oplockEntry->qhead); - spin_unlock(&cifs_oplock_lock); - kmem_cache_free(cifs_oplock_cachep, oplockEntry); -} - - -void DeleteTconOplockQEntries(struct cifsTconInfo *tcon) -{ - struct oplock_q_entry *temp; - - if (tcon == NULL) - return; - - spin_lock(&cifs_oplock_lock); - list_for_each_entry(temp, &cifs_oplock_list, qhead) { - if ((temp->tcon) && (temp->tcon == tcon)) { - list_del(&temp->qhead); - kmem_cache_free(cifs_oplock_cachep, temp); - } - } - spin_unlock(&cifs_oplock_lock); -} - static int smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec) {