I understand why this works but it is weird not to select a new batch head in ::request_cleanup. I would have preferred that but I think this PR needs to be merged urgently. Maybe in a follow-up cleanup?

Does it really need ?

 2079 void Server::respond_to_request(const MDRequestRef& mdr, int r)
 2080 {
 2081   mdr->result = r;
 2082   if (mdr->client_request) {
 2083     if (mdr->is_batch_head()) {
 2084       dout(20) << __func__ << ": batch head " << *mdr << dendl;
 2085       mdr->release_batch_op()->respond(r);                                                                                                                                                                                                              
 2086     } else {
 2087      reply_client_request(mdr, make_message<MClientReply>(*mdr->client_request, r));
 2088     }
 2089   } else if (mdr->internal_op > -1) { 
 2090     dout(10) << __func__ << ": completing with result " << cpp_strerror(r) << " on internal " << *mdr << dendl;
 2091     auto c = mdr->internal_op_finish;   
 2092     if (!c)
 2093       ceph_abort_msg("trying to respond to internal op without finisher");
 2094     mdcache->request_finish(mdr);
 2095     c->complete(r);
 2096   }
 2097 }

We can see in Line#2085 it will cleanup all the batch requests when the batch head finishes.

Let me have a check carefully and try to improve the code.

@batrick please confirm that the if (dead) check only applies to internal requests, and can be moved after the client and peer conditional dispatches

We can see in Line#2085 it will cleanup all the batch requests when the batch head finishes.

In this case, Server::respond_to_request is not called because the request was killed (that's why dead == true). This logic for batch_head handling in ::respond_to_request and now MDCache::dispatch_request should be uniformly handled in a single location: MDCache::request_cleanup.

right, this is what @lxbsz had also figured out. So my question then is, shouldn't we just move that new if(dead) check into the else?

No, I think we need to make a better effort unify handling of requests in the MDS. A dead request should not be dispatched no matter its type.

@batrick But in the case when the sessions are killed and then all the corresponding requests will be marked killed too. Is that fine if we just remove the queued requests from the finisher queue directly ? If not then we cannot avoid this IMO, because the killed requests maybe still in the finisher queue and not dispatched yet.

But we can just choose a batch head directly when the a batch head request is being killed instead of doing this when the request is being dispatched.

@batrick But in the case when the sessions are killed and then all the corresponding requests will be marked killed too. Is that fine if we just remove the queued requests from the finisher queue directly ? If not then we cannot avoid this IMO, because the killed requests maybe still in the finisher queue and not dispatched yet.

I understand. I am probably being confusing. It's okay that a dead request is re-dispatched through MDCache::dispatch_request but if it's marked dead, do nothing else and return;.

But we can just choose a batch head directly when the a batch head request is being killed instead of doing this when the request is being dispatched.

Yes!

Let me try to improve it. Thanks @batrick @leonid-s-usov