| Message ID | 1250618822-6131-6-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 > > > > > >
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?
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,
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?
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.
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);
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(-)