[linux-cifs-client] [PATCH 2/5] cifs: take tcon reference for oplock breaks
Shirish Pargaonkar
shirishpargaonkar at gmail.com
Sat Aug 29 23:10:39 MDT 2009
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);
> + 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
>
Acked-by: Shirish Pargaonkar <shirishpargaonkar at gmail.com>
More information about the linux-cifs-client
mailing list