From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sandeen.net ([63.231.237.45]:45200 "EHLO sandeen.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728798AbeGaDwo (ORCPT ); Mon, 30 Jul 2018 23:52:44 -0400 Subject: Re: [PATCH] mkfs: avoid divide-by-zero when hardware reports optimal i/o size as 0 From: Eric Sandeen References: <6f62dbc6-f516-e8b5-1f08-6be227a61219@suse.com> <58f58144-d0a3-905b-0eb3-d1f33e42a5de@sandeen.net> Message-ID: Date: Mon, 30 Jul 2018 21:14:50 -0500 MIME-Version: 1.0 In-Reply-To: <58f58144-d0a3-905b-0eb3-d1f33e42a5de@sandeen.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Jeff Mahoney , "linux-xfs@vger.kernel.org" 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 > > 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