Re: [PATCH 2/5] cifs: take tcon reference for oplock breaks

On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<[email protected]> 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 <[email protected]>
> ---
>  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
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>