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

login
register
mail settings
Submitter Jeff Layton
Date Aug. 18, 2009, 6:07 p.m.
Message ID <1250618822-6131-6-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/31582/
State New
Headers show

Comments

Jeff Layton - Aug. 18, 2009, 6:07 p.m.
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 <jlayton@redhat.com>
---
 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(-)
Steve French - Aug. 21, 2009, 12:45 a.m.
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)

On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <jlayton@redhat.com> 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 <jlayton@redhat.com>
> ---
>  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 - Aug. 21, 2009, 10:41 a.m.
On Thu, 20 Aug 2009 19:45:15 -0500
Steve French <smfrench@gmail.com> 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 <jlayton@redhat.com> 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 <jlayton@redhat.com>
> > ---
> >  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
> >
> >
> 
>
Shirish Pargaonkar - Aug. 28, 2009, 7:14 p.m.
On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@redhat.com> 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 <jlayton@redhat.com>
> ---
>  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
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>


Acked-by: Shirish Pargaonkar <shiishpargaonkar@yahoo.com>

with a question, does what you say about pTcon->needs_reconnect  apply
to oplock->cancel also?
Jeff Layton - Aug. 28, 2009, 7:26 p.m.
On Fri, 28 Aug 2009 14:14:43 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@redhat.com> 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 <jlayton@redhat.com>
> > ---
> >  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
> >
> > _______________________________________________
> > linux-cifs-client mailing list
> > linux-cifs-client@lists.samba.org
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >
> 
> 
> Acked-by: Shirish Pargaonkar <shiishpargaonkar@yahoo.com>
> 
> with a question, does what you say about pTcon->needs_reconnect  apply
> to oplock->cancel also?

I don't think so. oplock->cancel doesn't get cleared on a reconnect.

...or did I misunderstand the question?

Thanks,
Shirish Pargaonkar - Aug. 28, 2009, 7:38 p.m.
On Fri, Aug 28, 2009 at 2:26 PM, Jeff Layton<jlayton@redhat.com> wrote:
> On Fri, 28 Aug 2009 14:14:43 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>
>> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@redhat.com> 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 <jlayton@redhat.com>
>> > ---
>> >  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
>> >
>> > _______________________________________________
>> > linux-cifs-client mailing list
>> > linux-cifs-client@lists.samba.org
>> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
>> >
>>
>>
>> Acked-by: Shirish Pargaonkar <shiishpargaonkar@yahoo.com>
>>
>> with a question, does what you say about pTcon->needs_reconnect  apply
>> to oplock->cancel also?
>
> I don't think so. oplock->cancel doesn't get cleared on a reconnect.
>
> ...or did I misunderstand the question?
>
> Thanks,
> --
> Jeff Layton <jlayton@redhat.com>
>

Jeff, I was thinking, if oplock->cancel is false but before call is
sent out, reconnect happens
and it is not necessary to send the call (oplock->cancel is true) and
yet it could get sent,
similar to what can happen in case of tcon->need_reconnect?
Jeff Layton - Aug. 28, 2009, 8:09 p.m.
On Fri, 28 Aug 2009 14:38:28 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Fri, Aug 28, 2009 at 2:26 PM, Jeff Layton<jlayton@redhat.com> wrote:
> > On Fri, 28 Aug 2009 14:14:43 -0500
> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> >
> >> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@redhat.com> 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 <jlayton@redhat.com>
> >> > ---
> >> >  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
> >> >
> >> > _______________________________________________
> >> > linux-cifs-client mailing list
> >> > linux-cifs-client@lists.samba.org
> >> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >> >
> >>
> >>
> >> Acked-by: Shirish Pargaonkar <shiishpargaonkar@yahoo.com>
> >>
> >> with a question, does what you say about pTcon->needs_reconnect  apply
> >> to oplock->cancel also?
> >
> > I don't think so. oplock->cancel doesn't get cleared on a reconnect.
> >
> > ...or did I misunderstand the question?
> >
> > Thanks,
> > --
> > Jeff Layton <jlayton@redhat.com>
> >
> 
> Jeff, I was thinking, if oplock->cancel is false but before call is
> sent out, reconnect happens
> and it is not necessary to send the call (oplock->cancel is true) and
> yet it could get sent,
> similar to what can happen in case of tcon->need_reconnect?

That is a possibility I suppose. It's sort of an unlikely race -- you'd
have to have to check the ->cancel flag, then mark the tcon for
reconnect and have it actually be reconnected and a filehandle opened
with the same netfid before the CIFSSMBLock call can go out.

In truth, this problem is always a possibility with handle-based calls
If we want to fix that then you really need to do so at a deeper
level than this. How can we always ensure that a call doesn't go out on
the wire that was only relevant for a different tcon, etc...

One possibility is to stamp each marshaled-up buffer with a serial
number of the socket/smbses/tcon. Then just before you send it, you'd
need to check and see whether that number has changed. That's a fairly
major change though, and I just don't see it happening in the context
of this patchset.

Patch

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