Re: [PATCH 5/5] cifs: cancel oplock release callbacks during reconnect

On Thu, 20 Aug 2009 19:45:15 -0500
Steve French <[email protected]> wrote:

> ACK -  (although an extra oplock break on an incorrect file handle is not a
> big deal, slightly slower, and would be very rare - this approach is
> cleaner)
> 

I was also worried about what happens if you get an oplock break and
then a reconnect causes the fid to change to a completely different
file. The client would send the oplock release and then proceed
thinking that it had an oplock on the other file at that point when it
really didn't.

Not an extremely likely race, but simple enough to prevent.

> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <[email protected]> wrote:
> 
> > cifs_oplock_thread has a check for pTcon->needs_reconnect and will skip
> > the CIFSSMBLock call if it's set.
> >
> > Problem: what if the tcon has this set and then gets reconnected before
> > the call goes out on the wire? The oplock release isn't needed and could
> > be a bad thing at that point if the filehandle was reclaimed.
> >
> > Cancel oplock release calls during a reconnect event.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> >  fs/cifs/cifsfs.c   |    2 +-
> >  fs/cifs/cifsglob.h |    1 +
> >  fs/cifs/connect.c  |    9 +++++++++
> >  fs/cifs/misc.c     |    1 +
> >  4 files changed, 12 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 647c5bc..92e06c1 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -1024,7 +1024,7 @@ static int cifs_oplock_thread(void *dummyarg)
> >                         * to server still is disconnected since oplock
> >                         * already released by the server in that case
> >                         */
> > -                       if (!tcon->need_reconnect) {
> > +                       if (!oplock->cancel) {
> >                                rc = CIFSSMBLock(0, tcon, oplock->netfid,
> >                                                0 /* len */ , 0 /* offset
> > */, 0,
> >                                                0,
> > LOCKING_ANDX_OPLOCK_RELEASE,
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 363dbcf..676c107 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -448,6 +448,7 @@ struct oplock_q_entry {
> >        struct inode *pinode;
> >        struct cifsTconInfo *tcon;
> >        __u16 netfid;
> > +       bool cancel;
> >  };
> >
> >  /* for pending dnotify requests */
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index f49304d..b7b67e9 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -121,6 +121,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >        struct cifsSesInfo *ses;
> >        struct cifsTconInfo *tcon;
> >        struct mid_q_entry *mid_entry;
> > +       struct oplock_q_entry *oplock;
> >
> >        spin_lock(&GlobalMid_Lock);
> >        if (server->tcpStatus == CifsExiting) {
> > @@ -137,6 +138,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >
> >        /* before reconnecting the tcp session, mark the smb session (uid)
> >                and the tid bad so they are not used until reconnected */
> > +       spin_lock(&cifs_oplock_lock);
> >        read_lock(&cifs_tcp_ses_lock);
> >        list_for_each(tmp, &server->smb_ses_list) {
> >                ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> > @@ -145,9 +147,16 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >                list_for_each(tmp2, &ses->tcon_list) {
> >                        tcon = list_entry(tmp2, struct cifsTconInfo,
> > tcon_list);
> >                        tcon->need_reconnect = true;
> > +                       list_for_each_entry(oplock, &cifs_oplock_list,
> > qhead) {
> > +                               if (oplock->tcon == tcon)
> > +                                       oplock->cancel = true;
> > +                       }
> >                }
> > +
> >        }
> >        read_unlock(&cifs_tcp_ses_lock);
> > +       spin_unlock(&cifs_oplock_lock);
> > +
> >        /* do not want to be sending data on a socket we are freeing */
> >        mutex_lock(&server->srv_mutex);
> >        if (server->ssocket) {
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index 3bf3a52..4a2d297 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -612,6 +612,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct
> > TCP_Server_Info *srv,
> >                                oplock->tcon = tcon;
> >                                oplock->pinode = inode;
> >                                oplock->netfid = netfile->netfid;
> > +                               oplock->cancel = false;
> >                                spin_lock(&cifs_oplock_lock);
> >                                list_add_tail(&oplock->qhead,
> >                                              &cifs_oplock_list);
> > --
> > 1.6.0.6
> >
> >
> 
> 


-- 
Jeff Layton <[email protected]>