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

On Sat, 29 Aug 2009 12:17:13 -0500
Shirish Pargaonkar <[email protected]> wrote:

> 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)
> 

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
> > [email protected]
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >