On Wed, Jan 18, 2023 at 08:07:57AM +1100, Dave Chinner wrote:
> 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....
This is similar to the exchange we had last March:
https://lore.kernel.org/linux-xfs/20220324104521.GF12805@kadam/
wherein there was a series of if tests that would bail out early on
error, followed by one last test that checks if we've no further work to
do and returns a zero that was set earlier in the function. This sort
of situation is probably very difficult to detect within a static
checker, but I would much prefer that we review the online fsck
patchset[1] instead of reopening review on existing code that already
works and has already been merged.
[1] https://lore.kernel.org/linux-xfs/Y69UceeA2MEpjMJ8@magnolia/
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
