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

["Darrick J. Wong" <djwong@kernel.org>]

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@fromorbit.com