Re: [PATCH] mkfs.xfs: introduce sunit/swidth validation helper

Darrick J. Wong
2020-08-03T15:28:17Z

On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote:
> Currently stripe unit/width checking logic is all over xfsprogs.
> So, refactor the same code snippet into a single validation helper
> xfs_validate_stripe_factors(), including:
> - integer overflows of either value
> - sunit and swidth alignment wrt sector size
> - if either sunit or swidth are zero, both should be zero
> - swidth must be a multiple of sunit
>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>
> This patch follows Darrick's original suggestion [1], yet I'm
> not sure if I'm doing the right thing or if something is still
> missing (e.g the meaning of six(ish) places)... So post it
> right now...
>
> TBH, especially all these naming and the helper location (whether
> in topology.c)...plus, click a dislike on calc_stripe_factors()
> itself...
>
> (Hopefully hear some advice about this... Thanks!)
>
> [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia
>
> libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> libfrog/topology.h | 15 ++++++++++++++
> mkfs/xfs_mkfs.c | 48 ++++++++++++++++++++++----------------------
> 3 files changed, 89 insertions(+), 24 deletions(-)
>
> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..cf56fb03 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -174,6 +174,41 @@ out:
> return ret;
> }
>
> +enum xfs_stripe_retcode
> +xfs_validate_stripe_factors(

libfrog functions (and enums) should be prefixed with libfrog, not xfs.

LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.}

> + int sectorsize,
> + int *sup,

Errant space between "int" and "*sup".

> + int *swp)

Strange that a validator function has out parameters...

Also, uh, .... full names, please.

int *sunitp,
int *swidthp)

(I'm vaguely wondering why we use signed ints here vs. unsigned, but
that isn't critical...)

> +{
> + int sunit = *sup, swidth = *swp;
> +
> + if (sectorsize) {
> + long long big_swidth;
> +
> + if (sunit % sectorsize)
> + return XFS_STRIPE_RET_SUNIT_MISALIGN;
> +
> + sunit = (int)BTOBBT(sunit);

Hmm. On input, *sup is in units of bytes, but on output it can be in
units of 512b blocks? That is very surprising...

> + big_swidth = (long long)sunit * swidth;
> +
> + if (big_swidth > INT_MAX)
> + return XFS_STRIPE_RET_SWIDTH_OVERFLOW;
> + swidth = big_swidth;
> + }
> + if ((sunit && !swidth) || (!sunit && swidth))
> + return XFS_STRIPE_RET_PARTIAL_VALID;
> +
> + if (sunit > swidth)
> + return XFS_STRIPE_RET_SUNIT_TOO_LARGE;
> +
> + if (sunit && (swidth % sunit))
> + return XFS_STRIPE_RET_SWIDTH_MISALIGN;
> +
> + *sup = sunit;

...especially since in the !sectorsize case we don't change it at all.

> + *swp = swidth;
> + return XFS_STRIPE_RET_OK;
> +}
> +
> static void blkid_get_topology(
> const char *device,
> int *sunit,
> @@ -229,6 +264,21 @@ static void blkid_get_topology(
> */
> *sunit = *sunit >> 9;
> *swidth = *swidth >> 9;
> + switch (xfs_validate_stripe_factors(0, sunit, swidth)) {
> + case XFS_STRIPE_RET_OK:
> + break;
> + case XFS_STRIPE_RET_PARTIAL_VALID:
> + fprintf(stderr,
> +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"),
> + progname, BBTOB(*sunit), BBTOB(*swidth));

Needs a "/* fallthrough */" comment here.

> + default:

Why don't we warn about receiving garbage geometry that produces
MISALIGN or OVERFLOW?

> + /*
> + * if firmware is broken, just give up and set both to zero,
> + * we can't trust information from this device.
> + */
> + *sunit = 0;
> + *swidth = 0;
> + }
>
> if (blkid_topology_get_alignment_offset(tp) != 0) {
> fprintf(stderr,
> diff --git a/libfrog/topology.h b/libfrog/topology.h
> index 6fde868a..e8be26b2 100644
> --- a/libfrog/topology.h
> +++ b/libfrog/topology.h
> @@ -36,4 +36,19 @@ extern int
> check_overwrite(
> const char *device);
>
> +enum xfs_stripe_retcode {
> + XFS_STRIPE_RET_OK = 0,
> + XFS_STRIPE_RET_SUNIT_MISALIGN,
> + XFS_STRIPE_RET_SWIDTH_OVERFLOW,
> + XFS_STRIPE_RET_PARTIAL_VALID,
> + XFS_STRIPE_RET_SUNIT_TOO_LARGE,
> + XFS_STRIPE_RET_SWIDTH_MISALIGN,
> +};
> +
> +enum xfs_stripe_retcode
> +xfs_validate_stripe_factors(
> + int sectorsize,
> + int *sup,

Errant space between "int" and "*sup".

> + int *swp);
> +
> #endif /* __LIBFROG_TOPOLOGY_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280..a3d6032c 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2255,7 +2255,6 @@ calc_stripe_factors(
> struct cli_params *cli,
> struct fs_topology *ft)
> {
> - long long int big_dswidth;
> int dsunit = 0;
> int dswidth = 0;
> int lsunit = 0;
> @@ -2263,6 +2262,7 @@ calc_stripe_factors(
> int dsw = 0;
> int lsu = 0;
> bool use_dev = false;
> + int error;
>
> if (cli_opt_set(&dopts, D_SUNIT))
> dsunit = cli->dsunit;
> @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n"));
> usage();
> }
>
> - if (dsu % cfg->sectorsize) {
> + dsunit = dsu;
> + dswidth = dsw;
> + error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth);

I thought this function returned an enum?

> + switch(error) {
> + case XFS_STRIPE_RET_SUNIT_MISALIGN:
> fprintf(stderr,
> _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> usage();
> - }
> -
> - dsunit = (int)BTOBBT(dsu);
> - big_dswidth = (long long int)dsunit * dsw;
> - if (big_dswidth > INT_MAX) {
> + break;
> + case XFS_STRIPE_RET_SWIDTH_OVERFLOW:
> fprintf(stderr,
> -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> - big_dswidth, dsunit);
> +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"),

Why change this message?

> + dsw, dsunit);
> usage();
> + break;
> }
> - dswidth = big_dswidth;
> + } else {
> + error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
> }
>
> - if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> - (dsunit && (dswidth % dsunit != 0))) {
> + if (error == XFS_STRIPE_RET_PARTIAL_VALID ||
> + error == XFS_STRIPE_RET_SWIDTH_MISALIGN) {
> fprintf(stderr,
> _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> dswidth, dsunit);
> usage();
> }
>
> + if (error) {
> + fprintf(stderr,
> +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth);

Invalid how? We know the exact reason, so we should say so.

--D

> + usage();
> + }
> +
> /* If sunit & swidth were manually specified as 0, same as noalign */
> if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
> !dsunit && !dswidth)
> @@ -2328,18 +2337,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>
> /* if no stripe config set, use the device default */
> if (!dsunit) {
> - /* Ignore nonsense from device. XXX add more validation */
> - if (ft->dsunit && ft->dswidth == 0) {
> - fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> - progname, BBTOB(ft->dsunit));
> - ft->dsunit = 0;
> - ft->dswidth = 0;
> - } else {
> - dsunit = ft->dsunit;
> - dswidth = ft->dswidth;
> - use_dev = true;
> - }
> + dsunit = ft->dsunit;
> + dswidth = ft->dswidth;
> + use_dev = true;
> } else {
> /* check and warn if user-specified alignment is sub-optimal */
> if (ft->dsunit && ft->dsunit != dsunit) {
> --
> 2.18.1
>