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@xxxxxxxxxxxxx
