On Sun, Aug 05, 2018 at 11:06:57PM -0500, Eric Sandeen wrote:

On 8/5/18 5:20 PM, Dave Chinner wrote:

quoted

On Wed, Aug 01, 2018 at 03:49:45PM -0500, Eric Sandeen wrote:

quoted

From: Jeff Mahoney <redacted>

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.

To resolve this we can check the topology before consuming it, and
ignore the bad stripe geometry.

[sandeen: remove guessing heuristic, just warn and ignore bad data.]

Fixes: 051b4e37f5e (mkfs: factor AG alignment)
Signed-off-by: Jeff Mahoney <redacted>
Reviewed-by: Eric Sandeen <redacted>
Signed-off-by: Eric Sandeen <redacted>
---

so, I rewrote this a bit.  I'm not a fan of guessing what the kernel
really must have meant, becaue next time the root cause may be differnt.
In other cases we ignore bad geometry, I think we should in this case as
well.  This will also let me go forward with a factored-out geometry checker,
and for user-specified badness we'll warn and exit, for kernel-provided
badness we'll warn and ignore.

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1074886..2e53c1e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c

@@ -2281,11 +2281,20 @@ _("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) {
-		dsunit = ft->dsunit;
-		dswidth = ft->dswidth;
-		use_dev = true;
+		/* Ignore nonsense from device.  XXX add more validation */
+		if (ft->dsunit && ft->dswidth == 0) {
+			fprintf(stderr,
+_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
+				progname, BBTOB(ft->dsunit));
+			ft->dsunit = 0;
+			ft->dswidth = 0;

Not sure this is the right thing to do. If a stripe unit has been
given, then the device has an alignment requirement. If it hasn't
given an "optimal IO size", then shouldn't we just set ft->dswidth =
ft->dsunit to retain the alignment the device requested?

Yeah, I'm on the fence about that.  If it's giving us inconsistent information,
how can we know what's right and wrong?

In general, adding alignment when it's not needed does not hurt
performance. However, not having alignment when it is needed almost
always hurts performance.

From that perspective, I think what we should do here is obvious :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com