Ceph » CephFS

There is a bug in this routine:

static void __wake_requests(struct ceph_mds_client *mdsc,
                            struct list_head *head)
{
        struct ceph_mds_request *req, *nreq;

        list_for_each_entry_safe(req, nreq, head, r_wait) {
                list_del_init(&req->r_wait);
                __do_request(mdsc, req);
        }
}

The list_for_each_entry_safe() isn't the right routine to use here because the head is a pointer to the s_waiting part of a struct ceph_mds_session. However, the list macros assumes that head is the same pointer type as the first argument in this case a struct ceph_mds_request *. So the condition check &pos->member != (head) will NEVER be false if the list wasn't empty. This works for an empty list because even though pos is a bogus pointer &pos->member == head.

There is already a fix for this in wip-fs branch. I'm not clear if we need to be processing through the list once or keep trying because we are waiting for the session to open for example.

commit cd197cc5490a3110b9dfba26ee9d90a586a923af
Author: Yan, Zheng <zheng.z.yan@intel.com>
Date:   Mon Nov 19 10:49:06 2012 +0800

    ceph: Fix infinite loop in __wake_requests

    __wake_requests() will enter infinite loop if we use it to wake
    requests in the session->s_waiting list. __wake_requests() deletes
    requests from the list and __do_request() adds requests back to
    the list.

    Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
    Signed-off-by: Sage Weil <sage@inktank.com>

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 62d2342..9165eb8 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1876,9 +1876,14 @@ finish:
 static void __wake_requests(struct ceph_mds_client *mdsc,
                            struct list_head *head)
 {
-       struct ceph_mds_request *req, *nreq;
+       struct ceph_mds_request *req;
+       LIST_HEAD(tmp_list);
+
+       list_splice_init(head, &tmp_list);

-       list_for_each_entry_safe(req, nreq, head, r_wait) {
+       while (!list_empty(&tmp_list)) {
+               req = list_entry(tmp_list.next,
+                                struct ceph_mds_request, r_wait);
                list_del_init(&req->r_wait);
                __do_request(mdsc, req);
        }

Ah, nice catch! We just need to cycle through the list once to requeue the requests for the MDS. If Yan's patch looks good, go ahead and cherry-pick it into testing (and add your Tested-by or Reviewed-by, if you like). For that matter, if there are other patches in that branch you want to review, that's great too :).

I queued that branch for a qa run several days ago and it passed. It's not a particularly thorough test suite for mds behavior, though.. at least not yet.