[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ceph-devel
Subject:    Re: [PATCH 08/11] rbd: bio_chain_clone() cleanups
From:       Yehuda Sadeh <yehuda () inktank ! com>
Date:       2012-08-30 17:40:08
Message-ID: CAC-hyiGDLCgpS43d6wYbCq-bGBd0OndPVTVWaMN9EY=7muSUGg () mail ! gmail ! com
[Download RAW message or body]

Reviewed-by: Yehuda Sadeh <yehuda@inktank.com>

we still need to fix the bio_pair leak though

On Fri, Aug 24, 2012 at 9:35 AM, Alex Elder <elder@inktank.com> wrote:
> In bio_chain_clone(), at the end of the function the bi_next field
> of the tail of the new bio chain is nulled.  This isn't necessary,
> because if "tail" is non-null, its value will be the last bio
> structure allocated at the top of the while loop in that function.
> And before that structure is added to the end of the new chain, its
> bi_next pointer is always made null.
>
> While touching that function, clean a few other things:
>     - define each local variable on its own line
>     - move the definition of "tmp" to an inner scope
>     - move the modification of gfpmask closer to where it's used
>     - rearrange the logic that sets the chain's tail pointer
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  drivers/block/rbd.c |   21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index e94336d..bb8dad7 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -741,7 +741,9 @@ static struct bio *bio_chain_clone(struct bio **old,
> struct bio **next,
>                                    struct bio_pair **bp,
>                                    int len, gfp_t gfpmask)
>  {
> -       struct bio *tmp, *old_chain = *old, *new_chain = NULL, *tail = NULL;
> +       struct bio *old_chain = *old;
> +       struct bio *new_chain = NULL;
> +       struct bio *tail;
>         int total = 0;
>
>         if (*bp) {
> @@ -750,9 +752,12 @@ static struct bio *bio_chain_clone(struct bio
> **old, struct bio **next,
>         }
>
>         while (old_chain && (total < len)) {
> +               struct bio *tmp;
> +
>                 tmp = bio_kmalloc(gfpmask, old_chain->bi_max_vecs);
>                 if (!tmp)
>                         goto err_out;
> +               gfpmask &= ~__GFP_WAIT; /* can't wait after the first */
>
>                 if (total + old_chain->bi_size > len) {
>                         struct bio_pair *bp;
> @@ -780,15 +785,12 @@ static struct bio *bio_chain_clone(struct bio
> **old, struct bio **next,
>                 }
>
>                 tmp->bi_bdev = NULL;
> -               gfpmask &= ~__GFP_WAIT;
>                 tmp->bi_next = NULL;
> -
> -               if (!new_chain) {
> -                       new_chain = tail = tmp;
> -               } else {
> +               if (new_chain)
>                         tail->bi_next = tmp;
> -                       tail = tmp;
> -               }
> +               else
> +                       new_chain = tmp;
> +               tail = tmp;
>                 old_chain = old_chain->bi_next;
>
>                 total += tmp->bi_size;
> @@ -796,9 +798,6 @@ static struct bio *bio_chain_clone(struct bio **old,
> struct bio **next,
>
>         BUG_ON(total < len);
>
> -       if (tail)
> -               tail->bi_next = NULL;
> -
>         *old = old_chain;
>
>         return new_chain;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]