[Cluster-devel] [bug report] GFS2: Fix bug-trap in ail flush code

[Cluster-devel] [bug report] GFS2: Fix bug-trap in ail flush code

[Dan Carpenter]

Hi GFS2 devs,

This is 10 year old code, but it looks suspicious and hopefully the
recovery code doesn't get testing very often in runtime.

The patch 75549186edf1: "GFS2: Fix bug-trap in ail flush code" from
Aug 2, 2011, leads to the following static checker warning:

	fs/gfs2/glock.c:1487 gfs2_glock_dq()
	warn: sleeping in atomic context

fs/gfs2/glops.c
    57  static void __gfs2_ail_flush(struct gfs2_glock *gl, bool fsync,
    58                               unsigned int nr_revokes)
    59  {
    60          struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
    61          struct list_head *head = &gl->gl_ail_list;
    62          struct gfs2_bufdata *bd, *tmp;
    63          struct buffer_head *bh;
    64          const unsigned long b_state = (1UL << BH_Dirty)|(1UL << BH_Pinned)|(1UL << BH_Lock);
    65  
    66          gfs2_log_lock(sdp);
                ^^^^^^^^^^^^^^^^^^
    67          spin_lock(&sdp->sd_ail_lock);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We're holding a spinlock here

    68          list_for_each_entry_safe_reverse(bd, tmp, head, bd_ail_gl_list) {
    69                  if (nr_revokes == 0)
    70                          break;
    71                  bh = bd->bd_bh;
    72                  if (bh->b_state & b_state) {
    73                          if (fsync)
    74                                  continue;
    75                          gfs2_ail_error(gl, bh);
                                ^^^^^^^^^^^^^^^^^^^^^^
The gfs2_ail_error() function calls gfs2_withdraw() which can sleep or
the call tree that this is complains about is:

--> gfs2_ail_error()
   --> gfs2_withdraw()
    --> signal_our_withdraw()
        -->gfs2_glock_dq()

It's also very possible that this is a false positive...  Smatch doesn't
understand bit tests very well and especially across function
boundaries.

    76                  }
    77                  gfs2_trans_add_revoke(sdp, bd);
    78                  nr_revokes--;
    79          }
    80          GLOCK_BUG_ON(gl, !fsync && atomic_read(&gl->gl_ail_count));
    81          spin_unlock(&sdp->sd_ail_lock);
    82          gfs2_log_unlock(sdp);
    83  }

regards,
dan carpenter

[Cluster-devel] [bug report] GFS2: Fix bug-trap in ail flush code

[Bob Peterson]

On 7/28/21 3:31 AM, Dan Carpenter wrote:
> 
> Hi GFS2 devs,
> 
> This is 10 year old code, but it looks suspicious and hopefully the
> recovery code doesn't get testing very often in runtime.
> 
> The patch 75549186edf1: "GFS2: Fix bug-trap in ail flush code" from
> Aug 2, 2011, leads to the following static checker warning:
> 
> 	fs/gfs2/glock.c:1487 gfs2_glock_dq()
> 	warn: sleeping in atomic context
> 
> fs/gfs2/glops.c
>      57  static void __gfs2_ail_flush(struct gfs2_glock *gl, bool fsync,
>      58                               unsigned int nr_revokes)
>      59  {
>      60          struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
>      61          struct list_head *head = &gl->gl_ail_list;
>      62          struct gfs2_bufdata *bd, *tmp;
>      63          struct buffer_head *bh;
>      64          const unsigned long b_state = (1UL << BH_Dirty)|(1UL << BH_Pinned)|(1UL << BH_Lock);
>      65
>      66          gfs2_log_lock(sdp);
>                  ^^^^^^^^^^^^^^^^^^
>      67          spin_lock(&sdp->sd_ail_lock);
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We're holding a spinlock here
> 
>      68          list_for_each_entry_safe_reverse(bd, tmp, head, bd_ail_gl_list) {
>      69                  if (nr_revokes == 0)
>      70                          break;
>      71                  bh = bd->bd_bh;
>      72                  if (bh->b_state & b_state) {
>      73                          if (fsync)
>      74                                  continue;
>      75                          gfs2_ail_error(gl, bh);
>                                  ^^^^^^^^^^^^^^^^^^^^^^
> The gfs2_ail_error() function calls gfs2_withdraw() which can sleep or
> the call tree that this is complains about is:
> 
> --> gfs2_ail_error()
>     --> gfs2_withdraw()
>      --> signal_our_withdraw()
>          -->gfs2_glock_dq()
> 
> It's also very possible that this is a false positive...  Smatch doesn't
> understand bit tests very well and especially across function
> boundaries.
> 
>      76                  }
>      77                  gfs2_trans_add_revoke(sdp, bd);
>      78                  nr_revokes--;
>      79          }
>      80          GLOCK_BUG_ON(gl, !fsync && atomic_read(&gl->gl_ail_count));
>      81          spin_unlock(&sdp->sd_ail_lock);
>      82          gfs2_log_unlock(sdp);
>      83  }
> 
> regards,
> dan carpenter
> 
Hi Dan,

Thanks. I don't think it's a false positive. I think we should be using 
a delayed withdraw rather than an actual withdraw. I'll send a patch out 
to fix it shortly.

Regards,

Bob Peterson