On Tue, 14 Dec 2010 15:33:48 -0600
Steve French <smfrench@xxxxxxxxx> wrote:
> Why wouldn't we issue SMB NTCancel on these? That way we only have to
> wait until the timeout for the NTCancel (at worst) and can't leak midq
> entries.
>
I suppose we could, but...
a) windows doesn't do it
b) we don't have appropriate infrastructure for it
...sounds like a good follow-on project, but not something I really
want to tackle in this set.
> On Tue, Dec 14, 2010 at 2:40 PM, Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> > 2010/12/14 Jeff Layton <jlayton@xxxxxxxxxx>:
> >> On Tue, 14 Dec 2010 10:24:28 +0300
> >> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >>
> >>> 2010/12/10 Jeff Layton <jlayton@xxxxxxxxxx>:
> >>> > Currently, when a request is cancelled via signal, we delete the mid
> >>> > immediately. If the request was already transmitted however, the client
> >>> > is still likely to receive a response. When it does, it won't recognize
> >>> > it however and will pop a printk.
> >>> >
> >>> > It's also a little dangerous to just delete the mid entry like this. We
> >>> > may end up reusing that mid. If we do then we could potentially get the
> >>> > response from the first request confused with the later one.
> >>> >
> >>> > Prevent the reuse of mids by marking them as cancelled and keeping them
> >>> > on the pending_mid_q list. If the reply comes in, we'll delete it from
> >>> > the list then. If it never comes, then we'll delete it at reconnect
> >>> > or when cifsd comes down.
> >>> >
> >>> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >>> > ---
> >>> > fs/cifs/cifsglob.h | 2 +-
> >>> > fs/cifs/cifsproto.h | 1 +
> >>> > fs/cifs/connect.c | 44 +++++++++++++++++++++++++++++++++++++-------
> >>> > fs/cifs/transport.c | 30 ++++++++++++++++++++++--------
> >>> > 4 files changed, 61 insertions(+), 16 deletions(-)
> >>> >
> >>> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >>> > index cc43ada..fb75f04 100644
> >>> > --- a/fs/cifs/cifsglob.h
> >>> > +++ b/fs/cifs/cifsglob.h
> >>> > @@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
> >>> > #define MID_REQUEST_SUBMITTED 2
> >>> > #define MID_RESPONSE_RECEIVED 4
> >>> > #define MID_RETRY_NEEDED 8 /* session closed while this request out */
> >>> > -#define MID_NO_RESP_NEEDED 0x10
> >>> > +#define MID_REQUEST_CANCELLED 0x10 /* discard any reply */
> >>> >
> >>> > /* Types of response buffer returned from SendReceive2 */
> >>> > #define CIFS_NO_BUFFER 0 /* Response buffer not returned */
> >>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >>> > index fe77e69..a8fc606 100644
> >>> > --- a/fs/cifs/cifsproto.h
> >>> > +++ b/fs/cifs/cifsproto.h
> >>> > @@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
> >>> > const char *fullpath, const struct dfs_info3_param *ref,
> >>> > char **devname);
> >>> > /* extern void renew_parental_timestamps(struct dentry *direntry);*/
> >>> > +extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> >>> > extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
> >>> > struct smb_hdr * /* input */ ,
> >>> > struct smb_hdr * /* out */ ,
> >>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >>> > index 7e20ece..0feb592 100644
> >>> > --- a/fs/cifs/connect.c
> >>> > +++ b/fs/cifs/connect.c
> >>> > @@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>> > {
> >>> > int rc = 0;
> >>> > struct list_head *tmp, *tmp2;
> >>> > - struct list_head retry;
> >>> > + struct list_head retry, dispose;
> >>> > struct cifsSesInfo *ses;
> >>> > struct cifsTconInfo *tcon;
> >>> > struct mid_q_entry *mid_entry;
> >>> > @@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>> > */
> >>> > cFYI(1, "%s: moving mids to retry list", __func__);
> >>> > INIT_LIST_HEAD(&retry);
> >>> > + INIT_LIST_HEAD(&dispose);
> >>> > spin_lock(&GlobalMid_Lock);
> >>> > list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
> >>> > mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > if (mid_entry->midState == MID_REQUEST_SUBMITTED)
> >>> > list_move(tmp, &retry);
> >>> > + else if (mid_entry->midState == MID_REQUEST_CANCELLED)
> >>> > + list_move(tmp, &dispose);
> >>> > }
> >>> > spin_unlock(&GlobalMid_Lock);
> >>> >
> >>> > + /* now walk private dispose list and delete entries */
> >>> > + list_for_each_safe(tmp, tmp2, &dispose) {
> >>> > + mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > + DeleteMidQEntry(mid_entry);
> >>> > + }
> >>> > +
> >>> > while ((server->tcpStatus != CifsExiting) &&
> >>> > (server->tcpStatus != CifsGood)) {
> >>> > try_to_freeze();
> >>> > @@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>> > }
> >>> > }
> >>> >
> >>> > - /* now, issue callback for all mids in flight */
> >>> > + /* issue callback for all mids in flight */
> >>> > list_for_each_safe(tmp, tmp2, &retry) {
> >>> > list_del_init(tmp);
> >>> > mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > @@ -575,9 +584,13 @@ incomplete_rcv:
> >>> > list_for_each(tmp, &server->pending_mid_q) {
> >>> > mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> >
> >>> > - if ((mid_entry->mid == smb_buffer->Mid) &&
> >>> > - (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
> >>> > - (mid_entry->command == smb_buffer->Command)) {
> >>> > + if (mid_entry->mid != smb_buffer->Mid)
> >>> > + goto next_mid;
> >>> > + if (mid_entry->command != smb_buffer->Command)
> >>> > + goto next_mid;
> >>> > + if (mid_entry->midState == MID_REQUEST_CANCELLED)
> >>> > + break;
> >>> > + if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >>> > if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
> >>> > /* We have a multipart transact2 resp */
> >>> > isMultiRsp = true;
> >>> > @@ -623,11 +636,16 @@ multi_t2_fnd:
> >>> > server->lstrp = jiffies;
> >>> > break;
> >>> > }
> >>> > +next_mid:
> >>> > mid_entry = NULL;
> >>> > }
> >>> > spin_unlock(&GlobalMid_Lock);
> >>> >
> >>> > if (mid_entry != NULL) {
> >>> > + if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> >>> > + DeleteMidQEntry(mid_entry);
> >>> > + continue;
> >>> > + }
> >>> > mid_entry->callback(mid_entry);
> >>> > /* Was previous buf put in mpx struct for multi-rsp? */
> >>> > if (!isMultiRsp) {
> >>> > @@ -704,6 +722,9 @@ multi_t2_fnd:
> >>> > }
> >>> > spin_unlock(&cifs_tcp_ses_lock);
> >>> > } else {
> >>> > + struct mid_q_entry *tmp_mid;
> >>> > + struct list_head dispose;
> >>> > +
> >>> > /* although we can not zero the server struct pointer yet,
> >>> > since there are active requests which may depnd on them,
> >>> > mark the corresponding SMB sessions as exiting too */
> >>> > @@ -713,17 +734,26 @@ multi_t2_fnd:
> >>> > ses->status = CifsExiting;
> >>> > }
> >>> >
> >>> > + INIT_LIST_HEAD(&dispose);
> >>> > spin_lock(&GlobalMid_Lock);
> >>> > - list_for_each(tmp, &server->pending_mid_q) {
> >>> > - mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > + list_for_each_entry_safe(mid_entry, tmp_mid,
> >>> > + &server->pending_mid_q, qhead) {
> >>> > if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >>> > cFYI(1, "Clearing Mid 0x%x - issuing callback",
> >>> > mid_entry->mid);
> >>> > mid_entry->callback(mid_entry);
> >>> > + } else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> >>> > + cFYI(1, "Clearing Mid 0x%x - Cancelled",
> >>> > + mid_entry->mid);
> >>> > + list_move(&mid_entry->qhead, &dispose);
> >>>
> >>> Why do we need another list here? It seems to me that we can simply
> >>> delete cancelled mid.
> >>>
> >>
> >> Nope, we can't. DeleteMidQEntry will try to take the GlobalMid_Lock on
> >> its own, so you can't call it there without deadlocking. You also can't
> >> drop the spinlock and then call DeleteMidQEntry since that might mean
> >> that the list will change in the middle of walking it. We could add a
> >> "DeleteMidQEntry_locked", but dealing with locked and non-locked
> >> variants gets messy.
> >>
> >> Moving it to a private dispose list and then walking that list outside
> >> of the lock takes care of those problems. I added the "retry" list in
> >> this function for similar reasons.
> >>
> >>> > }
> >>> > }
> >>> > spin_unlock(&GlobalMid_Lock);
> >>> > spin_unlock(&cifs_tcp_ses_lock);
> >>> > +
> >>> > + /* now delete all of the cancelled mids */
> >>> > + list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
> >>> > + DeleteMidQEntry(mid_entry);
> >>> > /* 1/8th of sec is more than enough time for them to exit */
> >>> > msleep(125);
> >>> > }
> >>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >>> > index 79647db..97a1170 100644
> >>> > --- a/fs/cifs/transport.c
> >>> > +++ b/fs/cifs/transport.c
> >>> > @@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
> >>> > return temp;
> >>> > }
> >>> >
> >>> > -static void
> >>> > +void
> >>> > DeleteMidQEntry(struct mid_q_entry *midEntry)
> >>> > {
> >>> > #ifdef CONFIG_CIFS_STATS2
> >>> > @@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
> >>> > goto out;
> >>> >
> >>> > rc = wait_for_response(ses->server, midQ);
> >>> > - if (rc != 0)
> >>> > - goto out;
> >>> > + if (rc != 0) {
> >>> > + /* no longer considered to be "in-flight" */
> >>> > + midQ->midState = MID_REQUEST_CANCELLED;
> >>> > + atomic_dec(&ses->server->inFlight);
> >>> > + wake_up(&ses->server->request_q);
> >>> > + return rc;
> >>> > + }
> >>> >
> >>> > rc = handle_mid_result(midQ, ses->server);
> >>> > if (rc != 0)
> >>> > @@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
> >>> > goto out;
> >>> >
> >>> > rc = wait_for_response(ses->server, midQ);
> >>> > - if (rc != 0)
> >>> > - goto out;
> >>> > + if (rc != 0) {
> >>> > + /* no longer considered to be "in-flight" */
> >>> > + midQ->midState = MID_REQUEST_CANCELLED;
> >>> > + atomic_dec(&ses->server->inFlight);
> >>> > + wake_up(&ses->server->request_q);
> >>> > + return rc;
> >>> > + }
> >>> >
> >>> > rc = handle_mid_result(midQ, ses->server);
> >>> > if (rc != 0)
> >>> > @@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
> >>> > }
> >>> > }
> >>> >
> >>> > - if (wait_for_response(ses->server, midQ) == 0) {
> >>> > - /* We got the response - restart system call. */
> >>> > - rstart = 1;
> >>> > + rc = wait_for_response(ses->server, midQ);
> >>> > + if (rc) {
> >>> > + midQ->midState = MID_REQUEST_CANCELLED;
> >>> > + return rc;
> >>> > }
> >>> > +
> >>> > + /* We got the response - restart system call. */
> >>> > + rstart = 1;
> >>> > }
> >>> >
> >>> > rc = handle_mid_result(midQ, ses->server);
> >>> > --
> >>> > 1.7.3.2
> >>> >
> >>> > --
> >>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >>> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>> >
> >>>
> >>>
> >>>
> >>
> >>
> >> --
> >> Jeff Layton <jlayton@xxxxxxxxxx>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >
> > Ok. Adding DeleteMidQEntry without lock/unlock looks better to me, but
> > anyway it's ok.
> >
> > Reviewed-by: Pavel Shilovsky <piastryyy@xxxxxxxxx>
> >
> > --
> > Best regards,
> > Pavel Shilovsky.
> >
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
