The main fix is the first commit; the second commit adds a couple of null checks, though this may prevent hiding actual issues so happy to drop it.

@mchangir PTAL

@theanalyst the crash is most probably this issue #62684

@theanalyst the crash is most probably this issue #62684
@mchangir
That explains the locking issue, however if you see what happens when we find a new batch head currently:

    auto new_batch_head = it->second->find_new_head();
    if (!new_batch_head) {
      mdr->batch_op_map->erase(it);
    } 
    mds->finisher->queue(new C_MDS_RetryRequest(this, new_batch_head));
  }

if the new_batch_head is not found it defaults to null, and this is passed to Finisher. We see this happening quite a lot if you launch requests with find etc on a large tree where a single inode might have only a single batch request and then you pass nullptr to Finisher which crashes. So I believe passing a non null new_batch_head is needed. What do you think?

@theanalyst the crash is most probably this issue #62684
@mchangir
That explains the locking issue, however if you see what happens when we find a new batch head currently:

    auto new_batch_head = it->second->find_new_head();
    if (!new_batch_head) {
      mdr->batch_op_map->erase(it);
    } 
    mds->finisher->queue(new C_MDS_RetryRequest(this, new_batch_head));
  }

if the new_batch_head is not found it defaults to null, and this is passed to Finisher. We see this happening quite a lot if you launch requests with find etc on a large tree where a single inode might have only a single batch request and then you pass nullptr to Finisher which crashes. So I believe passing a non null new_batch_head is needed. What do you think?

@theanalyst would you be able to form a set of bash commands that mimic your workload ?
I'd like to run them and see the crash myself.

I've just seen this in the wild on a single active v18.2.7 fs. I don't have any more detail about the use-case that triggered it.

@mchangir I believe @theanalyst shared the reproducer elsewhere (via mail?). Could you please update the tracker with those details please. I will plan to have a look then.

From the codeflow, what I can understand, when a session gets evicted it tries to clear the requests,

while (!session->requests.empty()) {
    auto mdr = MDRequestRef(*session->requests.begin());
    mdcache->request_kill(mdr);
  }

This void MDCache::request_kill(MDRequestRef &mdr) function in turns is doing the request_cleanup

While following the comment,

void MDCache::request_kill(MDRequestRef &mdr) {
 // rollback peer requests is tricky. just let the request proceed.

mds wants to just proceed the request marking it as a killed request. But when it comes to batch_head request, either it should replaced by new batch head(which is not killed) request completely in the request_cleanup and let it proceed or we should remove this assert as we are allowing the killed batch head to proceed. So the comment, // Should already be reset in request_cleanup(). doesn't justify.

void Server::dispatch_client_request(MDRequestRef &mdr) {
 // we shouldn't be waiting on anyone.
 ceph_assert(!mdr->has_more() || mdr->more()->waiting_on_peer.empty());

 if (mdr->killed) {
   // Should already be reset in request_cleanup().
   ceph_assert(!mdr->is_batch_head());

@mchangir gentle nudge on this.

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

(unmarking stale)

I haven't been following the discussion here closely. @mchangir - Is this good to go? What about @sajibreadd comment #62630 (comment) ?

This PR is under test in https://tracker.ceph.com/issues/73693.