Re: [PATCH] mkfs: avoid divide-by-zero when hardware reports optimal i/o size as 0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/30/18 9:10 PM, Eric Sandeen wrote:
> On 7/19/18 4:23 PM, Jeff Mahoney wrote:
>> Commit 051b4e37f5e (mkfs: factor AG alignment) factored out the
>> AG alignment code into a separate function.  It got rid of
>> redundant checks for dswidth != 0 since calc_stripe_factors was
>> supposed to guarantee that if dsunit is non-zero dswidth will be
>> as well.  Unfortunately, there's hardware out there that reports its
>> optimal i/o size as larger than the maximum i/o size, which the kernel
>> treats as broken and zeros out the optimal i/o size.  We'll accept
>> the multi-sector dsunit but have a zero dswidth and hit a divide-by-zero
>> in align_ag_geometry.
>>
>> To resolve this we can check the topology before consuming it, default
>> to using the stripe unit as the stripe width, and warn the user about it.
>>
>> Fixes: 051b4e37f5e (mkfs: factor AG alignment)
>> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
> 
> Looks fine to me logically.  Sorry for nitpicking a patch again (it's
> a character flaw) but I'd like to massage this slightly:
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1074886..231542f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2281,6 +2281,16 @@ _("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) {
> +		/* Watch out for nonsense from device */
> +		if (ft->dsunit && ft->dswidth == 0) {
> +			fprintf(stderr,
> +_("%s: Volume reports stripe unit of %d bytes but stripe width of 0.\n"),
> +				progname, ft->dsunit << 9);
> +			fprintf(stderr,
> +_("Using stripe width of %d bytes, which may not be optimal.\n"),
> +				ft->dsunit << 9);
> +			ft->dswidth = ft->dsunit;
> +		}
>  		dsunit = ft->dsunit;
>  		dswidth = ft->dswidth;
>  		use_dev = true;
> 
> to make it a bit more clear that we're checking the /device/-reported
> topology (by looking at ft before using it) and also to break up
> the long warning message into < 80 char lines.  OK?
> 
> This all seems a little messy yet (an inherited mess) but that's slightly
> clearer to me.

Hm, though now I'm half tempted to put all the dswidth-vs-dsunit checks in a helper,
and if it fails on the commandline values, usage(); on detected values, set to 0
with a warning, as it does here anyway:

        /*
         * now we have our stripe config, check it's a multiple of block
         * size.
         */
        if ((BBTOB(dsunit) % cfg->blocksize) ||
            (BBTOB(dswidth) % cfg->blocksize)) {
                if (!use_dev) {
...
                }
                dsunit = 0;
                dswidth = 0;
                cfg->sb_feat.nodalign = true;)

and let the user respecify if they wish.  *shrug* I may follow up with another patch
if it works out.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux