linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] xfsprogs: consolidate stripe validation
@ 2020-10-09  5:24 Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 1/3] libxfs: allow i18n to xfs printk Gao Xiang
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Gao Xiang @ 2020-10-09  5:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen, Dave Chinner, Gao Xiang

v4: https://lore.kernel.org/r/20201007140402.14295-1-hsiangkao@aol.com

Hi,

This is another approach suggested by Eric in the reply of v3
(if I understand correctly), which also attempts to use
i18n-enabled xfsprogs xfs_notice() to error out sanity check
failure suggested by Dave on IRC.

kernel side of [PATCH 2/3]
https://lore.kernel.org/r/20201009050546.32174-1-hsiangkao@redhat.com

Thanks,
Gao Xiang

Changes since v4:
 - [2/3] stretch columns for commit message (Darrick);
 - [2/3] add a comment to hasdalign check (Darrick);
 - [2/3] break old sunit / swidth != 0 check into two
         seperate checks (Darrick);
 - [2/3] update an error message description (Darrick);
 - [2/3] use bytes for sunit / swidth representation,
         so users can see values in the unique unit.
 - [3/3] introduce a userspace wrapper
         libxfs_validate_stripe_factors (Darrick).

Gao Xiang (3):
  libxfs: allow i18n to xfs printk
  xfs: introduce xfs_validate_stripe_factors()
  mkfs: make use of xfs_validate_stripe_factors()

 libxfs/libxfs_api_defs.h |  1 +
 libxfs/libxfs_priv.h     |  8 ++---
 libxfs/xfs_sb.c          | 65 +++++++++++++++++++++++++++++++++-------
 libxfs/xfs_sb.h          |  3 ++
 mkfs/xfs_mkfs.c          | 23 +++++---------
 5 files changed, 69 insertions(+), 31 deletions(-)

-- 
2.18.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 1/3] libxfs: allow i18n to xfs printk
  2020-10-09  5:24 [PATCH v5 0/3] xfsprogs: consolidate stripe validation Gao Xiang
@ 2020-10-09  5:24 ` Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors() Gao Xiang
  2 siblings, 0 replies; 7+ messages in thread
From: Gao Xiang @ 2020-10-09  5:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen, Dave Chinner, Gao Xiang

In preparation to a common stripe validation helper, allow i18n to
xfs_{notice,warn,err,alert} so xfsprogs can share code with kernel.

Suggested-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 libxfs/libxfs_priv.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 4356fa43..bd724c32 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -121,10 +121,10 @@ extern char    *progname;
 extern void cmn_err(int, char *, ...);
 enum ce { CE_DEBUG, CE_CONT, CE_NOTE, CE_WARN, CE_ALERT, CE_PANIC };
 
-#define xfs_notice(mp,fmt,args...)		cmn_err(CE_NOTE,fmt, ## args)
-#define xfs_warn(mp,fmt,args...)		cmn_err(CE_WARN,fmt, ## args)
-#define xfs_err(mp,fmt,args...)			cmn_err(CE_ALERT,fmt, ## args)
-#define xfs_alert(mp,fmt,args...)		cmn_err(CE_ALERT,fmt, ## args)
+#define xfs_notice(mp,fmt,args...)	cmn_err(CE_NOTE, _(fmt), ## args)
+#define xfs_warn(mp,fmt,args...)	cmn_err(CE_WARN, _(fmt), ## args)
+#define xfs_err(mp,fmt,args...)		cmn_err(CE_ALERT, _(fmt), ## args)
+#define xfs_alert(mp,fmt,args...)	cmn_err(CE_ALERT, _(fmt), ## args)
 
 #define xfs_buf_ioerror_alert(bp,f)	((void) 0);
 
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/3] xfs: introduce xfs_validate_stripe_factors()
  2020-10-09  5:24 [PATCH v5 0/3] xfsprogs: consolidate stripe validation Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 1/3] libxfs: allow i18n to xfs printk Gao Xiang
@ 2020-10-09  5:24 ` Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors() Gao Xiang
  2 siblings, 0 replies; 7+ messages in thread
From: Gao Xiang @ 2020-10-09  5:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen, Dave Chinner, Gao Xiang

Introduce a common helper to consolidate stripe validation process.
Also make kernel code xfs_validate_sb_common() use it first.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 libxfs/xfs_sb.c | 65 ++++++++++++++++++++++++++++++++++++++++---------
 libxfs/xfs_sb.h |  3 +++
 2 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index 302eea16..0f2697ed 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -357,21 +357,18 @@ xfs_validate_sb_common(
 		}
 	}
 
-	if (sbp->sb_unit) {
-		if (!xfs_sb_version_hasdalign(sbp) ||
-		    sbp->sb_unit > sbp->sb_width ||
-		    (sbp->sb_width % sbp->sb_unit) != 0) {
-			xfs_notice(mp, "SB stripe unit sanity check failed");
-			return -EFSCORRUPTED;
-		}
-	} else if (xfs_sb_version_hasdalign(sbp)) {
+	/*
+	 * Either (sb_unit and !hasdalign) or (!sb_unit and hasdalign)
+	 * would imply the image is corrupted.
+	 */
+	if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
 		xfs_notice(mp, "SB stripe alignment sanity check failed");
 		return -EFSCORRUPTED;
-	} else if (sbp->sb_width) {
-		xfs_notice(mp, "SB stripe width sanity check failed");
-		return -EFSCORRUPTED;
 	}
 
+	if (!xfs_validate_stripe_factors(mp, XFS_FSB_TO_B(mp, sbp->sb_unit),
+					 XFS_FSB_TO_B(mp, sbp->sb_width), 0))
+		return -EFSCORRUPTED;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
@@ -1208,3 +1205,49 @@ xfs_sb_get_secondary(
 	*bpp = bp;
 	return 0;
 }
+
+/*
+ * sunit, swidth, sectorsize(optional with 0) should be all in bytes,
+ * so users won't be confused by values in error messages.
+ */
+bool
+xfs_validate_stripe_factors(
+	struct xfs_mount	*mp,
+	__s64			sunit,
+	__s64			swidth,
+	int			sectorsize)
+{
+	if (sectorsize && sunit % sectorsize) {
+		xfs_notice(mp,
+"stripe unit (%lld) must be a multiple of the sector size (%d)",
+			   sunit, sectorsize);
+		return false;
+	}
+
+	if (sunit && !swidth) {
+		xfs_notice(mp,
+"invalid stripe unit (%lld) and stripe width of 0", sunit);
+		return false;
+	}
+
+	if (!sunit && swidth) {
+		xfs_notice(mp,
+"invalid stripe width (%lld) and stripe unit of 0", swidth);
+		return false;
+	}
+
+	if (sunit > swidth) {
+		xfs_notice(mp,
+"stripe unit (%lld) is larger than the stripe width (%lld)", sunit, swidth);
+		return false;
+	}
+
+	if (sunit && (swidth % sunit)) {
+		xfs_notice(mp,
+"stripe width (%lld) must be a multiple of the stripe unit (%lld)",
+			   swidth, sunit);
+		return false;
+	}
+	return true;
+}
+
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index 92465a9a..2d3504eb 100644
--- a/libxfs/xfs_sb.h
+++ b/libxfs/xfs_sb.h
@@ -42,4 +42,7 @@ extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
 				struct xfs_trans *tp, xfs_agnumber_t agno,
 				struct xfs_buf **bpp);
 
+extern bool	xfs_validate_stripe_factors(struct xfs_mount *mp,
+				__s64 sunit, __s64 swidth, int sectorsize);
+
 #endif	/* __XFS_SB_H__ */
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors()
  2020-10-09  5:24 [PATCH v5 0/3] xfsprogs: consolidate stripe validation Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 1/3] libxfs: allow i18n to xfs printk Gao Xiang
  2020-10-09  5:24 ` [PATCH v5 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
@ 2020-10-09  5:24 ` Gao Xiang
  2020-10-12 13:06   ` Brian Foster
  2 siblings, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2020-10-09  5:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen, Dave Chinner, Gao Xiang

Check stripe numbers in calc_stripe_factors() by using
xfs_validate_stripe_factors().

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 libxfs/libxfs_api_defs.h |  1 +
 mkfs/xfs_mkfs.c          | 23 +++++++----------------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index e7e42e93..c1b009c1 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -188,6 +188,7 @@
 #define xfs_trans_roll_inode		libxfs_trans_roll_inode
 #define xfs_trans_roll			libxfs_trans_roll
 
+#define xfs_validate_stripe_factors	libxfs_validate_stripe_factors
 #define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_agino		libxfs_verify_agino
 #define xfs_verify_cksum		libxfs_verify_cksum
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 8fe149d7..5ce063ae 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2305,12 +2305,6 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}
 
-		if (dsu % cfg->sectorsize) {
-			fprintf(stderr,
-_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
-			usage();
-		}
-
 		dsunit  = (int)BTOBBT(dsu);
 		big_dswidth = (long long int)dsunit * dsw;
 		if (big_dswidth > INT_MAX) {
@@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit (
 		dswidth = big_dswidth;
 	}
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
-	    (dsunit && (dswidth % dsunit != 0))) {
-		fprintf(stderr,
-_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
-			dswidth, dsunit);
+	if (!libxfs_validate_stripe_factors(NULL, BBTOB(dsunit), BBTOB(dswidth),
+					    cfg->sectorsize))
 		usage();
-	}
 
 	/* If sunit & swidth were manually specified as 0, same as noalign */
 	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
@@ -2344,11 +2334,12 @@ _("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) {
-		/* Ignore nonsense from device.  XXX add more validation */
-		if (ft->dsunit && ft->dswidth == 0) {
+		/* Ignore nonsense from device report. */
+		if (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit),
+						    BBTOB(ft->dswidth), 0)) {
 			fprintf(stderr,
-_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
-				progname, BBTOB(ft->dsunit));
+_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
+				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
 			ft->dsunit = 0;
 			ft->dswidth = 0;
 		} else {
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors()
  2020-10-09  5:24 ` [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors() Gao Xiang
@ 2020-10-12 13:06   ` Brian Foster
  2020-10-12 14:07     ` Gao Xiang
  0 siblings, 1 reply; 7+ messages in thread
From: Brian Foster @ 2020-10-12 13:06 UTC (permalink / raw)
  To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Eric Sandeen, Dave Chinner

On Fri, Oct 09, 2020 at 01:24:21PM +0800, Gao Xiang wrote:
> Check stripe numbers in calc_stripe_factors() by using
> xfs_validate_stripe_factors().
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  libxfs/libxfs_api_defs.h |  1 +
>  mkfs/xfs_mkfs.c          | 23 +++++++----------------
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
...
> @@ -2344,11 +2334,12 @@ _("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) {
> -		/* Ignore nonsense from device.  XXX add more validation */
> -		if (ft->dsunit && ft->dswidth == 0) {
> +		/* Ignore nonsense from device report. */
> +		if (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit),
> +						    BBTOB(ft->dswidth), 0)) {

The logic seems fine and from the previous comment it sounds like we're
lacking validation in this particular scenario, but do we want to print
more error noise from the validation helper in scenarios where failure
is not a fatal error?

Brian

>  			fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> -				progname, BBTOB(ft->dsunit));
> +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
>  			ft->dsunit = 0;
>  			ft->dswidth = 0;
>  		} else {
> -- 
> 2.18.1
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors()
  2020-10-12 13:06   ` Brian Foster
@ 2020-10-12 14:07     ` Gao Xiang
  2020-10-12 14:20       ` Brian Foster
  0 siblings, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2020-10-12 14:07 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs, Darrick J. Wong, Eric Sandeen, Dave Chinner

Hi Brian,

On Mon, Oct 12, 2020 at 09:06:51AM -0400, Brian Foster wrote:
> On Fri, Oct 09, 2020 at 01:24:21PM +0800, Gao Xiang wrote:
> > Check stripe numbers in calc_stripe_factors() by using
> > xfs_validate_stripe_factors().
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  libxfs/libxfs_api_defs.h |  1 +
> >  mkfs/xfs_mkfs.c          | 23 +++++++----------------
> >  2 files changed, 8 insertions(+), 16 deletions(-)
> > 
> ...
> > @@ -2344,11 +2334,12 @@ _("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) {
> > -		/* Ignore nonsense from device.  XXX add more validation */
> > -		if (ft->dsunit && ft->dswidth == 0) {
> > +		/* Ignore nonsense from device report. */
> > +		if (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit),
> > +						    BBTOB(ft->dswidth), 0)) {
> 
> The logic seems fine and from the previous comment it sounds like we're
> lacking validation in this particular scenario, but do we want to print
> more error noise from the validation helper in scenarios where failure
> is not a fatal error?

Yeah, If I understand correctly, I think that is an open question here,
so I think you suggested that we could silence for this case by passing
a "bool silent" argument? or some better idea for this?

Thanks,
Gao Xiang

> 
> Brian
> 
> >  			fprintf(stderr,
> > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> > -				progname, BBTOB(ft->dsunit));
> > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> > +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
> >  			ft->dsunit = 0;
> >  			ft->dswidth = 0;
> >  		} else {
> > -- 
> > 2.18.1
> > 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors()
  2020-10-12 14:07     ` Gao Xiang
@ 2020-10-12 14:20       ` Brian Foster
  0 siblings, 0 replies; 7+ messages in thread
From: Brian Foster @ 2020-10-12 14:20 UTC (permalink / raw)
  To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Eric Sandeen, Dave Chinner

On Mon, Oct 12, 2020 at 10:07:15PM +0800, Gao Xiang wrote:
> Hi Brian,
> 
> On Mon, Oct 12, 2020 at 09:06:51AM -0400, Brian Foster wrote:
> > On Fri, Oct 09, 2020 at 01:24:21PM +0800, Gao Xiang wrote:
> > > Check stripe numbers in calc_stripe_factors() by using
> > > xfs_validate_stripe_factors().
> > > 
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > ---
> > >  libxfs/libxfs_api_defs.h |  1 +
> > >  mkfs/xfs_mkfs.c          | 23 +++++++----------------
> > >  2 files changed, 8 insertions(+), 16 deletions(-)
> > > 
> > ...
> > > @@ -2344,11 +2334,12 @@ _("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) {
> > > -		/* Ignore nonsense from device.  XXX add more validation */
> > > -		if (ft->dsunit && ft->dswidth == 0) {
> > > +		/* Ignore nonsense from device report. */
> > > +		if (!libxfs_validate_stripe_factors(NULL, BBTOB(ft->dsunit),
> > > +						    BBTOB(ft->dswidth), 0)) {
> > 
> > The logic seems fine and from the previous comment it sounds like we're
> > lacking validation in this particular scenario, but do we want to print
> > more error noise from the validation helper in scenarios where failure
> > is not a fatal error?
> 
> Yeah, If I understand correctly, I think that is an open question here,
> so I think you suggested that we could silence for this case by passing
> a "bool silent" argument? or some better idea for this?
> 

Sure, that seems reasonable to me. Maybe others have thoughts. My
concern was primarily based on usability in terms of not potentially
confusing users with spurious error messages.

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > >  			fprintf(stderr,
> > > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> > > -				progname, BBTOB(ft->dsunit));
> > > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> > > +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
> > >  			ft->dsunit = 0;
> > >  			ft->dswidth = 0;
> > >  		} else {
> > > -- 
> > > 2.18.1
> > > 
> > 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-12 14:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09  5:24 [PATCH v5 0/3] xfsprogs: consolidate stripe validation Gao Xiang
2020-10-09  5:24 ` [PATCH v5 1/3] libxfs: allow i18n to xfs printk Gao Xiang
2020-10-09  5:24 ` [PATCH v5 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
2020-10-09  5:24 ` [PATCH v5 3/3] mkfs: make use of xfs_validate_stripe_factors() Gao Xiang
2020-10-12 13:06   ` Brian Foster
2020-10-12 14:07     ` Gao Xiang
2020-10-12 14:20       ` Brian Foster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).