* [PATCH v4 0/3] xfsprogs: consolidate stripe validation
[not found] <20201007140402.14295-1-hsiangkao.ref@aol.com>
@ 2020-10-07 14:03 ` Gao Xiang
2020-10-07 14:04 ` [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk Gao Xiang
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Gao Xiang @ 2020-10-07 14:03 UTC (permalink / raw)
To: linux-xfs; +Cc: Eric Sandeen, Darrick J . Wong, Dave Chinner, Gao Xiang
From: Gao Xiang <hsiangkao@redhat.com>
Hi,
v3: https://lore.kernel.org/r/20200806130301.27937-1-hsiangkao@redhat.com
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.
In addition, I manually ported [PATCH 2/3] to the kernel side
as well then fault injection with xfs_db and it seems work
as expected.
Thanks,
Gao Xiang
changes since v3:
- mainly follow Eric suggestion mentioned in the reply of v3,
e.g directly prints the error using xfs_notice/warn() and
return bool;
- with an exception that it doesn't need the unit of sunit/swidth
are in bytes if sectorsize is not specified, since the sanity
check logic condition only needs these are in the same unit,
so it saves calculation for these FSB / sector-based
representation.
Gao Xiang (3):
xfsprogs: allow i18n to xfs printk
xfs: introduce xfs_validate_stripe_factors()
xfsprogs: make use of xfs_validate_stripe_factors()
libxfs/libxfs_priv.h | 8 +++----
libxfs/xfs_sb.c | 54 +++++++++++++++++++++++++++++++++++---------
libxfs/xfs_sb.h | 3 +++
mkfs/xfs_mkfs.c | 23 ++++++-------------
4 files changed, 57 insertions(+), 31 deletions(-)
--
2.24.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk
2020-10-07 14:03 ` [PATCH v4 0/3] xfsprogs: consolidate stripe validation Gao Xiang
@ 2020-10-07 14:04 ` Gao Xiang
2020-10-07 15:28 ` Darrick J. Wong
2020-10-07 14:04 ` [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
2020-10-07 14:04 ` [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors() Gao Xiang
2 siblings, 1 reply; 12+ messages in thread
From: Gao Xiang @ 2020-10-07 14:04 UTC (permalink / raw)
To: linux-xfs
Cc: Eric Sandeen, Darrick J . Wong, Dave Chinner, Gao Xiang, Dave Chinner
From: Gao Xiang <hsiangkao@redhat.com>
In preparation to a common stripe validation helper,
allow i18n to xfs_{notice,warn,err,alert} so that
xfsprogs can share code with kernel.
Suggested-by: Dave Chinner <dchinner@redhat.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 5688284deb4e..545a66dec9b8 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.24.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors()
2020-10-07 14:03 ` [PATCH v4 0/3] xfsprogs: consolidate stripe validation Gao Xiang
2020-10-07 14:04 ` [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk Gao Xiang
@ 2020-10-07 14:04 ` Gao Xiang
2020-10-07 22:29 ` Darrick J. Wong
2020-10-07 14:04 ` [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors() Gao Xiang
2 siblings, 1 reply; 12+ messages in thread
From: Gao Xiang @ 2020-10-07 14:04 UTC (permalink / raw)
To: linux-xfs; +Cc: Eric Sandeen, Darrick J . Wong, Dave Chinner, Gao Xiang
From: Gao Xiang <hsiangkao@redhat.com>
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 | 54 +++++++++++++++++++++++++++++++++++++++----------
libxfs/xfs_sb.h | 3 +++
2 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index d37d60b39a52..bd65828c844e 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -357,21 +357,13 @@ 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)) {
+ 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, sbp->sb_unit, sbp->sb_width, 0))
+ return -EFSCORRUPTED;
if (xfs_sb_version_hascrc(&mp->m_sb) &&
sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
@@ -1208,3 +1200,43 @@ xfs_sb_get_secondary(
*bpp = bp;
return 0;
}
+
+/*
+ * If sectorsize is specified, sunit / swidth must be in bytes;
+ * or both can be in any kind of units (e.g. 512B sector or blocksize).
+ */
+bool
+xfs_validate_stripe_factors(
+ struct xfs_mount *mp,
+ int sunit,
+ int swidth,
+ int sectorsize)
+{
+ if (sectorsize && sunit % sectorsize) {
+ xfs_notice(mp,
+"stripe unit (%d) must be a multiple of the sector size (%d)",
+ sunit, sectorsize);
+ return false;
+ }
+
+ if ((sunit && !swidth) || (!sunit && swidth)) {
+ xfs_notice(mp,
+"stripe unit (%d) and width (%d) are partially valid", sunit, swidth);
+ return false;
+ }
+
+ if (sunit > swidth) {
+ xfs_notice(mp,
+"stripe unit (%d) is too large of the stripe width (%d)", sunit, swidth);
+ return false;
+ }
+
+ if (sunit && (swidth % sunit)) {
+ xfs_notice(mp,
+"stripe width (%d) must be a multiple of the stripe unit (%d)",
+ swidth, sunit);
+ return false;
+ }
+ return true;
+}
+
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index 92465a9a5162..015b2605f587 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,
+ int sunit, int swidth, int sectorsize);
+
#endif /* __XFS_SB_H__ */
--
2.24.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors()
2020-10-07 14:03 ` [PATCH v4 0/3] xfsprogs: consolidate stripe validation Gao Xiang
2020-10-07 14:04 ` [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk Gao Xiang
2020-10-07 14:04 ` [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
@ 2020-10-07 14:04 ` Gao Xiang
2020-10-07 22:30 ` Darrick J. Wong
2 siblings, 1 reply; 12+ messages in thread
From: Gao Xiang @ 2020-10-07 14:04 UTC (permalink / raw)
To: linux-xfs; +Cc: Eric Sandeen, Darrick J . Wong, Dave Chinner, Gao Xiang
From: Gao Xiang <hsiangkao@redhat.com>
Check stripe numbers in calc_stripe_factors() by
using xfs_validate_stripe_factors().
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
mkfs/xfs_mkfs.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280e388..b7f8f98147eb 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2289,12 +2289,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) {
@@ -2306,13 +2300,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 (!xfs_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)) &&
@@ -2328,11 +2318,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 (!xfs_validate_stripe_factors(NULL, 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));
+_("%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.24.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk
2020-10-07 14:04 ` [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk Gao Xiang
@ 2020-10-07 15:28 ` Darrick J. Wong
2020-10-09 1:01 ` Gao Xiang
0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2020-10-07 15:28 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Eric Sandeen, Dave Chinner, Gao Xiang, Dave Chinner
On Wed, Oct 07, 2020 at 10:04:00PM +0800, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
>
> In preparation to a common stripe validation helper,
> allow i18n to xfs_{notice,warn,err,alert} so that
> xfsprogs can share code with kernel.
>
> Suggested-by: Dave Chinner <dchinner@redhat.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 5688284deb4e..545a66dec9b8 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)
AFAICT there isn't anything that passes a _() string to
xfs_{alert,notice,warn,err} so this looks ok to me. It'll be nice to
add the libxfs strings to the message catalogue at last...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
>
> #define xfs_buf_ioerror_alert(bp,f) ((void) 0);
>
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors()
2020-10-07 14:04 ` [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
@ 2020-10-07 22:29 ` Darrick J. Wong
2020-10-09 0:54 ` Gao Xiang
0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2020-10-07 22:29 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Eric Sandeen, Dave Chinner, Gao Xiang
On Wed, Oct 07, 2020 at 10:04:01PM +0800, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
>
> Introduce a common helper to consolidate
> stripe validation process. Also make kernel
> code xfs_validate_sb_common() use it first.
Please use all 72(?) columns here.
>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> libxfs/xfs_sb.c | 54 +++++++++++++++++++++++++++++++++++++++----------
> libxfs/xfs_sb.h | 3 +++
These libxfs changes will have to go through the kernel first.
> 2 files changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
> index d37d60b39a52..bd65828c844e 100644
> --- a/libxfs/xfs_sb.c
> +++ b/libxfs/xfs_sb.c
> @@ -357,21 +357,13 @@ 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)) {
> + if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
Urgh, this logic makes my brain hurt.
"If the zeroness of sb_unit differs from the unsetness of the dalign
feature"? This might need some kind of comment, such as:
/*
* Either sb_unit and hasdalign are both set, or they are zero
* and not set, respectively.
*/
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, sbp->sb_unit, sbp->sb_width, 0))
> + return -EFSCORRUPTED;
>
> if (xfs_sb_version_hascrc(&mp->m_sb) &&
> sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> @@ -1208,3 +1200,43 @@ xfs_sb_get_secondary(
> *bpp = bp;
> return 0;
> }
> +
> +/*
> + * If sectorsize is specified, sunit / swidth must be in bytes;
> + * or both can be in any kind of units (e.g. 512B sector or blocksize).
> + */
> +bool
> +xfs_validate_stripe_factors(
> + struct xfs_mount *mp,
> + int sunit,
> + int swidth,
> + int sectorsize)
> +{
> + if (sectorsize && sunit % sectorsize) {
> + xfs_notice(mp,
> +"stripe unit (%d) must be a multiple of the sector size (%d)",
> + sunit, sectorsize);
> + return false;
> + }
> +
> + if ((sunit && !swidth) || (!sunit && swidth)) {
> + xfs_notice(mp,
> +"stripe unit (%d) and width (%d) are partially valid", sunit, swidth);
I would break these into separate checks and messages.
> + return false;
> + }
> +
> + if (sunit > swidth) {
> + xfs_notice(mp,
> +"stripe unit (%d) is too large of the stripe width (%d)", sunit, swidth);
"stripe unit (%d) is larger than the stripe width..."
--D
> + return false;
> + }
> +
> + if (sunit && (swidth % sunit)) {
> + xfs_notice(mp,
> +"stripe width (%d) must be a multiple of the stripe unit (%d)",
> + swidth, sunit);
> + return false;
> + }
> + return true;
> +}
> +
> diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
> index 92465a9a5162..015b2605f587 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,
> + int sunit, int swidth, int sectorsize);
> +
> #endif /* __XFS_SB_H__ */
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors()
2020-10-07 14:04 ` [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors() Gao Xiang
@ 2020-10-07 22:30 ` Darrick J. Wong
2020-10-09 0:58 ` Gao Xiang
0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2020-10-07 22:30 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Eric Sandeen, Dave Chinner, Gao Xiang
On Wed, Oct 07, 2020 at 10:04:02PM +0800, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
>
> Check stripe numbers in calc_stripe_factors() by
> using xfs_validate_stripe_factors().
>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 23 +++++++----------------
> 1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280e388..b7f8f98147eb 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2289,12 +2289,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) {
> @@ -2306,13 +2300,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 (!xfs_validate_stripe_factors(NULL, BBTOB(dsunit), BBTOB(dswidth),
if (!libxfs_validate_stripe_factors(...))
Unless we get rid of the weird libxfs macro thing, you're supposed to
use prefixes in userspace.
--D
> + 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)) &&
> @@ -2328,11 +2318,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 (!xfs_validate_stripe_factors(NULL, 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));
> +_("%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.24.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors()
2020-10-07 22:29 ` Darrick J. Wong
@ 2020-10-09 0:54 ` Gao Xiang
0 siblings, 0 replies; 12+ messages in thread
From: Gao Xiang @ 2020-10-09 0:54 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Gao Xiang, linux-xfs, Eric Sandeen, Dave Chinner
On Wed, Oct 07, 2020 at 03:29:42PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 07, 2020 at 10:04:01PM +0800, Gao Xiang wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> >
> > Introduce a common helper to consolidate
> > stripe validation process. Also make kernel
> > code xfs_validate_sb_common() use it first.
>
> Please use all 72(?) columns here.
will fix.
>
> >
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> > libxfs/xfs_sb.c | 54 +++++++++++++++++++++++++++++++++++++++----------
> > libxfs/xfs_sb.h | 3 +++
>
> These libxfs changes will have to go through the kernel first.
will send another patch together with the next version.
>
> > 2 files changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
> > index d37d60b39a52..bd65828c844e 100644
> > --- a/libxfs/xfs_sb.c
> > +++ b/libxfs/xfs_sb.c
> > @@ -357,21 +357,13 @@ 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)) {
> > + if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
>
> Urgh, this logic makes my brain hurt.
>
> "If the zeroness of sb_unit differs from the unsetness of the dalign
> feature"? This might need some kind of comment, such as:
>
> /*
> * Either sb_unit and hasdalign are both set, or they are zero
> * and not set, respectively.
> */
> if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
>
Ok, yet I think the comment might describe failure condition (which causes
-EFSCORRUPTED) instead directly, like,
/*
* 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, sbp->sb_unit, sbp->sb_width, 0))
> > + return -EFSCORRUPTED;
> >
> > if (xfs_sb_version_hascrc(&mp->m_sb) &&
> > sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> > @@ -1208,3 +1200,43 @@ xfs_sb_get_secondary(
> > *bpp = bp;
> > return 0;
> > }
> > +
> > +/*
> > + * If sectorsize is specified, sunit / swidth must be in bytes;
> > + * or both can be in any kind of units (e.g. 512B sector or blocksize).
> > + */
> > +bool
> > +xfs_validate_stripe_factors(
> > + struct xfs_mount *mp,
> > + int sunit,
> > + int swidth,
> > + int sectorsize)
> > +{
> > + if (sectorsize && sunit % sectorsize) {
> > + xfs_notice(mp,
> > +"stripe unit (%d) must be a multiple of the sector size (%d)",
> > + sunit, sectorsize);
> > + return false;
> > + }
> > +
> > + if ((sunit && !swidth) || (!sunit && swidth)) {
> > + xfs_notice(mp,
> > +"stripe unit (%d) and width (%d) are partially valid", sunit, swidth);
>
> I would break these into separate checks and messages.
Ok, will update in the next version.
>
> > + return false;
> > + }
> > +
> > + if (sunit > swidth) {
> > + xfs_notice(mp,
> > +"stripe unit (%d) is too large of the stripe width (%d)", sunit, swidth);
>
> "stripe unit (%d) is larger than the stripe width..."
Will update too.
Thanks,
Gao Xiang
>
> --D
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors()
2020-10-07 22:30 ` Darrick J. Wong
@ 2020-10-09 0:58 ` Gao Xiang
2020-10-09 13:02 ` Eric Sandeen
0 siblings, 1 reply; 12+ messages in thread
From: Gao Xiang @ 2020-10-09 0:58 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Gao Xiang, linux-xfs, Eric Sandeen, Dave Chinner
On Wed, Oct 07, 2020 at 03:30:44PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 07, 2020 at 10:04:02PM +0800, Gao Xiang wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> >
> > Check stripe numbers in calc_stripe_factors() by
> > using xfs_validate_stripe_factors().
> >
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> > mkfs/xfs_mkfs.c | 23 +++++++----------------
> > 1 file changed, 7 insertions(+), 16 deletions(-)
> >
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2e6cd280e388..b7f8f98147eb 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -2289,12 +2289,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) {
> > @@ -2306,13 +2300,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 (!xfs_validate_stripe_factors(NULL, BBTOB(dsunit), BBTOB(dswidth),
>
> if (!libxfs_validate_stripe_factors(...))
>
> Unless we get rid of the weird libxfs macro thing, you're supposed to
> use prefixes in userspace.
I vaguely remembered Christoph sent out a patch intending to get
rid of xfsprogs libxfs_ prefix months ago, so I assumed there was
no need to introduce any new libxfs_ userspace API wrappers anymore.
But yeah, will add such libxfs_ marco wrapper in the next version.
Thanks,
Gao Xiang
>
> --D
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk
2020-10-07 15:28 ` Darrick J. Wong
@ 2020-10-09 1:01 ` Gao Xiang
0 siblings, 0 replies; 12+ messages in thread
From: Gao Xiang @ 2020-10-09 1:01 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Gao Xiang, linux-xfs, Eric Sandeen, Dave Chinner, Dave Chinner
On Wed, Oct 07, 2020 at 08:28:00AM -0700, Darrick J. Wong wrote:
> On Wed, Oct 07, 2020 at 10:04:00PM +0800, Gao Xiang wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> >
> > In preparation to a common stripe validation helper,
> > allow i18n to xfs_{notice,warn,err,alert} so that
> > xfsprogs can share code with kernel.
> >
> > Suggested-by: Dave Chinner <dchinner@redhat.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 5688284deb4e..545a66dec9b8 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)
>
> AFAICT there isn't anything that passes a _() string to
> xfs_{alert,notice,warn,err} so this looks ok to me. It'll be nice to
> add the libxfs strings to the message catalogue at last...
>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Ok, assume that I don't have to update this patch
if my reading is correct. will resend this with
this RVB tag.
Thanks,
Gao Xiang
>
> --D
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors()
2020-10-09 0:58 ` Gao Xiang
@ 2020-10-09 13:02 ` Eric Sandeen
2020-10-09 13:50 ` Gao Xiang
0 siblings, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2020-10-09 13:02 UTC (permalink / raw)
To: Gao Xiang, Darrick J. Wong; +Cc: Gao Xiang, linux-xfs, Dave Chinner
On 10/8/20 7:58 PM, Gao Xiang wrote:
>> Unless we get rid of the weird libxfs macro thing, you're supposed to
>> use prefixes in userspace.
> I vaguely remembered Christoph sent out a patch intending to get
> rid of xfsprogs libxfs_ prefix months ago, so I assumed there was
> no need to introduce any new libxfs_ userspace API wrappers anymore.
He did, and it's on my (perpetual) TODO list to get that finally reviewed,
sorry.
For now we still have libxfs*
-Eric
> But yeah, will add such libxfs_ marco wrapper in the next version.
>
> Thanks,
> Gao Xiang
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors()
2020-10-09 13:02 ` Eric Sandeen
@ 2020-10-09 13:50 ` Gao Xiang
0 siblings, 0 replies; 12+ messages in thread
From: Gao Xiang @ 2020-10-09 13:50 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Darrick J. Wong, Gao Xiang, linux-xfs, Dave Chinner
On Fri, Oct 09, 2020 at 08:02:02AM -0500, Eric Sandeen wrote:
> On 10/8/20 7:58 PM, Gao Xiang wrote:
> >> Unless we get rid of the weird libxfs macro thing, you're supposed to
> >> use prefixes in userspace.
> > I vaguely remembered Christoph sent out a patch intending to get
> > rid of xfsprogs libxfs_ prefix months ago, so I assumed there was
> > no need to introduce any new libxfs_ userspace API wrappers anymore.
>
> He did, and it's on my (perpetual) TODO list to get that finally reviewed,
> sorry.
>
> For now we still have libxfs*
Yeah, I've sent out the next version with libxfs_ prefix at
https://lore.kernel.org/r/20201009052421.3328-4-hsiangkao@redhat.com
Thanks for the information as well!
Thanks,
Gao Xiang
>
> -Eric
>
> > But yeah, will add such libxfs_ marco wrapper in the next version.
> >
> > Thanks,
> > Gao Xiang
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-09 13:51 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20201007140402.14295-1-hsiangkao.ref@aol.com>
2020-10-07 14:03 ` [PATCH v4 0/3] xfsprogs: consolidate stripe validation Gao Xiang
2020-10-07 14:04 ` [PATCH v4 1/3] xfsprogs: allow i18n to xfs printk Gao Xiang
2020-10-07 15:28 ` Darrick J. Wong
2020-10-09 1:01 ` Gao Xiang
2020-10-07 14:04 ` [PATCH v4 2/3] xfs: introduce xfs_validate_stripe_factors() Gao Xiang
2020-10-07 22:29 ` Darrick J. Wong
2020-10-09 0:54 ` Gao Xiang
2020-10-07 14:04 ` [PATCH v4 3/3] xfsprogs: make use of xfs_validate_stripe_factors() Gao Xiang
2020-10-07 22:30 ` Darrick J. Wong
2020-10-09 0:58 ` Gao Xiang
2020-10-09 13:02 ` Eric Sandeen
2020-10-09 13:50 ` Gao Xiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.