[Cluster-devel] [bug report] gfs2: Introduce flag for glock holder auto-demotion

[Cluster-devel] [bug report] gfs2: Introduce flag for glock holder auto-demotion

[Dan Carpenter]

Hello Bob Peterson,

The patch dc732906c245: "gfs2: Introduce flag for glock holder
auto-demotion" from Aug 19, 2021, leads to the following Smatch
static checker warning:

	fs/gfs2/glock.c:421 demote_incompat_holders()
	warn: iterator 'gh->gh_list.next' changed during iteration

fs/gfs2/glock.c
    411 static void demote_incompat_holders(struct gfs2_glock *gl,
    412                                     struct gfs2_holder *new_gh)
    413 {
    414         struct gfs2_holder *gh;
    415 
    416         /*
    417          * Demote incompatible holders before we make ourselves eligible.
    418          * (This holder may or may not allow auto-demoting, but we don't want
    419          * to demote the new holder before it's even granted.)
    420          */
--> 421         list_for_each_entry(gh, &gl->gl_holders, gh_list) {
    422                 /*
    423                  * Since holders are at the front of the list, we stop when we
    424                  * find the first non-holder.
    425                  */
    426                 if (!test_bit(HIF_HOLDER, &gh->gh_iflags))
    427                         return;
    428                 if (test_bit(HIF_MAY_DEMOTE, &gh->gh_iflags) &&
    429                     !may_grant(gl, new_gh, gh)) {
    430                         /*
    431                          * We should not recurse into do_promote because
    432                          * __gfs2_glock_dq only calls handle_callback,
    433                          * gfs2_glock_add_to_lru and __gfs2_glock_queue_work.
    434                          */
    435                         __gfs2_glock_dq(gh);
                                ^^^^^^^^^^^^^^^^^^^^
This calls list_del_init(&gh->gh_list); which sets the ->next pointer.
So I think that means we could hit a forever loop situation looking for
the original &gl->gl_holders list head.

Should it use list_for_each_entry_safe()?

    436                 }
    437         }
    438 }

regards,
dan carpenter

[Cluster-devel] [bug report] gfs2: Introduce flag for glock holder auto-demotion

[Andreas Gruenbacher]

Hi Dan,

On Mon, Nov 8, 2021 at 3:55 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Bob Peterson,
>
> The patch dc732906c245: "gfs2: Introduce flag for glock holder
> auto-demotion" from Aug 19, 2021, leads to the following Smatch
> static checker warning:
>
>         fs/gfs2/glock.c:421 demote_incompat_holders()
>         warn: iterator 'gh->gh_list.next' changed during iteration
>
> fs/gfs2/glock.c
>     411 static void demote_incompat_holders(struct gfs2_glock *gl,
>     412                                     struct gfs2_holder *new_gh)
>     413 {
>     414         struct gfs2_holder *gh;
>     415
>     416         /*
>     417          * Demote incompatible holders before we make ourselves eligible.
>     418          * (This holder may or may not allow auto-demoting, but we don't want
>     419          * to demote the new holder before it's even granted.)
>     420          */
> --> 421         list_for_each_entry(gh, &gl->gl_holders, gh_list) {
>     422                 /*
>     423                  * Since holders are at the front of the list, we stop when we
>     424                  * find the first non-holder.
>     425                  */
>     426                 if (!test_bit(HIF_HOLDER, &gh->gh_iflags))
>     427                         return;
>     428                 if (test_bit(HIF_MAY_DEMOTE, &gh->gh_iflags) &&
>     429                     !may_grant(gl, new_gh, gh)) {
>     430                         /*
>     431                          * We should not recurse into do_promote because
>     432                          * __gfs2_glock_dq only calls handle_callback,
>     433                          * gfs2_glock_add_to_lru and __gfs2_glock_queue_work.
>     434                          */
>     435                         __gfs2_glock_dq(gh);
>                                 ^^^^^^^^^^^^^^^^^^^^
> This calls list_del_init(&gh->gh_list); which sets the ->next pointer.
> So I think that means we could hit a forever loop situation looking for
> the original &gl->gl_holders list head.
>
> Should it use list_for_each_entry_safe()?

that's exactly right. Thanks for catching this!

Andreas