[linux-cifs-client] [PATCH 2/5] cifs: take tcon reference for oplock breaks
Jeff Layton
jlayton at redhat.com
Sat Aug 29 17:13:45 MDT 2009
On Sat, 29 Aug 2009 12:17:13 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
> On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<jlayton at 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 at 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 at lists.samba.org
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >
More information about the linux-cifs-client
mailing list