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.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
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);
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
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
fs/gfs2/quota.c | 13 ++++++-------
fs/gfs2/recovery.c | 22 ++++++++++------------
2 files changed, 16 insertions(+), 19 deletions(-)
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index be0997e24d60..dafd04fb9164 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -443,7 +443,7 @@ static int qd_check_sync(struct gfs2_sbd *sdp, struct gfs2_quota_data *qd,
static int qd_fish(struct gfs2_sbd *sdp, struct gfs2_quota_data **qdp)
{
- struct gfs2_quota_data *qd = NULL;
+ struct gfs2_quota_data *qd = NULL, *iter;
int error;
int found = 0;
@@ -454,15 +454,14 @@ static int qd_fish(struct gfs2_sbd *sdp, struct gfs2_quota_data **qdp)
spin_lock(&qd_lock);
- list_for_each_entry(qd, &sdp->sd_quota_list, qd_list) {
- found = qd_check_sync(sdp, qd, &sdp->sd_quota_sync_gen);
- if (found)
+ list_for_each_entry(iter, &sdp->sd_quota_list, qd_list) {
+ found = qd_check_sync(sdp, iter, &sdp->sd_quota_sync_gen);
+ if (found) {
+ qd = iter;
break;
+ }
}
- if (!found)
- qd = NULL;
-
spin_unlock(&qd_lock);
if (qd) {
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 016ed1b2ca1d..2bb085a72e8e 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -55,17 +55,16 @@ int gfs2_replay_read_block(struct gfs2_jdesc *jd, unsigned int blk,
int gfs2_revoke_add(struct gfs2_jdesc *jd, u64 blkno, unsigned int where)
{
struct list_head *head = &jd->jd_revoke_list;
- struct gfs2_revoke_replay *rr;
- int found = 0;
+ struct gfs2_revoke_replay *rr = NULL, *iter;
- list_for_each_entry(rr, head, rr_list) {
- if (rr->rr_blkno == blkno) {
- found = 1;
+ list_for_each_entry(iter, head, rr_list) {
+ if (iter->rr_blkno == blkno) {
+ rr = iter;
break;
}
}
- if (found) {
+ if (rr) {
rr->rr_where = where;
return 0;
}
@@ -83,18 +82,17 @@ int gfs2_revoke_add(struct gfs2_jdesc *jd, u64 blkno, unsigned int where)
int gfs2_revoke_check(struct gfs2_jdesc *jd, u64 blkno, unsigned int where)
{
- struct gfs2_revoke_replay *rr;
+ struct gfs2_revoke_replay *rr = NULL, *iter;
int wrap, a, b, revoke;
- int found = 0;
- list_for_each_entry(rr, &jd->jd_revoke_list, rr_list) {
- if (rr->rr_blkno == blkno) {
- found = 1;
+ list_for_each_entry(iter, &jd->jd_revoke_list, rr_list) {
+ if (iter->rr_blkno == blkno) {
+ rr = iter;
break;
}
}
- if (!found)
+ if (!rr)
return 0;
wrap = (rr->rr_where < jd->jd_replay_tail);
--
2.25.1
Hi Jakob,
On Fri, Apr 1, 2022 at 12:40 AM Jakob Koschel <[email protected]> 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 <[email protected]>
> ---
> 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
Jakob,
On Fri, Apr 1, 2022 at 12:40 AM Jakob Koschel <[email protected]> wrote:
> To move the list iterator variable into the list_for_each_entry_*()
> macro in the future it should be avoided to use the list iterator
> variable after the loop body.
>
> To *never* use the list iterator variable after the loop it was
> concluded to use a separate iterator variable instead of a
> found boolean [1].
>
> This removes the need to use a found variable and simply checking if
> the variable was set, can determine if the break/goto was hit.
>
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> fs/gfs2/quota.c | 13 ++++++-------
> fs/gfs2/recovery.c | 22 ++++++++++------------
> 2 files changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index be0997e24d60..dafd04fb9164 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -443,7 +443,7 @@ static int qd_check_sync(struct gfs2_sbd *sdp, struct gfs2_quota_data *qd,
>
> static int qd_fish(struct gfs2_sbd *sdp, struct gfs2_quota_data **qdp)
> {
> - struct gfs2_quota_data *qd = NULL;
> + struct gfs2_quota_data *qd = NULL, *iter;
> int error;
> int found = 0;
>
> @@ -454,15 +454,14 @@ static int qd_fish(struct gfs2_sbd *sdp, struct gfs2_quota_data **qdp)
>
> spin_lock(&qd_lock);
>
> - list_for_each_entry(qd, &sdp->sd_quota_list, qd_list) {
> - found = qd_check_sync(sdp, qd, &sdp->sd_quota_sync_gen);
> - if (found)
> + list_for_each_entry(iter, &sdp->sd_quota_list, qd_list) {
> + found = qd_check_sync(sdp, iter, &sdp->sd_quota_sync_gen);
> + if (found) {
> + qd = iter;
we might as well get rid of 'found' here like in the below two
changes. Let me fix that up.
> break;
> + }
> }
>
> - if (!found)
> - qd = NULL;
> -
> spin_unlock(&qd_lock);
>
> if (qd) {
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 016ed1b2ca1d..2bb085a72e8e 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -55,17 +55,16 @@ int gfs2_replay_read_block(struct gfs2_jdesc *jd, unsigned int blk,
> int gfs2_revoke_add(struct gfs2_jdesc *jd, u64 blkno, unsigned int where)
> {
> struct list_head *head = &jd->jd_revoke_list;
> - struct gfs2_revoke_replay *rr;
> - int found = 0;
> + struct gfs2_revoke_replay *rr = NULL, *iter;
>
> - list_for_each_entry(rr, head, rr_list) {
> - if (rr->rr_blkno == blkno) {
> - found = 1;
> + list_for_each_entry(iter, head, rr_list) {
> + if (iter->rr_blkno == blkno) {
> + rr = iter;
> break;
> }
> }
>
> - if (found) {
> + if (rr) {
> rr->rr_where = where;
> return 0;
> }
> @@ -83,18 +82,17 @@ int gfs2_revoke_add(struct gfs2_jdesc *jd, u64 blkno, unsigned int where)
>
> int gfs2_revoke_check(struct gfs2_jdesc *jd, u64 blkno, unsigned int where)
> {
> - struct gfs2_revoke_replay *rr;
> + struct gfs2_revoke_replay *rr = NULL, *iter;
> int wrap, a, b, revoke;
> - int found = 0;
>
> - list_for_each_entry(rr, &jd->jd_revoke_list, rr_list) {
> - if (rr->rr_blkno == blkno) {
> - found = 1;
> + list_for_each_entry(iter, &jd->jd_revoke_list, rr_list) {
> + if (iter->rr_blkno == blkno) {
> + rr = iter;
> break;
> }
> }
>
> - if (!found)
> + if (!rr)
> return 0;
>
> wrap = (rr->rr_where < jd->jd_replay_tail);
> --
> 2.25.1
>
Thanks,
Andreas
> On 4. Apr 2022, at 16:58, Andreas Gruenbacher <[email protected]> wrote:
>
> Hi Jakob,
>
> On Fri, Apr 1, 2022 at 12:40 AM Jakob Koschel <[email protected]> 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.
Well, as long as you only use it to access the list head, there
should be no problem, hence no bug.
The issue are more the cases that use other members of that 'bogus'
pointer and there were plenty of such cases [1].
That's why the goal is to "not use the iterator variable after the loop"
so the scope can be lowered and such cases are avoided by construction.
> Here, the iterator variables are not accessed outside the loops at
> all, though. So this patch is ugly, and it doesn't even help.
I do agree that this patch is ugly. I'm open to suggestions on how to
improve the code allowing to lower the scope of 'bd1' to the traversal
loop. But I don't agree that the iterator variables are not used outside
of the loops. (If with loops you mean the list traversals).
The iterator variables are not used "after" in terms of code location but
since it's wrapped by a while loop they are used "after" in regards of
execution time.
From the second iteration of the while loop onwards, it will used the
leftover 'bd1' pointer from the previous iterations list traversal, hence
using the variables "outside of the traversal loops". Lowering the scope
will not be possible because of this.
>
>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <[email protected]>
>> ---
>> 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().
I'm not sure if you are referring to using list_entry() here in the original or
in the changed version.
I don't see how that would be much better in either cases. 'bd1' can be NULL in both cases
which would break when simply using list_entry(). In the original code, 'bd1' would
be NULL for the first iteration of the while loop and in the updated version it
would be NULL in the first iteration + every time the list traversal in the previous
iteration did not break early.
Just using 'bd1 = list_entry(bd1, blist, bd_list);' when initializing would work
in the original code.
But it's not solving the scope issue where 'bd1' is used outside of the list
traversal loop.
>
[1] https://lore.kernel.org/linux-kernel/[email protected]/
Thanks,
Jakob