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

From: Jeff Mahoney <jeffm@suse.com>
To: "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
	Eric Sandeen <sandeen@sandeen.net>,
	Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH] mkfs: avoid divide-by-zero when hardware reports optimal i/o size as 0
Date: Fri, 20 Jul 2018 14:08:22 -0400	[thread overview]
Message-ID: <c175f758-bfa3-67d3-4c2d-74642b272aec@suse.com> (raw)
In-Reply-To: <20180720155529.qw7nthesfu6xu7ie@odin.usersys.redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 3240 bytes --]

On 7/20/18 11:55 AM, Carlos Maiolino wrote:
> On Thu, Jul 19, 2018 at 05:23:22PM -0400, 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.
>>
> 
> I wonder if this shouldn't go into blkid_get_topology since something is wrong
> with the information reported by the storage.
> And require a force_overwrite to continue, at this point, something looks quite
> wrong in the storage, and I think this is the last 'resource' a sysadmin will
> have to notice this before making the FS, and start using it, so, maybe requiring
> force_overwrite would bring more attention.

We discussed that initially here:
https://patchwork.kernel.org/patch/10479083/

I worked that up and what ends up happening is that, since we don't have
any context for how the topology will be used, if at all, we print the
error every time.  If the user specified stripe parameters manually, the
topology won't be used.  They won't care if it's broken and certainly
don't need to force it.

Lastly, this wasn't encountered in the real world on some weird discount
hardware.  It's a pretty big product from a major storage vendor.  I've
advised them to fix their firmware but we still need to get users
rolling again.  Warning about a potential suboptimal result is enough,
IMO.  It's not an emergency situation that will result in a completely
broken file system.

-Jeff

>> Fixes: 051b4e37f5e (mkfs: factor AG alignment)
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  mkfs/xfs_mkfs.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index a135e06e..35542e57 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -2295,6 +2295,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>>  	if (!dsunit) {
>>  		dsunit = ft->dsunit;
>>  		dswidth = ft->dswidth;
>> +		if (dsunit && dswidth == 0) {
>> +			fprintf(stderr,
>> +_("%s: Volume reports stripe unit of %d bytes but stripe width of 0.  Using stripe width of %d bytes, which may not be optimal.\n"),
>> +				progname, dsunit << 9, dsunit << 9);
>> +			dswidth = dsunit;
>> +		}
>>  		use_dev = true;
>>  	} else {
>>  		/* check and warn is alignment is sub-optimal */
>> -- 
>> 2.16.4
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Jeff Mahoney
SUSE Labs

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]