Hi Dan,
On Mon, Nov 8, 2021 at 3:55 PM Dan Carpenter <dan.carpen...@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
