On 3/14/19 4:05 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Validate extent and cow extent size hints that are passed to mkfs so
> that we avoid formatting a filesystem that will never mount.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> mkfs/xfs_mkfs.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d1387ddf..9e1c6ec5 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2202,6 +2202,66 @@ validate_rtextsize(
> ASSERT(cfg->rtextblocks);
> }
>
> +/* Validate the incoming extsize hint as if we were a file. */
I wonder why "as a file" when hints go on directories, right?
> +static void
> +validate_extsize_hint(
> + struct xfs_mount *mp,
> + struct cli_params *cli)
> +{
> + xfs_failaddr_t fa;
> + bool rt;
> + uint16_t flags = 0;
> +
> + rt = (cli->fsx.fsx_xflags & XFS_DIFLAG_RTINHERIT);
> +
> + if (cli->fsx.fsx_xflags & XFS_DIFLAG_EXTSZINHERIT)
> + flags |= XFS_DIFLAG_EXTSIZE;
i.e. that feels like a weird switch from dir inherit flag to file extsize
flag, and I'm not sure why it's done here.
since the mkfs option essentially sets the inherit flag on the root inode,
(right?) I'd kind of expected this to be:
+ if (cli->fsx.fsx_xflags & XFS_DIFLAG_EXTSZINHERIT)
+ flags |= XFS_DIFLAG_EXTSZINHERIT;
(also surely fsx_xflags should contain XFLAGs? but that's another patch)
> +
> + fa = libxfs_inode_validate_extsize(mp, cli->fsx.fsx_extsize, S_IFREG,
> + flags);
and then:
+ fa = libxfs_inode_validate_extsize(mp, cli->fsx.fsx_extsize, S_IFDIR,
+ flags);
doesn't really matter to the validator, but conceptually this seems like a
root dir flag, not a "pretend it's a file!" flag, and seems more consistent.
> + if (rt && fa == NULL)
> + fa = libxfs_inode_validate_extsize(mp, cli->fsx.fsx_extsize,
> + S_IFREG, flags | XFS_DIFLAG_REALTIME);
here too...?
We talked about this a little on irc but maybe you can remind me why you
switch to "it's a file!"
> + if (fa == NULL)
> + return;
> +
I wonder if there should be a comment here about keeping these calculations in
sync with xfs_inode_validate_extsize()?
> + if (rt && mp->m_sb.sb_rextsize > 1)
> + fprintf(stderr,
> + _("illegal extent size hint %lld, must be less than %u and a multiple of %u.\n"),
> + (long long)cli->fsx.fsx_extsize,
> + min(MAXEXTLEN, mp->m_sb.sb_agblocks / 2),
> + mp->m_sb.sb_rextsize);
> + else
> + fprintf(stderr,
> + _("illegal extent size hint %lld, must be less than %u.\n"),
> + (long long)cli->fsx.fsx_extsize,
> + min(MAXEXTLEN, mp->m_sb.sb_agblocks / 2));
> + usage();
> +}
> +
> +/* Validate the incoming CoW extsize hint as if we were a file. */
> +static void
> +validate_cowextsize_hint(
> + struct xfs_mount *mp,
> + struct cli_params *cli)
> +{
> + xfs_failaddr_t fa;
> + uint64_t flags2 = 0;
> +
> + if (cli->fsx.fsx_xflags & FS_XFLAG_COWEXTSIZE)
> + flags2 |= XFS_DIFLAG2_COWEXTSIZE;
(here the flag is the same for dir or file, but same basic question)
> + fa = libxfs_inode_validate_cowextsize(mp, cli->fsx.fsx_cowextsize,
> + S_IFREG, 0, flags2);
> + if (fa == NULL)
> + return;
> +
> + fprintf(stderr,
> +_("illegal CoW extent size hint %lld, must be less than %u.\n"),
> + (long long)cli->fsx.fsx_cowextsize,
> + min(MAXEXTLEN, mp->m_sb.sb_agblocks / 2));
> + usage();
> +}
> +
> /*
> * Validate the configured stripe geometry, or is none is specified, pull
> * the configuration from the underlying device.
> @@ -3945,6 +4005,10 @@ main(
>
> finish_superblock_setup(&cfg, mp, sbp);
>
> + /* Validate the extent size hints now that @mp is fully set up. */
> + validate_extsize_hint(mp, &cli);
> + validate_cowextsize_hint(mp, &cli);
> +
> /* Print the intended geometry of the fs. */
> if (!quiet || dry_run) {
> struct xfs_fsop_geom geo;
>
