* [bug report] xfs: allow setting and clearing of log incompat feature flags
@ 2023-01-17 10:56 Dan Carpenter
2023-01-17 21:07 ` Dave Chinner
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-01-17 10:56 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs
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.
1316
1317 /*
1318 * Write the primary superblock to disk immediately, because we need
1319 * the log_incompat bit to be set in the primary super now to protect
1320 * the log items that we're going to commit later.
1321 */
1322 dsb = mp->m_sb_bp->b_addr;
1323 xfs_sb_to_disk(dsb, &mp->m_sb);
1324 dsb->sb_features_log_incompat |= cpu_to_be32(feature);
1325 error = xfs_bwrite(mp->m_sb_bp);
1326 if (error)
1327 goto shutdown;
1328
1329 /*
1330 * Add the feature bits to the incore superblock before we unlock the
1331 * buffer.
1332 */
1333 xfs_sb_add_incompat_log_features(&mp->m_sb, feature);
1334 xfs_buf_relse(mp->m_sb_bp);
1335
1336 /* Log the superblock to disk. */
1337 return xfs_sync_sb(mp, false);
1338 shutdown:
1339 xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
1340 rele:
1341 xfs_buf_relse(mp->m_sb_bp);
1342 return error;
1343 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] xfs: allow setting and clearing of log incompat feature flags
2023-01-17 10:56 [bug report] xfs: allow setting and clearing of log incompat feature flags Dan Carpenter
@ 2023-01-17 21:07 ` Dave Chinner
2023-01-17 23:41 ` Darrick J. Wong
2023-01-18 8:31 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Dave Chinner @ 2023-01-17 21:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: djwong, linux-xfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] xfs: allow setting and clearing of log incompat feature flags
2023-01-17 21:07 ` Dave Chinner
@ 2023-01-17 23:41 ` Darrick J. Wong
2023-01-18 8:31 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2023-01-17 23:41 UTC (permalink / raw)
To: Dave Chinner; +Cc: Dan Carpenter, linux-xfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] xfs: allow setting and clearing of log incompat feature flags
2023-01-17 21:07 ` Dave Chinner
2023-01-17 23:41 ` Darrick J. Wong
@ 2023-01-18 8:31 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-01-18 8:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: djwong, linux-xfs
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....
The Smatch check is working as designed... The problem was me reading
the code, because I thought an "incompatible" feature would be bad, but
actually the function is called xfs_add_incompat_log_feature() so it's
good.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-18 9:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-17 10:56 [bug report] xfs: allow setting and clearing of log incompat feature flags Dan Carpenter
2023-01-17 21:07 ` Dave Chinner
2023-01-17 23:41 ` Darrick J. Wong
2023-01-18 8:31 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox