On 7/3/19 11:14 AM, Eric Sandeen wrote:
On 7/3/19 12:47 PM, Eric Sandeen wrote:On 7/2/19 6:27 PM, Allison Collins wrote:From: Allison Henderson <allison.henderson@xxxxxxxxxx> While investigating another mkfs bug, noticed that cfg->lsunit is sometimes left uninitialized when it should not. This is because calc_stripe_factors in some cases needs cfg->loginternal to be set first. This is done in validate_logdev. So move calc_stripe_factors below validate_logdev while parsing configs. Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>Ok, while I appreciate you taking Carlos's input, the patch now does far more than the commit log says, with no explanation of why it's doing so. (and there's no indication of what actually changed in V2: - putting that info below the "---" line is helpful) I'd prefer to take the original patch and if we really want to change how we initialize empty structures, that should be a separate patch, and should hit everywhere we do it, not just mkfs.Ok so I was on track up to hereBut to Carlos's point, cfg->lsunit isn't exactly "uninitialized" (to me, uninitialized means that it was never set, when in fact it was initialized, to zero, right?)and I guess this is just semantics...So it's not quite clear to me what's happening here; I guess this test: /* * check that log sunit is modulo fsblksize or default it to dsunit. */ if (lsunit) { /* convert from 512 byte blocks to fs blocks */ cfg->lsunit = DTOBT(lsunit, cfg->blocklog); } else if (cfg->sb_feat.log_version == 2 && cfg->loginternal && cfg->dsunit) { /* lsunit and dsunit now in fs blocks */ cfg->lsunit = cfg->dsunit; } is doing the wrong thing because cfg->loginternal hasn't actually been evaluated yet? Is there a mkfs command that demonstrates the problem which could be included in the changelog? Does it only happen with external logs? If you can provide a bit more information about when and how this actually fails, that would improve the changelog for future generations.OK, TBH I had confused cfg-> with cli-> (derp) and I see that as you said, validate_logdev() sets up cfg->loginternal, sorry. So I'm ok with V1 and its changelog as it stands, I think. Sorry for my confusion and the noise, -Eric
Alrighty then, I think I answered all your other questions in the IRC chat. Thank you!!
Allison
(also agreeing w/ darrick that these seem like little time bombs...) Thanks, -Eric--- mkfs/xfs_mkfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 468b8fd..6e32403 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -3861,15 +3861,15 @@ main( .isdirect = LIBXFS_DIRECT, .isreadonly = LIBXFS_EXCLUSIVELY, }; - struct xfs_mount mbuf = {}; + struct xfs_mount mbuf = {0}; struct xfs_mount *mp = &mbuf; struct xfs_sb *sbp = &mp->m_sb; - struct fs_topology ft = {}; + struct fs_topology ft = {0}; struct cli_params cli = { .xi = &xi, .loginternal = 1, }; - struct mkfs_params cfg = {}; + struct mkfs_params cfg = {0};/* build time defaults */struct mkfs_default_params dft = { @@ -4008,7 +4008,6 @@ main( cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");validate_rtextsize(&cfg, &cli, &ft);- calc_stripe_factors(&cfg, &cli, &ft);/** Open and validate the device configurations @@ -4018,6 +4017,7 @@ main( validate_datadev(&cfg, &cli); validate_logdev(&cfg, &cli, &logfile); validate_rtdev(&cfg, &cli, &rtfile); + calc_stripe_factors(&cfg, &cli, &ft);/** At this point when know exactly what size all the devices are,
