| Message ID | 1250618822-6131-3-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<jlayton@redhat.com> wrote: > 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 <jlayton@redhat.com> > --- > 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); The only comment I have here is, do we need to check whether oplock_entry is null or not? There are other places in cifs code where we check for that (return value of kmem_cache_alloc) > + 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) > { > -- > 1.6.0.6 > > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client >
On Sat, 29 Aug 2009 12:17:13 -0500 Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<jlayton@redhat.com> wrote: > > 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 <jlayton@redhat.com> > > --- > > 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); > > The only comment I have here is, do we need to check whether > oplock_entry is null or not? There are other places in cifs code > where we check for that (return value of kmem_cache_alloc) > This is probably the most confusing part of this patch. It's awkward, but I didn't see a way around it given the way that this code works. oplock_entry will never be NULL since it's a pointer on the stack of cifs_demultiplex_thread and gets passed into this function. *oplock_entry (and hence "oplock") could be however and the code checks for that a little farther down before trying to use the allocated memory. > > + 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) > > { > > -- > > 1.6.0.6 > > > > _______________________________________________ > > linux-cifs-client mailing list > > linux-cifs-client@lists.samba.org > > https://lists.samba.org/mailman/listinfo/linux-cifs-client > >
On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<jlayton@redhat.com> wrote: > 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 <jlayton@redhat.com> > --- > 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) > { > -- > 1.6.0.6 > > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client > Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
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) {
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 <jlayton@redhat.com> --- 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(-)