All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ilya Dryomov <idryomov@gmail.com>
To: Hannes Reinecke <hare@suse.de>
Cc: Sage Weil <sage@redhat.com>, Daniel Disseldorp <ddiss@suse.com>,
	Jens Axboe <axboe@kernel.dk>,
	Ceph Development <ceph-devel@vger.kernel.org>,
	linux-block <linux-block@vger.kernel.org>
Subject: Re: [PATCH 07/15] rbd: use callback for image request completion
Date: Mon, 3 Feb 2020 18:13:59 +0100	[thread overview]
Message-ID: <CAOi1vP-Fn=EM7Bw57snxcf=YDw0Dft=fSSGcG7GX8q9iJnSeiQ@mail.gmail.com> (raw)
In-Reply-To: <20200131103739.136098-8-hare@suse.de>

On Fri, Jan 31, 2020 at 11:38 AM Hannes Reinecke <hare@suse.de> wrote:
>
> Using callbacks to simplify code and to separate out the different
> code paths for parent and child requests.
>
> Suggested-by: David Disseldorp <ddiss@suse.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/block/rbd.c | 61 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c31507a5fdd2..8cfd9407cbb8 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -317,6 +317,9 @@ struct rbd_obj_request {
>         struct kref             kref;
>  };
>
> +typedef void (*rbd_img_request_cb_t)(struct rbd_img_request *img_request,
> +                                    int result);
> +
>  enum img_req_flags {
>         IMG_REQ_CHILD,          /* initiator: block = 0, child image = 1 */
>         IMG_REQ_LAYERED,        /* ENOENT handling: normal = 0, layered = 1 */
> @@ -339,11 +342,8 @@ struct rbd_img_request {
>                 u64                     snap_id;        /* for reads */
>                 struct ceph_snap_context *snapc;        /* for writes */
>         };
> -       union {
> -               struct request          *rq;            /* block request */
> -               struct rbd_obj_request  *obj_request;   /* obj req initiator */
> -       };
> -
> +       void                    *callback_data;
> +       rbd_img_request_cb_t    callback;
>         struct list_head        lock_item;
>         struct list_head        object_extents; /* obj_req.ex structs */
>         struct mutex            object_mutex;
> @@ -506,6 +506,8 @@ static ssize_t add_single_major_store(struct bus_type *bus, const char *buf,
>  static ssize_t remove_single_major_store(struct bus_type *bus, const char *buf,
>                                          size_t count);
>  static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth);
> +static void rbd_img_end_child_request(struct rbd_img_request *img_req,
> +                                     int result);
>
>  static int rbd_dev_id_to_minor(int dev_id)
>  {
> @@ -2882,7 +2884,8 @@ static int rbd_obj_read_from_parent(struct rbd_obj_request *obj_req)
>                 return -ENOMEM;
>
>         __set_bit(IMG_REQ_CHILD, &child_img_req->flags);
> -       child_img_req->obj_request = obj_req;
> +       child_img_req->callback = rbd_img_end_child_request;
> +       child_img_req->callback_data = obj_req;
>
>         dout("%s child_img_req %p for obj_req %p\n", __func__, child_img_req,
>              obj_req);
> @@ -3506,14 +3509,12 @@ static bool __rbd_obj_handle_request(struct rbd_obj_request *obj_req,
>         return done;
>  }
>
> -/*
> - * This is open-coded in rbd_img_handle_request() to avoid parent chain
> - * recursion.
> - */
>  static void rbd_obj_handle_request(struct rbd_obj_request *obj_req, int result)
>  {
> -       if (__rbd_obj_handle_request(obj_req, &result))
> +       if (__rbd_obj_handle_request(obj_req, &result)) {
> +               /* Recurse into parent */
>                 rbd_img_handle_request(obj_req->img_request, result);
> +       }
>  }
>
>  static bool need_exclusive_lock(struct rbd_img_request *img_req)
> @@ -3695,26 +3696,29 @@ static bool __rbd_img_handle_request(struct rbd_img_request *img_req,
>         return done;
>  }
>
> -static void rbd_img_handle_request(struct rbd_img_request *img_req, int result)
> +static void rbd_img_end_child_request(struct rbd_img_request *img_req,
> +                                     int result)
>  {
> -again:
> -       if (!__rbd_img_handle_request(img_req, &result))
> -               return;
> +       struct rbd_obj_request *obj_req = img_req->callback_data;
>
> -       if (test_bit(IMG_REQ_CHILD, &img_req->flags)) {
> -               struct rbd_obj_request *obj_req = img_req->obj_request;
> +       rbd_img_request_put(img_req);
> +       rbd_obj_handle_request(obj_req, result);
> +}
>
> -               rbd_img_request_put(img_req);
> -               if (__rbd_obj_handle_request(obj_req, &result)) {
> -                       img_req = obj_req->img_request;
> -                       goto again;
> -               }
> -       } else {
> -               struct request *rq = img_req->rq;
> +static void rbd_img_end_request(struct rbd_img_request *img_req, int result)
> +{
> +       struct request *rq = img_req->callback_data;
>
> -               rbd_img_request_put(img_req);
> -               blk_mq_end_request(rq, errno_to_blk_status(result));
> -       }
> +       rbd_img_request_put(img_req);
> +       blk_mq_end_request(rq, errno_to_blk_status(result));
> +}
> +
> +void rbd_img_handle_request(struct rbd_img_request *img_req, int result)
> +{
> +       if (!__rbd_img_handle_request(img_req, &result))
> +               return;
> +
> +       img_req->callback(img_req, result);
>  }
>
>  static const struct rbd_client_id rbd_empty_cid;
> @@ -4840,7 +4844,8 @@ static void rbd_queue_workfn(struct work_struct *work)
>                 result = -ENOMEM;
>                 goto err_rq;
>         }
> -       img_request->rq = rq;
> +       img_request->callback = rbd_img_end_request;
> +       img_request->callback_data = rq;
>         snapc = NULL; /* img_request consumes a ref */
>
>         dout("%s rbd_dev %p img_req %p %s %llu~%llu\n", __func__, rbd_dev,

We walked away from callbacks to avoid recursion described here:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6d69bb536bac0d403d83db1ca841444981b280cd

The title says "on rbd map", but it was on the I/O path as well.
The functions have changed, but the gist is the same: some object
request completes, which completes its image request, which completes
the object request in the child image, which completes the image
request in the child image, etc.

The plan is to get rid of RBD_MAX_PARENT_CHAIN_LEN after refactoring
header read-in code.  If rbd_img_handle_request() is confusing, let's
add a comment.  There already is one (removed in this patch), but it
is far away, so I can see how that logic may seem over-complicated.

Thanks,

                Ilya

  reply	other threads:[~2020-02-03 17:13 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-31 10:37 [PATCH 00/15] rbd: switch to blk-mq Hannes Reinecke
2020-01-31 10:37 ` [PATCH 01/15] rbd: lock object request list Hannes Reinecke
2020-02-03 16:38   ` Ilya Dryomov
2020-01-31 10:37 ` [PATCH 02/15] rbd: use READ_ONCE() when checking the mapping size Hannes Reinecke
2020-02-03 16:50   ` Ilya Dryomov
2020-02-04  7:05     ` Hannes Reinecke
2020-01-31 10:37 ` [PATCH 03/15] rbd: reorder rbd_img_advance() Hannes Reinecke
2020-01-31 10:37 ` [PATCH 04/15] rbd: reorder switch statement in rbd_advance_read() Hannes Reinecke
2020-01-31 10:37 ` [PATCH 05/15] rbd: reorder switch statement in rbd_advance_write() Hannes Reinecke
2020-01-31 10:37 ` [PATCH 06/15] rbd: add 'done' state for rbd_obj_advance_copyup() Hannes Reinecke
2020-01-31 10:37 ` [PATCH 07/15] rbd: use callback for image request completion Hannes Reinecke
2020-02-03 17:13   ` Ilya Dryomov [this message]
2020-01-31 10:37 ` [PATCH 08/15] rbd: add debugging statements for the state machine Hannes Reinecke
2020-01-31 10:37 ` [PATCH 09/15] rbd: count pending object requests in-line Hannes Reinecke
2020-02-03 17:47   ` Ilya Dryomov
2020-02-04  6:59     ` Hannes Reinecke
2020-01-31 10:37 ` [PATCH 10/15] rbd: kill 'work_result' Hannes Reinecke
2020-01-31 10:37 ` [PATCH 11/15] rbd: drop state_mutex in __rbd_img_handle_request() Hannes Reinecke
2020-02-03 18:01   ` Ilya Dryomov
2020-01-31 10:37 ` [PATCH 12/15] rbd: kill img_request kref Hannes Reinecke
2020-01-31 10:37 ` [PATCH 13/15] rbd: schedule image_request after preparation Hannes Reinecke
2020-02-03 18:40   ` Ilya Dryomov
2020-01-31 10:37 ` [PATCH 14/15] rbd: embed image request as blk_mq request payload Hannes Reinecke
2020-01-31 10:37 ` [PATCH 15/15] rbd: switch to blk-mq Hannes Reinecke
2020-02-03  8:36   ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOi1vP-Fn=EM7Bw57snxcf=YDw0Dft=fSSGcG7GX8q9iJnSeiQ@mail.gmail.com' \
    --to=idryomov@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=ceph-devel@vger.kernel.org \
    --cc=ddiss@suse.com \
    --cc=hare@suse.de \
    --cc=linux-block@vger.kernel.org \
    --cc=sage@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.