Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220331223857.902911-1-jakobkoschel@gmail.com>
In-Reply-To: <20220331223857.902911-1-jakobkoschel@gmail.com>
From:   Andreas Gruenbacher <agruenba@redhat.com>
Date:   Mon, 4 Apr 2022 16:58:53 +0200
Message-ID: <CAHc6FU4s952rPPsceefvZER8dPM9RVWyQrmneMyqaW_0oViFSA@mail.gmail.com>
Subject: Re: [PATCH 1/2] gfs2: remove usage of list iterator variable for list_for_each_entry_continue()
To:     Jakob Koschel <jakobkoschel@gmail.com>
Cc:     Bob Peterson <rpeterso@redhat.com>,
        cluster-devel <cluster-devel@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jakob,

On Fri, Apr 1, 2022 at 12:40 AM Jakob Koschel <jakobkoschel@gmail.com> wrote:
> In preparation to limiting the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to iterate through the list [1].
>
> Since that variable should not be used past the loop iteration, a
> separate variable is used to 'remember the current location within the
> loop'.
>
> To either continue iterating from that position or start a new
> iteration (if the previous iteration was complete) list_prepare_entry()
> is used.

I can see how accessing an iterator variable past a for_each_entry
loop will cause problems when it ends up pointing at the list head.
Here, the iterator variables are not accessed outside the loops at
all, though. So this patch is ugly, and it doesn't even help.

> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  fs/gfs2/lops.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 6ba51cbb94cf..74e6d05cee2c 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -653,7 +653,7 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
>                                 bool is_databuf)
>  {
>         struct gfs2_log_descriptor *ld;
> -       struct gfs2_bufdata *bd1 = NULL, *bd2;
> +       struct gfs2_bufdata *bd1 = NULL, *bd2, *tmp1, *tmp2;
>         struct page *page;
>         unsigned int num;
>         unsigned n;
> @@ -661,7 +661,7 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
>
>         gfs2_log_lock(sdp);
>         list_sort(NULL, blist, blocknr_cmp);
> -       bd1 = bd2 = list_prepare_entry(bd1, blist, bd_list);
> +       tmp1 = tmp2 = list_prepare_entry(bd1, blist, bd_list);

We should actually be using list_entry() here, not list_prepare_entry().

>         while(total) {
>                 num = total;
>                 if (total > limit)
> @@ -675,14 +675,18 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
>                 ptr = (__be64 *)(ld + 1);
>
>                 n = 0;
> +               bd1 = list_prepare_entry(tmp1, blist, bd_list);
> +               tmp1 = NULL;
>                 list_for_each_entry_continue(bd1, blist, bd_list) {
>                         *ptr++ = cpu_to_be64(bd1->bd_bh->b_blocknr);
>                         if (is_databuf) {
>                                 gfs2_check_magic(bd1->bd_bh);
>                                 *ptr++ = cpu_to_be64(buffer_escaped(bd1->bd_bh) ? 1 : 0);
>                         }
> -                       if (++n >= num)
> +                       if (++n >= num) {
> +                               tmp1 = bd1;
>                                 break;
> +                       }
>                 }
>
>                 gfs2_log_unlock(sdp);
> @@ -690,6 +694,8 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
>                 gfs2_log_lock(sdp);
>
>                 n = 0;
> +               bd2 = list_prepare_entry(tmp2, blist, bd_list);
> +               tmp2 = NULL;
>                 list_for_each_entry_continue(bd2, blist, bd_list) {
>                         get_bh(bd2->bd_bh);
>                         gfs2_log_unlock(sdp);
> @@ -712,8 +718,10 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
>                                 gfs2_log_write_bh(sdp, bd2->bd_bh);
>                         }
>                         gfs2_log_lock(sdp);
> -                       if (++n >= num)
> +                       if (++n >= num) {
> +                               tmp2 = bd2;
>                                 break;
> +                       }
>                 }
>
>                 BUG_ON(total < num);
>
> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275
> --
> 2.25.1
>

Thanks,
Andreas