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