Re: [bug report] xfs: allow setting and clearing of log incompat feature flags

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 17, 2023 at 01:56:11PM +0300, Dan Carpenter wrote:
> Hello Darrick J. Wong,
> 
> The patch 908ce71e54f8: "xfs: allow setting and clearing of log
> incompat feature flags" from Aug 8, 2021, leads to the following
> Smatch static checker warning:
> 
> 	fs/xfs/xfs_mount.c:1315 xfs_add_incompat_log_feature()
> 	warn: missing error code 'error'
> 
> fs/xfs/xfs_mount.c
>     1280 int
>     1281 xfs_add_incompat_log_feature(
>     1282         struct xfs_mount        *mp,
>     1283         uint32_t                feature)
>     1284 {
>     1285         struct xfs_dsb                *dsb;
>     1286         int                        error;
>     1287 
>     1288         ASSERT(hweight32(feature) == 1);
>     1289         ASSERT(!(feature & XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN));
>     1290 
>     1291         /*
>     1292          * Force the log to disk and kick the background AIL thread to reduce
>     1293          * the chances that the bwrite will stall waiting for the AIL to unpin
>     1294          * the primary superblock buffer.  This isn't a data integrity
>     1295          * operation, so we don't need a synchronous push.
>     1296          */
>     1297         error = xfs_log_force(mp, XFS_LOG_SYNC);
>     1298         if (error)
>     1299                 return error;
>     1300         xfs_ail_push_all(mp->m_ail);
>     1301 
>     1302         /*
>     1303          * Lock the primary superblock buffer to serialize all callers that
>     1304          * are trying to set feature bits.
>     1305          */
>     1306         xfs_buf_lock(mp->m_sb_bp);
>     1307         xfs_buf_hold(mp->m_sb_bp);
>     1308 
>     1309         if (xfs_is_shutdown(mp)) {
>     1310                 error = -EIO;
>     1311                 goto rele;
>     1312         }
>     1313 
>     1314         if (xfs_sb_has_incompat_log_feature(&mp->m_sb, feature))
> --> 1315                 goto rele;
>                          ^^^^^^^^^
> It's not clear to me, why this old code is suddenly showing up as a new
> warning...  But it does feel like it should be an error path.

Seems like a smatch issue?

error at this point will be zero and this test is checking if the
superblock is already marked with the incompat feature we need to
add as there can be races with adding and removing the feature flag.
If it is set once we hold the superblock buffer locked, then we just
need to release the locked superblock buffer and return 0 to say it
is set.

IOWs, it looks to me like the code is correct and the checker hasn't
understood the code pattern being used....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com