[linux-cifs-client] [PATCH 5/5] cifs: cancel oplock release callbacks during reconnect
Jeff Layton
jlayton at redhat.com
Fri Aug 28 14:09:40 MDT 2009
On Fri, 28 Aug 2009 14:38:28 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
> On Fri, Aug 28, 2009 at 2:26 PM, Jeff Layton<jlayton at redhat.com> wrote:
> > On Fri, 28 Aug 2009 14:14:43 -0500
> > Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
> >
> >> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton at 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 at 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 at lists.samba.org
> >> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >> >
> >>
> >>
> >> Acked-by: Shirish Pargaonkar <shiishpargaonkar at 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 at 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.
--
Jeff Layton <jlayton at redhat.com>
More information about the linux-cifs-client
mailing list