LU-898 ptlrpc: fix ptlrpc request race.

Allow request reorder from export, only if request has been
added to the list.

Race condition with req error handle at ptlrpc_server_handle_req_in().
1. req is added to rq_export->exp_queued_rpc by ptlrpc_hpreq_init()
2. ptlrpc_server_request_add() returns error ( ost_validate_obdo() whith
one of this condition !(fid_seq_is_rsvd(oa->o_seq) ||
fid_seq_is_idif(oa->o_seq))
3. ptlrpc_server_drop_request(req) drops request (disconnect export),
but req is in rq_export->exp_queued_rpc
4. ldlm_server_blocking_ast handle rq_export->exp_queued_rpc and fail
because req->rq_export is NULL.
Fix allows request reorder from export only if request has been added to
the list. So workaround is not need for situation when request from export
was processed before ptlrpc_server_request_add.

Signed-off-by: Alexander Boyko <alexander_boyko@xyratex.com>
Reviewed-by: Vitaly Fertman <vitaly_fertman@xyratex.com>
Reviewed-by: Denis Kondratenko <denis_kondratenko@xyratex.com>
Reviewed-by: Andrew Perepechko <Andrew_Perepechko@xyratex.com>
Xyratex-bug-id: MRP-284
Change-Id: I5f763b3c4f19b6af5f803b50b43a5570dab3dc76
Reviewed-on: http://review.whamcloud.com/1799
Tested-by: Hudson
Tested-by: Maloo <whamcloud.maloo@gmail.com>
Reviewed-by: Andrew Perepechko <andrew_perepechko@xyratex.com>
Reviewed-by: Denis Kondratenko <Denis_Kondratenko@xyratex.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/ldlm/ldlm_lockd.c b/lustre/ldlm/ldlm_lockd.c
index 2be5aa9..55e10d8 100644 (file)
--- a/lustre/ldlm/ldlm_lockd.c
+++ b/lustre/ldlm/ldlm_lockd.c
@@ -706,7 +706,11 @@ static void ldlm_lock_reorder_req(struct ldlm_lock *lock)
         cfs_spin_lock_bh(&lock->l_export->exp_rpc_lock);
         cfs_list_for_each_entry(req, &lock->l_export->exp_queued_rpc,
                                 rq_exp_list) {
-                if (!req->rq_hp && req->rq_ops->hpreq_lock_match &&
+                /* Do not process requests that were not yet added to there
+                 * incoming queue or were already removed from there for
+                 * processing */
+                if (!req->rq_hp && !cfs_list_empty(&req->rq_list) &&
+                    req->rq_ops->hpreq_lock_match &&
                     req->rq_ops->hpreq_lock_match(req, lock))
                         ptlrpc_hpreq_reorder(req);
         }

diff --git a/lustre/ptlrpc/service.c b/lustre/ptlrpc/service.c
index 2fcffb5..dd702eb 100644 (file)
--- a/lustre/ptlrpc/service.c
+++ b/lustre/ptlrpc/service.c
@@ -1278,8 +1278,10 @@ void ptlrpc_hpreq_reorder(struct ptlrpc_request *req)
 
         cfs_spin_lock(&svc->srv_rq_lock);
         /* It may happen that the request is already taken for the processing
-         * but still in the export list, do not re-add it into the HP list. */
-        if (req->rq_phase == RQ_PHASE_NEW)
+         * but still in the export list, or the request is not in the request
+         * queue but in the export list already, do not add it into the
+         * HP list. */
+        if (!cfs_list_empty(&req->rq_list))
                 ptlrpc_hpreq_reorder_nolock(svc, req);
         cfs_spin_unlock(&svc->srv_rq_lock);
         EXIT;
@@ -1314,16 +1316,13 @@ static int ptlrpc_server_request_add(struct ptlrpc_service *svc,
                 RETURN(rc);
 
         cfs_spin_lock(&svc->srv_rq_lock);
-        /* Before inserting the request into the queue, check if it is not
-         * inserted yet, or even already handled -- it may happen due to
-         * a racing ldlm_server_blocking_ast(). */
-        if (req->rq_phase == RQ_PHASE_NEW && cfs_list_empty(&req->rq_list)) {
-                if (rc)
-                        ptlrpc_hpreq_reorder_nolock(svc, req);
-                else
-                        cfs_list_add_tail(&req->rq_list,
-                                          &svc->srv_request_queue);
-        }
+
+        if (rc)
+                ptlrpc_hpreq_reorder_nolock(svc, req);
+        else
+                cfs_list_add_tail(&req->rq_list,
+                                  &svc->srv_request_queue);
+
         cfs_spin_unlock(&svc->srv_rq_lock);
 
         RETURN(0);
@@ -1454,11 +1453,6 @@ ptlrpc_server_handle_req_in(struct ptlrpc_service *svc)
         svc->srv_n_queued_reqs--;
         /* Consider this still a "queued" request as far as stats are
            concerned */
-        /* ptlrpc_hpreq_init() inserts it to the export list and by the time
-         * of ptlrpc_server_request_add() it could be already handled and
-         * released. To not lose request in between, take an extra reference
-         * on the request. */
-        ptlrpc_request_addref(req);
         cfs_spin_unlock(&svc->srv_lock);
 
         /* go through security check/transform */
@@ -1566,14 +1560,14 @@ ptlrpc_server_handle_req_in(struct ptlrpc_service *svc)
 
         /* Move it over to the request processing queue */
         rc = ptlrpc_server_request_add(svc, req);
-        if (rc)
+        if (rc) {
+                ptlrpc_hpreq_fini(req);
                 GOTO(err_req, rc);
+        }
         cfs_waitq_signal(&svc->srv_waitq);
-        ptlrpc_server_drop_request(req);
         RETURN(1);
 
 err_req:
-        ptlrpc_server_drop_request(req);
         cfs_spin_lock(&svc->srv_rq_lock);
         svc->srv_n_active_reqs++;
         cfs_spin_unlock(&svc->srv_rq_lock);
@@ -1638,11 +1632,9 @@ ptlrpc_server_handle_request(struct ptlrpc_service *svc,
         if (request->rq_hp)
                 svc->srv_n_active_hpreq++;
 
-        /* The phase is changed under the lock here because we need to know
-         * the request is under processing (see ptlrpc_hpreq_reorder()). */
-        ptlrpc_rqphase_move(request, RQ_PHASE_INTERPRET);
         cfs_spin_unlock(&svc->srv_rq_lock);
 
+        ptlrpc_rqphase_move(request, RQ_PHASE_INTERPRET);
         ptlrpc_hpreq_fini(request);
 
         if(OBD_FAIL_CHECK(OBD_FAIL_PTLRPC_DUMP_LOG))