Thread (6 messages) 6 messages, 3 authors, 2020-01-31

Re: [PATCH] rbd: lock object request list

From: Ilya Dryomov <idryomov@gmail.com>
Date: 2020-01-30 15:09:19
Also in: ceph-devel

On Thu, Jan 30, 2020 at 12:43 PM Hannes Reinecke [off-list ref] wrote:
quoted hunk ↗ jump to hunk
The object request list can be accessed from various contexts
so we need to lock it to avoid concurrent modifications and
random crashes.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/block/rbd.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 5710b2a8609c..ddc170661607 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -344,6 +344,7 @@ struct rbd_img_request {

        struct list_head        lock_item;
        struct list_head        object_extents; /* obj_req.ex structs */
+       struct mutex            object_mutex;

        struct mutex            state_mutex;
        struct pending_result   pending;
@@ -1664,6 +1665,7 @@ static struct rbd_img_request *rbd_img_request_create(
        INIT_LIST_HEAD(&img_request->lock_item);
        INIT_LIST_HEAD(&img_request->object_extents);
        mutex_init(&img_request->state_mutex);
+       mutex_init(&img_request->object_mutex);
        kref_init(&img_request->kref);

        return img_request;
@@ -1680,8 +1682,10 @@ static void rbd_img_request_destroy(struct kref *kref)
        dout("%s: img %p\n", __func__, img_request);

        WARN_ON(!list_empty(&img_request->lock_item));
+       mutex_lock(&img_request->object_mutex);
        for_each_obj_request_safe(img_request, obj_request, next_obj_request)
                rbd_img_obj_request_del(img_request, obj_request);
+       mutex_unlock(&img_request->object_mutex);

        if (img_request_layered_test(img_request)) {
                img_request_layered_clear(img_request);
@@ -2486,6 +2490,7 @@ static int __rbd_img_fill_request(struct rbd_img_request *img_req)
        struct rbd_obj_request *obj_req, *next_obj_req;
        int ret;

+       mutex_lock(&img_req->object_mutex);
        for_each_obj_request_safe(img_req, obj_req, next_obj_req) {
                switch (img_req->op_type) {
                case OBJ_OP_READ:
@@ -2510,7 +2515,7 @@ static int __rbd_img_fill_request(struct rbd_img_request *img_req)
                        continue;
                }
        }
-
+       mutex_unlock(&img_req->object_mutex);
        img_req->state = RBD_IMG_START;
        return 0;
 }
@@ -2569,6 +2574,7 @@ static int rbd_img_fill_request_nocopy(struct rbd_img_request *img_req,
         * position in the provided bio (list) or bio_vec array.
         */
        fctx->iter = *fctx->pos;
+       mutex_lock(&img_req->object_mutex);
        for (i = 0; i < num_img_extents; i++) {
                ret = ceph_file_to_extents(&img_req->rbd_dev->layout,
                                           img_extents[i].fe_off,
@@ -2576,10 +2582,12 @@ static int rbd_img_fill_request_nocopy(struct rbd_img_request *img_req,
                                           &img_req->object_extents,
                                           alloc_object_extent, img_req,
                                           fctx->set_pos_fn, &fctx->iter);
-               if (ret)
+               if (ret) {
+                       mutex_unlock(&img_req->object_mutex);
                        return ret;
+               }
        }
-
+       mutex_unlock(&img_req->object_mutex);
        return __rbd_img_fill_request(img_req);
 }
@@ -2620,6 +2628,7 @@ static int rbd_img_fill_request(struct rbd_img_request *img_req,
         * or bio_vec array because when mapped, those bio_vecs can straddle
         * stripe unit boundaries.
         */
+       mutex_lock(&img_req->object_mutex);
        fctx->iter = *fctx->pos;
        for (i = 0; i < num_img_extents; i++) {
                ret = ceph_file_to_extents(&rbd_dev->layout,
@@ -2629,15 +2638,17 @@ static int rbd_img_fill_request(struct rbd_img_request *img_req,
                                           alloc_object_extent, img_req,
                                           fctx->count_fn, &fctx->iter);
                if (ret)
-                       return ret;
+                       goto out_unlock;
        }

        for_each_obj_request(img_req, obj_req) {
                obj_req->bvec_pos.bvecs = kmalloc_array(obj_req->bvec_count,
                                              sizeof(*obj_req->bvec_pos.bvecs),
                                              GFP_NOIO);
-               if (!obj_req->bvec_pos.bvecs)
-                       return -ENOMEM;
+               if (!obj_req->bvec_pos.bvecs) {
+                       ret = -ENOMEM;
+                       goto out_unlock;
+               }
        }

        /*
@@ -2652,10 +2663,14 @@ static int rbd_img_fill_request(struct rbd_img_request *img_req,
                                           &img_req->object_extents,
                                           fctx->copy_fn, &fctx->iter);
                if (ret)
-                       return ret;
+                       goto out_unlock;
        }
+       mutex_unlock(&img_req->object_mutex);

        return __rbd_img_fill_request(img_req);
+out_unlock:
+       mutex_unlock(&img_req->object_mutex);
+       return ret;
 }

 static int rbd_img_fill_nodata(struct rbd_img_request *img_req,
@@ -3552,6 +3567,7 @@ static void rbd_img_object_requests(struct rbd_img_request *img_req)

        rbd_assert(!img_req->pending.result && !img_req->pending.num_pending);

+       mutex_lock(&img_req->object_mutex);
        for_each_obj_request(img_req, obj_req) {
                int result = 0;
@@ -3564,6 +3580,7 @@ static void rbd_img_object_requests(struct rbd_img_request *img_req)
                        img_req->pending.num_pending++;
                }
        }
+       mutex_unlock(&img_req->object_mutex);
 }

 static bool rbd_img_advance(struct rbd_img_request *img_req, int *result)
Hi Hannes,

I'm afraid I don't immediately see the issue and the commit
message is very light on details.  Can you elaborate on what
concurrent modifications are possible?  An example of a crash?

Thanks,

                Ilya
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help