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 here
> But 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
> (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,
>>
>
