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.
--
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
