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

[Carlos Maiolino <cmaiolino@redhat.com>]

On Fri, Jul 20, 2018 at 09:19:23AM -0700, Darrick J. Wong wrote:
> On Fri, Jul 20, 2018 at 05:55:29PM +0200, 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.
> 
> If the storage gives us crap geometry information we don't have to use
> it.  Keep the message that we autodetected nonsense and are dropping it;
> the sysadmin can always re-run mkfs with sensible dsunit/dswidth.
> 
> > 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.
> 
> I prefer reserving -f for "This is about to destroy something and can't
> be undone", not "This auto-optimization is screwed up, continue? (y/N)"

Yeah, I agree, forget everything I said here :P

> 
> --D
> 
> > > 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
> > 
> > -- 
> > Carlos
> > --
> > 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
> --
> 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

-- 
Carlos