* [PATCH 1/9] mkfs.xfs: add helper to parse command line options
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 2/9] mkfs.xfs: move dopts to struct mkfs_xfs_opts Luis R. Rodriguez
` (10 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
Command line arguments are verified using a scheme which
has been extended over the years. Making re-use of it however
is not so easy mostly because all variables required are local
on main() and assumptions of this for the verifiers.
To enable future use of the verifiers in other areas add helper
routine parse_subopts() which can be used to parse subopts, the
subopts must be set then in the struct mkfs_xfs_opts, and all we
pass is the value being parsed along with the subopt type.
In the future this work should also help enable moving a lot of
cruft on main() into helpers, given eventually we'll be able to
pass around all important parameters.
We start of with adding support for the b subopts.
Leave the globals as-is for now.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 167 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 93 insertions(+), 74 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index affa4052d62d..7ca972ee9675 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -708,6 +708,12 @@ struct opt_params mopts = {
},
};
+struct mkfs_xfs_opts {
+ int blocklog;
+ int blflag;
+ int bsflag;
+};
+
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
#define GIGABYTES(count, blog) ((__uint64_t)(count) << (30 - (blog)))
#define MEGABYTES(count, blog) ((__uint64_t)(count) << (20 - (blog)))
@@ -715,7 +721,7 @@ struct opt_params mopts = {
/*
* Use this macro before we have superblock and mount structure
*/
-#define DTOBT(d) ((xfs_rfsblock_t)((d) >> (blocklog - BBSHIFT)))
+#define DTOBT(d) ((xfs_rfsblock_t)((d) >> (params.blocklog - BBSHIFT)))
/*
* Use this for block reservations needed for mkfs's conditions
@@ -1393,6 +1399,38 @@ getstr(
return str;
}
+static void
+parse_subopts(
+ int type,
+ char *p,
+ struct mkfs_xfs_opts *params)
+{
+ char *value;
+
+ switch (type) {
+ case 'b':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)bopts.subopts, &value)) {
+ case B_LOG:
+ params->blocklog = getnum(value, &bopts, B_LOG);
+ blocksize = 1 << params->blocklog;
+ params->blflag = 1;
+ break;
+ case B_SIZE:
+ blocksize = getnum(value, &bopts, B_SIZE);
+ params->blocklog = libxfs_highbit32(blocksize);
+ params->bsflag = 1;
+ break;
+ default:
+ unknown('b', value);
+ }
+ }
+ break;
+ default:
+ usage();
+ }
+}
+
int
main(
int argc,
@@ -1405,9 +1443,6 @@ main(
__uint64_t agsize;
xfs_alloc_rec_t *arec;
struct xfs_btree_block *block;
- int blflag;
- int blocklog;
- int bsflag;
int bsize;
xfs_buf_t *buf;
int c;
@@ -1501,6 +1536,9 @@ main(
.rmapbt = false,
.reflink = false,
};
+ struct mkfs_xfs_opts params;
+
+ memset(¶ms, 0, sizeof(params));
platform_uuid_generate(&uuid);
progname = basename(argv[0]);
@@ -1508,8 +1546,8 @@ main(
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
- blflag = bsflag = slflag = ssflag = lslflag = lssflag = 0;
- blocklog = blocksize = 0;
+ slflag = ssflag = lslflag = lssflag = 0;
+ blocksize = 0;
sectorlog = lsectorlog = 0;
sectorsize = lsectorsize = 0;
agsize = daflag = dasize = dblocks = 0;
@@ -1541,26 +1579,7 @@ main(
break;
case 'b':
p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)bopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case B_LOG:
- blocklog = getnum(value, &bopts, B_LOG);
- blocksize = 1 << blocklog;
- blflag = 1;
- break;
- case B_SIZE:
- blocksize = getnum(value, &bopts,
- B_SIZE);
- blocklog = libxfs_highbit32(blocksize);
- bsflag = 1;
- break;
- default:
- unknown('b', value);
- }
- }
+ parse_subopts(c, p, ¶ms);
break;
case 'd':
p = optarg;
@@ -1941,8 +1960,8 @@ main(
* For RAID4/5/6 we want to align sector size and block size,
* so we need to start with the device geometry extraction too.
*/
- if (!blflag && !bsflag) {
- blocklog = XFS_DFL_BLOCKSIZE_LOG;
+ if (!params.blflag && !params.bsflag) {
+ params.blocklog = XFS_DFL_BLOCKSIZE_LOG;
blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG;
}
if (blocksize < XFS_MIN_BLOCKSIZE || blocksize > XFS_MAX_BLOCKSIZE) {
@@ -2159,7 +2178,7 @@ _("rmapbt not supported with realtime devices\n"));
if (blocksize < (1 << XFS_MIN_REC_DIRSIZE))
dirblocklog = XFS_MIN_REC_DIRSIZE;
else
- dirblocklog = blocklog;
+ dirblocklog = params.blocklog;
dirblocksize = 1 << dirblocklog;
}
@@ -2174,15 +2193,15 @@ _("rmapbt not supported with realtime devices\n"));
(long long)dbytes, XFS_MIN_BLOCKSIZE);
usage();
}
- dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
+ dblocks = (xfs_rfsblock_t)(dbytes >> params.blocklog);
if (dbytes % blocksize)
fprintf(stderr, _("warning: "
"data length %lld not a multiple of %d, truncated to %lld\n"),
(long long)dbytes, blocksize,
- (long long)(dblocks << blocklog));
+ (long long)(dblocks << params.blocklog));
}
if (ipflag) {
- inodelog = blocklog - libxfs_highbit32(inopblock);
+ inodelog = params.blocklog - libxfs_highbit32(inopblock);
isize = 1 << inodelog;
} else if (!ilflag && !isflag) {
inodelog = sb_feat.crcs_enabled ? XFS_DINODE_DFL_CRC_LOG
@@ -2206,12 +2225,12 @@ _("rmapbt not supported with realtime devices\n"));
(long long)logbytes, XFS_MIN_BLOCKSIZE);
usage();
}
- logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
+ logblocks = (xfs_rfsblock_t)(logbytes >> params.blocklog);
if (logbytes % blocksize)
fprintf(stderr,
_("warning: log length %lld not a multiple of %d, truncated to %lld\n"),
(long long)logbytes, blocksize,
- (long long)(logblocks << blocklog));
+ (long long)(logblocks << params.blocklog));
}
if (rtsize) {
__uint64_t rtbytes;
@@ -2223,12 +2242,12 @@ _("rmapbt not supported with realtime devices\n"));
(long long)rtbytes, XFS_MIN_BLOCKSIZE);
usage();
}
- rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
+ rtblocks = (xfs_rfsblock_t)(rtbytes >> params.blocklog);
if (rtbytes % blocksize)
fprintf(stderr,
_("warning: rt length %lld not a multiple of %d, truncated to %lld\n"),
(long long)rtbytes, blocksize,
- (long long)(rtblocks << blocklog));
+ (long long)(rtblocks << params.blocklog));
}
/*
* If specified, check rt extent size against its constraints.
@@ -2243,7 +2262,7 @@ _("rmapbt not supported with realtime devices\n"));
(long long)rtextbytes, blocksize);
usage();
}
- rtextblocks = (xfs_extlen_t)(rtextbytes >> blocklog);
+ rtextblocks = (xfs_extlen_t)(rtextbytes >> params.blocklog);
} else {
/*
* If realtime extsize has not been specified by the user,
@@ -2261,7 +2280,7 @@ _("rmapbt not supported with realtime devices\n"));
/* check that rswidth is a multiple of fs blocksize */
if (!norsflag && rswidth && !(BBTOB(rswidth) % blocksize)) {
rswidth = DTOBT(rswidth);
- rtextbytes = rswidth << blocklog;
+ rtextbytes = rswidth << params.blocklog;
if (XFS_MIN_RTEXTSIZE <= rtextbytes &&
(rtextbytes <= XFS_MAX_RTEXTSIZE)) {
rtextblocks = rswidth;
@@ -2269,7 +2288,7 @@ _("rmapbt not supported with realtime devices\n"));
}
if (!rtextblocks) {
rtextblocks = (blocksize < XFS_MIN_RTEXTSIZE) ?
- XFS_MIN_RTEXTSIZE >> blocklog : 1;
+ XFS_MIN_RTEXTSIZE >> params.blocklog : 1;
}
}
ASSERT(rtextblocks);
@@ -2472,7 +2491,7 @@ reported by the device (%u).\n"),
} else if (daflag) { /* User-specified AG count */
agsize = dblocks / agcount + (dblocks % agcount != 0);
} else {
- calc_default_ag_geometry(blocklog, dblocks,
+ calc_default_ag_geometry(params.blocklog, dblocks,
dsunit | dswidth, &agsize, &agcount);
}
@@ -2494,18 +2513,18 @@ reported by the device (%u).\n"),
/*
* Round up to stripe unit boundary. Also make sure
* that agsize is still larger than
- * XFS_AG_MIN_BLOCKS(blocklog)
+ * XFS_AG_MIN_BLOCKS(params.blocklog)
*/
tmp_agsize = ((agsize + (dsunit - 1))/ dsunit) * dsunit;
/*
* Round down to stripe unit boundary if rounding up
* created an AG size that is larger than the AG max.
*/
- if (tmp_agsize > XFS_AG_MAX_BLOCKS(blocklog))
+ if (tmp_agsize > XFS_AG_MAX_BLOCKS(params.blocklog))
tmp_agsize = ((agsize) / dsunit) * dsunit;
- if ((tmp_agsize >= XFS_AG_MIN_BLOCKS(blocklog)) &&
- (tmp_agsize <= XFS_AG_MAX_BLOCKS(blocklog))) {
+ if ((tmp_agsize >= XFS_AG_MIN_BLOCKS(params.blocklog)) &&
+ (tmp_agsize <= XFS_AG_MAX_BLOCKS(params.blocklog))) {
agsize = tmp_agsize;
if (!daflag)
agcount = dblocks/agsize +
@@ -2522,7 +2541,7 @@ reported by the device (%u).\n"),
* agsize is out of bounds, this will
* print nice details & exit.
*/
- validate_ag_geometry(blocklog, dblocks,
+ validate_ag_geometry(params.blocklog, dblocks,
agsize, agcount);
exit(1);
}
@@ -2535,7 +2554,7 @@ reported by the device (%u).\n"),
* does not happen.
*/
tmp_agsize = agsize - dsunit;
- if (tmp_agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
+ if (tmp_agsize < XFS_AG_MIN_BLOCKS(params.blocklog)) {
tmp_agsize = agsize + dsunit;
if (dblocks < agsize) {
/* oh well, nothing to do */
@@ -2557,7 +2576,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
*/
if ( dblocks % agsize != 0 &&
(dblocks % agsize <
- XFS_AG_MIN_BLOCKS(blocklog))) {
+ XFS_AG_MIN_BLOCKS(params.blocklog))) {
dblocks = (xfs_rfsblock_t)((agcount - 1) * agsize);
agcount--;
ASSERT(agcount != 0);
@@ -2582,17 +2601,17 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
* and drop the blocks.
*/
if ( dblocks % agsize != 0 &&
- (dblocks % agsize < XFS_AG_MIN_BLOCKS(blocklog))) {
+ (dblocks % agsize < XFS_AG_MIN_BLOCKS(params.blocklog))) {
ASSERT(!daflag);
dblocks = (xfs_rfsblock_t)((agcount - 1) * agsize);
agcount--;
ASSERT(agcount != 0);
}
- validate_ag_geometry(blocklog, dblocks, agsize, agcount);
+ validate_ag_geometry(params.blocklog, dblocks, agsize, agcount);
if (!imflag)
- imaxpct = calc_default_imaxpct(blocklog, dblocks);
+ imaxpct = calc_default_imaxpct(params.blocklog, dblocks);
/*
* check that log sunit is modulo fsblksize or default it to dsunit.
@@ -2615,18 +2634,18 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
fprintf(stderr,
_("log stripe unit adjusted to 32KiB\n"));
}
- lsunit = (32 * 1024) >> blocklog;
+ lsunit = (32 * 1024) >> params.blocklog;
}
min_logblocks = max_trans_res(agsize,
sb_feat.crcs_enabled, sb_feat.dir_version,
- sectorlog, blocklog, inodelog, dirblocklog,
+ sectorlog, params.blocklog, inodelog, dirblocklog,
sb_feat.log_version, lsunit, sb_feat.finobt,
sb_feat.rmapbt, sb_feat.reflink);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
- if (!logsize && dblocks >= (1024*1024*1024) >> blocklog)
- min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
+ if (!logsize && dblocks >= (1024*1024*1024) >> params.blocklog)
+ min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>params.blocklog);
if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
fprintf(stderr,
_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
@@ -2646,17 +2665,17 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
logblocks = 0;
} else if (loginternal && !logsize) {
- if (dblocks < GIGABYTES(1, blocklog)) {
+ if (dblocks < GIGABYTES(1, params.blocklog)) {
/* tiny filesystems get minimum sized logs. */
logblocks = min_logblocks;
- } else if (dblocks < GIGABYTES(16, blocklog)) {
+ } else if (dblocks < GIGABYTES(16, params.blocklog)) {
/*
* For small filesystems, we want to use the
* XFS_MIN_LOG_BYTES for filesystems smaller than 16G if
* at all possible, ramping up to 128MB at 256GB.
*/
- logblocks = MIN(XFS_MIN_LOG_BYTES >> blocklog,
+ logblocks = MIN(XFS_MIN_LOG_BYTES >> params.blocklog,
min_logblocks * XFS_DFL_LOG_FACTOR);
} else {
/*
@@ -2665,8 +2684,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* max log size of 128M at 256GB fs size. IOWs,
* the ratio of fs size to log size is 2048:1.
*/
- logblocks = (dblocks << blocklog) / 2048;
- logblocks = logblocks >> blocklog;
+ logblocks = (dblocks << params.blocklog) / 2048;
+ logblocks = logblocks >> params.blocklog;
}
/* Ensure the chosen size meets minimum log size requirements */
@@ -2678,18 +2697,18 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
/* and now clamp the size to the maximum supported size */
logblocks = MIN(logblocks, XFS_MAX_LOG_BLOCKS);
- if ((logblocks << blocklog) > XFS_MAX_LOG_BYTES)
- logblocks = XFS_MAX_LOG_BYTES >> blocklog;
+ if ((logblocks << params.blocklog) > XFS_MAX_LOG_BYTES)
+ logblocks = XFS_MAX_LOG_BYTES >> params.blocklog;
}
- validate_log_size(logblocks, blocklog, min_logblocks);
+ validate_log_size(logblocks, params.blocklog, min_logblocks);
protostring = setup_proto(protofile);
- bsize = 1 << (blocklog - BBSHIFT);
+ bsize = 1 << (params.blocklog - BBSHIFT);
mp = &mbuf;
sbp = &mp->m_sb;
memset(mp, 0, sizeof(xfs_mount_t));
- sbp->sb_blocklog = (__uint8_t)blocklog;
+ sbp->sb_blocklog = (__uint8_t)params.blocklog;
sbp->sb_sectlog = (__uint8_t)sectorlog;
sbp->sb_agblklog = (__uint8_t)libxfs_log2_roundup((unsigned int)agsize);
sbp->sb_agblocks = (xfs_agblock_t)agsize;
@@ -2713,7 +2732,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
libxfs_alloc_ag_max_usable(mp));
/* revalidate the log size is valid if we changed it */
- validate_log_size(logblocks, blocklog, min_logblocks);
+ validate_log_size(logblocks, params.blocklog, min_logblocks);
}
if (logblocks > agsize - libxfs_prealloc_blocks(mp)) {
fprintf(stderr,
@@ -2739,19 +2758,19 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
if (lsunit) {
logstart = fixup_internal_log_stripe(mp,
lsflag, logstart, agsize, lsunit,
- &logblocks, blocklog, &lalign);
+ &logblocks, params.blocklog, &lalign);
} else if (dsunit) {
logstart = fixup_internal_log_stripe(mp,
lsflag, logstart, agsize, dsunit,
- &logblocks, blocklog, &lalign);
+ &logblocks, params.blocklog, &lalign);
}
} else {
logstart = 0;
if (lsunit)
fixup_log_stripe_unit(lsflag, lsunit,
- &logblocks, blocklog);
+ &logblocks, params.blocklog);
}
- validate_log_size(logblocks, blocklog, min_logblocks);
+ validate_log_size(logblocks, params.blocklog, min_logblocks);
if (!qflag || Nflag) {
printf(_(
@@ -2773,10 +2792,10 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
"", dsunit, dswidth,
sb_feat.dir_version, dirblocksize, sb_feat.nci,
sb_feat.dirftype,
- logfile, 1 << blocklog, (long long)logblocks,
+ logfile, 1 << params.blocklog, (long long)logblocks,
sb_feat.log_version, "", lsectorsize, lsunit,
sb_feat.lazy_sb_counters,
- rtfile, rtextblocks << blocklog,
+ rtfile, rtextblocks << params.blocklog,
(long long)rtblocks, (long long)rtextents);
if (Nflag)
exit(0);
@@ -2803,7 +2822,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_inopblock = (__uint16_t)(blocksize / isize);
sbp->sb_sectlog = (__uint8_t)sectorlog;
sbp->sb_inodelog = (__uint8_t)inodelog;
- sbp->sb_inopblog = (__uint8_t)(blocklog - inodelog);
+ sbp->sb_inopblog = (__uint8_t)(params.blocklog - inodelog);
sbp->sb_rextslog =
(__uint8_t)(rtextents ?
libxfs_highbit32((unsigned int)rtextents) : 0);
@@ -2818,7 +2837,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_qflags = 0;
sbp->sb_unit = dsunit;
sbp->sb_width = dswidth;
- sbp->sb_dirblklog = dirblocklog - blocklog;
+ sbp->sb_dirblklog = dirblocklog - params.blocklog;
if (sb_feat.log_version == 2) { /* This is stored in bytes */
lsunit = (lsunit == 0) ? 1 : XFS_FSB_TO_B(mp, lsunit);
sbp->sb_logsunit = lsunit;
@@ -2828,7 +2847,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
int cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
if (sb_feat.crcs_enabled)
cluster_size *= isize / XFS_DINODE_MIN_SIZE;
- sbp->sb_inoalignmt = cluster_size >> blocklog;
+ sbp->sb_inoalignmt = cluster_size >> params.blocklog;
sb_feat.inode_align = sbp->sb_inoalignmt != 0;
} else
sbp->sb_inoalignmt = 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 2/9] mkfs.xfs: move dopts to struct mkfs_xfs_opts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 1/9] mkfs.xfs: add helper to parse command line options Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 3/9] mkfs.xfs: move iopts to " Luis R. Rodriguez
` (9 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
Following the logic, move all dopts into struct mkfs_xfs_opts to
enable re-parsing / resetting of the variables more easily later.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 616 ++++++++++++++++++++++++++++++--------------------------
1 file changed, 325 insertions(+), 291 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 7ca972ee9675..868cab2164d6 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -712,6 +712,22 @@ struct mkfs_xfs_opts {
int blocklog;
int blflag;
int bsflag;
+
+ struct fsxattr fsx;
+ libxfs_init_t xi;
+ __uint64_t agcount;
+ __uint64_t agsize;
+ int daflag;
+ int ssflag;
+ int dasize;
+ char *dsize;
+ int dsunit;
+ int dswidth;
+ int dsu;
+ int dsw;
+ int nodsflag;
+ int sectorlog;
+ int slflag;
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -1406,6 +1422,7 @@ parse_subopts(
struct mkfs_xfs_opts *params)
{
char *value;
+ int c;
switch (type) {
case 'b':
@@ -1426,6 +1443,81 @@ parse_subopts(
}
}
break;
+ case 'd':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)dopts.subopts, &value)) {
+ case D_AGCOUNT:
+ params->agcount = getnum(value, &dopts,
+ D_AGCOUNT);
+ params->daflag = 1;
+ break;
+ case D_AGSIZE:
+ params->agsize = getnum(value, &dopts, D_AGSIZE);
+ params->dasize = 1;
+ break;
+ case D_FILE:
+ params->xi.disfile = getnum(value, &dopts,
+ D_FILE);
+ break;
+ case D_NAME:
+ params->xi.dname = getstr(value, &dopts, D_NAME);
+ break;
+ case D_SIZE:
+ params->dsize = getstr(value, &dopts, D_SIZE);
+ break;
+ case D_SUNIT:
+ params->dsunit = getnum(value, &dopts, D_SUNIT);
+ break;
+ case D_SWIDTH:
+ params->dswidth = getnum(value, &dopts,
+ D_SWIDTH);
+ break;
+ case D_SU:
+ params->dsu = getnum(value, &dopts, D_SU);
+ break;
+ case D_SW:
+ params->dsw = getnum(value, &dopts, D_SW);
+ break;
+ case D_NOALIGN:
+ params->nodsflag = getnum(value, &dopts,
+ D_NOALIGN);
+ break;
+ case D_SECTLOG:
+ params->sectorlog = getnum(value, &dopts,
+ D_SECTLOG);
+ sectorsize = 1 << params->sectorlog;
+ params->slflag = 1;
+ break;
+ case D_SECTSIZE:
+ sectorsize = getnum(value, &dopts,
+ D_SECTSIZE);
+ params->sectorlog =
+ libxfs_highbit32(sectorsize);
+ params->ssflag = 1;
+ break;
+ case D_RTINHERIT:
+ c = getnum(value, &dopts, D_RTINHERIT);
+ if (c)
+ params->fsx.fsx_xflags |=
+ XFS_DIFLAG_RTINHERIT;
+ break;
+ case D_PROJINHERIT:
+ params->fsx.fsx_projid = getnum(value, &dopts,
+ D_PROJINHERIT);
+ params->fsx.fsx_xflags |=
+ XFS_DIFLAG_PROJINHERIT;
+ break;
+ case D_EXTSZINHERIT:
+ params->fsx.fsx_extsize = getnum(value, &dopts,
+ D_EXTSZINHERIT);
+ params->fsx.fsx_xflags |=
+ XFS_DIFLAG_EXTSZINHERIT;
+ break;
+ default:
+ unknown('d', value);
+ }
+ }
+ break;
default:
usage();
}
@@ -1436,29 +1528,19 @@ main(
int argc,
char **argv)
{
- __uint64_t agcount;
xfs_agf_t *agf;
xfs_agi_t *agi;
xfs_agnumber_t agno;
- __uint64_t agsize;
xfs_alloc_rec_t *arec;
struct xfs_btree_block *block;
int bsize;
xfs_buf_t *buf;
int c;
- int daflag;
- int dasize;
xfs_rfsblock_t dblocks;
char *dfile;
int dirblocklog;
int dirblocksize;
- char *dsize;
- int dsu;
- int dsw;
- int dsunit;
- int dswidth;
int force_overwrite;
- struct fsxattr fsx;
int ilflag;
int imaxpct;
int imflag;
@@ -1493,7 +1575,6 @@ main(
xfs_mount_t mbuf;
xfs_extlen_t nbmblocks;
int nlflag;
- int nodsflag;
int norsflag;
xfs_alloc_rec_t *nrec;
int nsflag;
@@ -1511,14 +1592,10 @@ main(
char *rtfile;
char *rtsize;
xfs_sb_t *sbp;
- int sectorlog;
__uint64_t sector_mask;
- int slflag;
- int ssflag;
__uint64_t tmp_agsize;
uuid_t uuid;
int worst_freelist;
- libxfs_init_t xi;
struct fs_topology ft;
struct sb_feat_args sb_feat = {
.finobt = 1,
@@ -1546,11 +1623,11 @@ main(
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
- slflag = ssflag = lslflag = lssflag = 0;
+ lslflag = lssflag = 0;
blocksize = 0;
- sectorlog = lsectorlog = 0;
+ lsectorlog = 0;
sectorsize = lsectorsize = 0;
- agsize = daflag = dasize = dblocks = 0;
+ dblocks = 0;
ilflag = imflag = ipflag = isflag = 0;
liflag = laflag = lsflag = lsuflag = lsunitflag = ldflag = lvflag = 0;
loginternal = 1;
@@ -1560,16 +1637,14 @@ main(
qflag = 0;
imaxpct = inodelog = inopblock = isize = 0;
dfile = logfile = rtfile = NULL;
- dsize = logsize = rtsize = rtextsize = protofile = NULL;
- dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
- nodsflag = norsflag = 0;
+ logsize = rtsize = rtextsize = protofile = NULL;
+ lalign = lsu = lsunit = 0;
+ norsflag = 0;
force_overwrite = 0;
worst_freelist = 0;
- memset(&fsx, 0, sizeof(fsx));
- memset(&xi, 0, sizeof(xi));
- xi.isdirect = LIBXFS_DIRECT;
- xi.isreadonly = LIBXFS_EXCLUSIVELY;
+ params.xi.isdirect = LIBXFS_DIRECT;
+ params.xi.isreadonly = LIBXFS_EXCLUSIVELY;
while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
switch (c) {
@@ -1578,87 +1653,9 @@ main(
force_overwrite = 1;
break;
case 'b':
- p = optarg;
- parse_subopts(c, p, ¶ms);
- break;
case 'd':
p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)dopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case D_AGCOUNT:
- agcount = getnum(value, &dopts,
- D_AGCOUNT);
- daflag = 1;
- break;
- case D_AGSIZE:
- agsize = getnum(value, &dopts, D_AGSIZE);
- dasize = 1;
- break;
- case D_FILE:
- xi.disfile = getnum(value, &dopts,
- D_FILE);
- break;
- case D_NAME:
- xi.dname = getstr(value, &dopts, D_NAME);
- break;
- case D_SIZE:
- dsize = getstr(value, &dopts, D_SIZE);
- break;
- case D_SUNIT:
- dsunit = getnum(value, &dopts, D_SUNIT);
- break;
- case D_SWIDTH:
- dswidth = getnum(value, &dopts,
- D_SWIDTH);
- break;
- case D_SU:
- dsu = getnum(value, &dopts, D_SU);
- break;
- case D_SW:
- dsw = getnum(value, &dopts, D_SW);
- break;
- case D_NOALIGN:
- nodsflag = getnum(value, &dopts,
- D_NOALIGN);
- break;
- case D_SECTLOG:
- sectorlog = getnum(value, &dopts,
- D_SECTLOG);
- sectorsize = 1 << sectorlog;
- slflag = 1;
- break;
- case D_SECTSIZE:
- sectorsize = getnum(value, &dopts,
- D_SECTSIZE);
- sectorlog =
- libxfs_highbit32(sectorsize);
- ssflag = 1;
- break;
- case D_RTINHERIT:
- c = getnum(value, &dopts, D_RTINHERIT);
- if (c)
- fsx.fsx_xflags |=
- XFS_DIFLAG_RTINHERIT;
- break;
- case D_PROJINHERIT:
- fsx.fsx_projid = getnum(value, &dopts,
- D_PROJINHERIT);
- fsx.fsx_xflags |=
- XFS_DIFLAG_PROJINHERIT;
- break;
- case D_EXTSZINHERIT:
- fsx.fsx_extsize = getnum(value, &dopts,
- D_EXTSZINHERIT);
- fsx.fsx_xflags |=
- XFS_DIFLAG_EXTSZINHERIT;
- break;
- default:
- unknown('d', value);
- }
- }
+ parse_subopts(c, p, ¶ms);
break;
case 'i':
p = optarg;
@@ -1721,7 +1718,7 @@ main(
laflag = 1;
break;
case L_FILE:
- xi.lisfile = getnum(value, &lopts,
+ params.xi.lisfile = getnum(value, &lopts,
L_FILE);
break;
case L_INTERNAL:
@@ -1740,7 +1737,7 @@ main(
case L_NAME:
case L_DEV:
logfile = getstr(value, &lopts, L_NAME);
- xi.logname = logfile;
+ params.xi.logname = logfile;
ldflag = 1;
loginternal = 0;
break;
@@ -1883,12 +1880,12 @@ main(
R_EXTSIZE);
break;
case R_FILE:
- xi.risfile = getnum(value, &ropts,
+ params.xi.risfile = getnum(value, &ropts,
R_FILE);
break;
case R_NAME:
case R_DEV:
- xi.rtname = getstr(value, &ropts,
+ params.xi.rtname = getstr(value, &ropts,
R_NAME);
break;
case R_SIZE:
@@ -1915,12 +1912,12 @@ main(
if (lssflag)
conflict('s', subopts,
S_SECTSIZE, S_SECTLOG);
- sectorlog = getnum(value, &sopts,
+ params.sectorlog = getnum(value, &sopts,
S_SECTLOG);
- lsectorlog = sectorlog;
- sectorsize = 1 << sectorlog;
+ lsectorlog = params.sectorlog;
+ sectorsize = 1 << params.sectorlog;
lsectorsize = sectorsize;
- lslflag = slflag = 1;
+ lslflag = params.slflag = 1;
break;
case S_SIZE:
case S_SECTSIZE:
@@ -1930,10 +1927,10 @@ main(
sectorsize = getnum(value, &sopts,
S_SECTSIZE);
lsectorsize = sectorsize;
- sectorlog =
+ params.sectorlog =
libxfs_highbit32(sectorsize);
- lsectorlog = sectorlog;
- lssflag = ssflag = 1;
+ lsectorlog = params.sectorlog;
+ lssflag = params.ssflag = 1;
break;
default:
unknown('s', value);
@@ -1951,9 +1948,10 @@ main(
fprintf(stderr, _("extra arguments\n"));
usage();
} else if (argc - optind == 1) {
- dfile = xi.volname = getstr(argv[optind], &dopts, D_NAME);
+ dfile = params.xi.volname =
+ getstr(argv[optind], &dopts, D_NAME);
} else
- dfile = xi.dname;
+ dfile = params.xi.dname;
/*
* Blocksize and sectorsize first, other things depend on them
@@ -1979,12 +1977,12 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
usage();
}
- if (!slflag && !ssflag) {
- sectorlog = XFS_MIN_SECTORSIZE_LOG;
+ if (!params.slflag && !params.ssflag) {
+ params.sectorlog = XFS_MIN_SECTORSIZE_LOG;
sectorsize = XFS_MIN_SECTORSIZE;
}
if (!lslflag && !lssflag) {
- lsectorlog = sectorlog;
+ lsectorlog = params.sectorlog;
lsectorsize = sectorsize;
}
@@ -1995,23 +1993,30 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
* sector size mismatches between the new filesystem and the underlying
* host filesystem.
*/
- check_device_type(dfile, &xi.disfile, !dsize, !dfile,
- Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
+ check_device_type(dfile, ¶ms.xi.disfile, !params.dsize, !dfile,
+ Nflag ? NULL : ¶ms.xi.dcreat,
+ force_overwrite, "d");
if (!loginternal)
- check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
- Nflag ? NULL : &xi.lcreat,
+ check_device_type(params.xi.logname,
+ ¶ms.xi.lisfile,
+ !logsize,
+ !params.xi.logname,
+ Nflag ? NULL : ¶ms.xi.lcreat,
force_overwrite, "l");
- if (xi.rtname)
- check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
- Nflag ? NULL : &xi.rcreat,
+ if (params.xi.rtname)
+ check_device_type(params.xi.rtname,
+ ¶ms.xi.risfile,
+ !rtsize,
+ !params.xi.rtname,
+ Nflag ? NULL : ¶ms.xi.rcreat,
force_overwrite, "r");
- if (xi.disfile || xi.lisfile || xi.risfile)
- xi.isdirect = 0;
+ if (params.xi.disfile || params.xi.lisfile || params.xi.risfile)
+ params.xi.isdirect = 0;
memset(&ft, 0, sizeof(ft));
- get_topology(&xi, &ft, force_overwrite);
+ get_topology(¶ms.xi, &ft, force_overwrite);
- if (!ssflag) {
+ if (!params.ssflag) {
/*
* Unless specified manually on the command line use the
* advertised sector size of the device. We use the physical
@@ -2039,17 +2044,17 @@ _("switching to logical sector size %d\n"),
}
}
- if (!ssflag) {
- sectorlog = libxfs_highbit32(sectorsize);
+ if (!params.ssflag) {
+ params.sectorlog = libxfs_highbit32(sectorsize);
if (loginternal) {
lsectorsize = sectorsize;
- lsectorlog = sectorlog;
+ lsectorlog = params.sectorlog;
}
}
if (sectorsize < XFS_MIN_SECTORSIZE ||
sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
- if (ssflag)
+ if (params.ssflag)
fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
else
fprintf(stderr,
@@ -2160,7 +2165,7 @@ _("reflink not supported without CRC support\n"));
}
- if (sb_feat.rmapbt && xi.rtname) {
+ if (sb_feat.rmapbt && params.xi.rtname) {
fprintf(stderr,
_("rmapbt not supported with realtime devices\n"));
usage();
@@ -2183,10 +2188,10 @@ _("rmapbt not supported with realtime devices\n"));
}
- if (dsize) {
+ if (params.dsize) {
__uint64_t dbytes;
- dbytes = getnum(dsize, &dopts, D_SIZE);
+ dbytes = getnum(params.dsize, &dopts, D_SIZE);
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal data length %lld, not a multiple of %d\n"),
@@ -2272,7 +2277,8 @@ _("rmapbt not supported with realtime devices\n"));
__uint64_t rswidth;
__uint64_t rtextbytes;
- if (!norsflag && !xi.risfile && !(!rtsize && xi.disfile))
+ if (!norsflag && !params.xi.risfile &&
+ !(!rtsize && params.xi.disfile))
rswidth = ft.rtswidth;
else
rswidth = 0;
@@ -2322,17 +2328,17 @@ _("rmapbt not supported with realtime devices\n"));
sb_feat.log_version = 2;
}
- calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
- &dsunit, &dswidth, &lsunit);
+ calc_stripe_factors(params.dsu, params.dsw, sectorsize, lsu, lsectorsize,
+ ¶ms.dsunit, ¶ms.dswidth, &lsunit);
- xi.setblksize = sectorsize;
+ params.xi.setblksize = sectorsize;
/*
* Initialize. This will open the log and rt devices as well.
*/
- if (!libxfs_init(&xi))
+ if (!libxfs_init(¶ms.xi))
usage();
- if (!xi.ddev) {
+ if (!params.xi.ddev) {
fprintf(stderr, _("no device name given in argument list\n"));
usage();
}
@@ -2348,50 +2354,52 @@ _("rmapbt not supported with realtime devices\n"));
* multiple of the sector size, or 1024, whichever is larger.
*/
- sector_mask = (__uint64_t)-1 << (MAX(sectorlog, 10) - BBSHIFT);
- xi.dsize &= sector_mask;
- xi.rtsize &= sector_mask;
- xi.logBBsize &= (__uint64_t)-1 << (MAX(lsectorlog, 10) - BBSHIFT);
+ sector_mask = (__uint64_t)-1 << (MAX(params.sectorlog, 10) - BBSHIFT);
+ params.xi.dsize &= sector_mask;
+ params.xi.rtsize &= sector_mask;
+ params.xi.logBBsize &= (__uint64_t)-1 << (MAX(lsectorlog, 10) - BBSHIFT);
/* don't do discards on print-only runs or on files */
if (discard && !Nflag) {
- if (!xi.disfile)
- discard_blocks(xi.ddev, xi.dsize);
- if (xi.rtdev && !xi.risfile)
- discard_blocks(xi.rtdev, xi.rtsize);
- if (xi.logdev && xi.logdev != xi.ddev && !xi.lisfile)
- discard_blocks(xi.logdev, xi.logBBsize);
+ if (!params.xi.disfile)
+ discard_blocks(params.xi.ddev, params.xi.dsize);
+ if (params.xi.rtdev && !params.xi.risfile)
+ discard_blocks(params.xi.rtdev, params.xi.rtsize);
+ if (params.xi.logdev && params.xi.logdev != params.xi.ddev &&
+ !params.xi.lisfile)
+ discard_blocks(params.xi.logdev, params.xi.logBBsize);
}
if (!liflag && !ldflag)
- loginternal = xi.logdev == 0;
- if (xi.logname)
- logfile = xi.logname;
+ loginternal = params.xi.logdev == 0;
+ if (params.xi.logname)
+ logfile = params.xi.logname;
else if (loginternal)
logfile = _("internal log");
- else if (xi.volname && xi.logdev)
+ else if (params.xi.volname && params.xi.logdev)
logfile = _("volume log");
else if (!ldflag) {
fprintf(stderr, _("no log subvolume or internal log\n"));
usage();
}
- if (xi.rtname)
- rtfile = xi.rtname;
+ if (params.xi.rtname)
+ rtfile = params.xi.rtname;
else
- if (xi.volname && xi.rtdev)
+ if (params.xi.volname && params.xi.rtdev)
rtfile = _("volume rt");
- else if (!xi.rtdev)
+ else if (!params.xi.rtdev)
rtfile = _("none");
- if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
+ if (params.dsize && params.xi.dsize > 0 &&
+ dblocks > DTOBT(params.xi.dsize)) {
fprintf(stderr,
_("size %s specified for data subvolume is too large, "
"maximum is %lld blocks\n"),
- dsize, (long long)DTOBT(xi.dsize));
+ params.dsize, (long long)DTOBT(params.xi.dsize));
usage();
- } else if (!dsize && xi.dsize > 0)
- dblocks = DTOBT(xi.dsize);
- else if (!dsize) {
+ } else if (!params.dsize && params.xi.dsize > 0)
+ dblocks = DTOBT(params.xi.dsize);
+ else if (!params.dsize) {
fprintf(stderr, _("can't get size of data subvolume\n"));
usage();
}
@@ -2402,7 +2410,7 @@ _("rmapbt not supported with realtime devices\n"));
usage();
}
- if (loginternal && xi.logdev) {
+ if (loginternal && params.xi.logdev) {
fprintf(stderr,
_("can't have both external and internal logs\n"));
usage();
@@ -2412,39 +2420,39 @@ _("rmapbt not supported with realtime devices\n"));
usage();
}
- if (xi.dbsize > sectorsize) {
+ if (params.xi.dbsize > sectorsize) {
fprintf(stderr, _(
"Warning: the data subvolume sector size %u is less than the sector size \n\
reported by the device (%u).\n"),
- sectorsize, xi.dbsize);
+ sectorsize, params.xi.dbsize);
}
- if (!loginternal && xi.lbsize > lsectorsize) {
+ if (!loginternal && params.xi.lbsize > lsectorsize) {
fprintf(stderr, _(
"Warning: the log subvolume sector size %u is less than the sector size\n\
reported by the device (%u).\n"),
- lsectorsize, xi.lbsize);
+ lsectorsize, params.xi.lbsize);
}
- if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
+ if (rtsize && params.xi.rtsize > 0 && params.xi.rtbsize > sectorsize) {
fprintf(stderr, _(
"Warning: the realtime subvolume sector size %u is less than the sector size\n\
reported by the device (%u).\n"),
- sectorsize, xi.rtbsize);
+ sectorsize, params.xi.rtbsize);
}
- if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
+ if (rtsize && params.xi.rtsize > 0 && rtblocks > DTOBT(params.xi.rtsize)) {
fprintf(stderr,
_("size %s specified for rt subvolume is too large, "
"maximum is %lld blocks\n"),
- rtsize, (long long)DTOBT(xi.rtsize));
+ rtsize, (long long)DTOBT(params.xi.rtsize));
usage();
- } else if (!rtsize && xi.rtsize > 0)
- rtblocks = DTOBT(xi.rtsize);
- else if (rtsize && !xi.rtdev) {
+ } else if (!rtsize && params.xi.rtsize > 0)
+ rtblocks = DTOBT(params.xi.rtsize);
+ else if (rtsize && !params.xi.rtdev) {
fprintf(stderr,
_("size specified for non-existent rt subvolume\n"));
usage();
}
- if (xi.rtdev) {
+ if (params.xi.rtdev) {
rtextents = rtblocks / rtextblocks;
nbmblocks = (xfs_extlen_t)howmany(rtextents, NBBY * blocksize);
} else {
@@ -2452,145 +2460,158 @@ reported by the device (%u).\n"),
nbmblocks = 0;
}
- if (!nodsflag) {
- if (dsunit) {
- if (ft.dsunit && ft.dsunit != dsunit) {
+ if (!params.nodsflag) {
+ if (params.dsunit) {
+ if (ft.dsunit && ft.dsunit != params.dsunit) {
fprintf(stderr,
_("%s: Specified data stripe unit %d "
"is not the same as the volume stripe "
"unit %d\n"),
- progname, dsunit, ft.dsunit);
+ progname, params.dsunit, ft.dsunit);
}
- if (ft.dswidth && ft.dswidth != dswidth) {
+ if (ft.dswidth && ft.dswidth != params.dswidth) {
fprintf(stderr,
_("%s: Specified data stripe width %d "
"is not the same as the volume stripe "
"width %d\n"),
- progname, dswidth, ft.dswidth);
+ progname, params.dswidth, ft.dswidth);
}
} else {
- dsunit = ft.dsunit;
- dswidth = ft.dswidth;
- nodsflag = 1;
+ params.dsunit = ft.dsunit;
+ params.dswidth = ft.dswidth;
+ params.nodsflag = 1;
}
- } /* else dsunit & dswidth can't be set if nodsflag is set */
+ } /* else dsunit & dswidth can't be set if params.nodsflag is set */
- if (dasize) { /* User-specified AG size */
+ if (params.dasize) { /* User-specified AG size */
/*
* Check specified agsize is a multiple of blocksize.
*/
- if (agsize % blocksize) {
+ if (params.agsize % blocksize) {
fprintf(stderr,
_("agsize (%lld) not a multiple of fs blk size (%d)\n"),
- (long long)agsize, blocksize);
+ (long long)params.agsize, blocksize);
usage();
}
- agsize /= blocksize;
- agcount = dblocks / agsize + (dblocks % agsize != 0);
+ params.agsize /= blocksize;
+ params.agcount = dblocks / params.agsize +
+ (dblocks % params.agsize != 0);
- } else if (daflag) { /* User-specified AG count */
- agsize = dblocks / agcount + (dblocks % agcount != 0);
+ } else if (params.daflag) { /* User-specified AG count */
+ params.agsize = dblocks / params.agcount +
+ (dblocks % params.agcount != 0);
} else {
- calc_default_ag_geometry(params.blocklog, dblocks,
- dsunit | dswidth, &agsize, &agcount);
+ calc_default_ag_geometry(params.blocklog,
+ dblocks,
+ params.dsunit | params.dswidth,
+ ¶ms.agsize,
+ ¶ms.agcount);
}
/*
* If dsunit is a multiple of fs blocksize, then check that is a
* multiple of the agsize too
*/
- if (dsunit && !(BBTOB(dsunit) % blocksize) &&
- dswidth && !(BBTOB(dswidth) % blocksize)) {
+ if (params.dsunit && !(BBTOB(params.dsunit) % blocksize) &&
+ params.dswidth && !(BBTOB(params.dswidth) % blocksize)) {
/* convert from 512 byte blocks to fs blocksize */
- dsunit = DTOBT(dsunit);
- dswidth = DTOBT(dswidth);
+ params.dsunit = DTOBT(params.dsunit);
+ params.dswidth = DTOBT(params.dswidth);
/*
* agsize is not a multiple of dsunit
*/
- if ((agsize % dsunit) != 0) {
+ if ((params.agsize % params.dsunit) != 0) {
/*
* Round up to stripe unit boundary. Also make sure
* that agsize is still larger than
* XFS_AG_MIN_BLOCKS(params.blocklog)
*/
- tmp_agsize = ((agsize + (dsunit - 1))/ dsunit) * dsunit;
+ tmp_agsize = ((params.agsize +
+ (params.dsunit - 1)) /
+ params.dsunit) * params.dsunit;
/*
* Round down to stripe unit boundary if rounding up
* created an AG size that is larger than the AG max.
*/
if (tmp_agsize > XFS_AG_MAX_BLOCKS(params.blocklog))
- tmp_agsize = ((agsize) / dsunit) * dsunit;
+ tmp_agsize = ((params.agsize) / params.dsunit)
+ * params.dsunit;
if ((tmp_agsize >= XFS_AG_MIN_BLOCKS(params.blocklog)) &&
(tmp_agsize <= XFS_AG_MAX_BLOCKS(params.blocklog))) {
- agsize = tmp_agsize;
- if (!daflag)
- agcount = dblocks/agsize +
- (dblocks % agsize != 0);
- if (dasize)
+ params.agsize = tmp_agsize;
+ if (!params.daflag)
+ params.agcount = dblocks/params.agsize +
+ (dblocks % params.agsize != 0);
+ if (params.dasize)
fprintf(stderr,
_("agsize rounded to %lld, swidth = %d\n"),
- (long long)agsize, dswidth);
+ (long long)params.agsize,
+ params.dswidth);
} else {
- if (nodsflag) {
- dsunit = dswidth = 0;
+ if (params.nodsflag) {
+ params.dsunit = params.dswidth = 0;
} else {
/*
* agsize is out of bounds, this will
* print nice details & exit.
*/
validate_ag_geometry(params.blocklog, dblocks,
- agsize, agcount);
+ params.agsize, params.agcount);
exit(1);
}
}
}
- if (dswidth && ((agsize % dswidth) == 0) && (agcount > 1)) {
+ if (params.dswidth &&
+ ((params.agsize % params.dswidth) == 0) &&
+ (params.agcount > 1)) {
/* This is a non-optimal configuration because all AGs
* start on the same disk in the stripe. Changing
* the AG size by one sunit will guarantee that this
* does not happen.
*/
- tmp_agsize = agsize - dsunit;
+ tmp_agsize = params.agsize - params.dsunit;
if (tmp_agsize < XFS_AG_MIN_BLOCKS(params.blocklog)) {
- tmp_agsize = agsize + dsunit;
- if (dblocks < agsize) {
+ tmp_agsize = params.agsize + params.dsunit;
+ if (dblocks < params.agsize) {
/* oh well, nothing to do */
- tmp_agsize = agsize;
+ tmp_agsize = params.agsize;
}
}
- if (daflag || dasize) {
+ if (params.daflag || params.dasize) {
fprintf(stderr, _(
"Warning: AG size is a multiple of stripe width. This can cause performance\n\
problems by aligning all AGs on the same disk. To avoid this, run mkfs with\n\
an AG size that is one stripe unit smaller, for example %llu.\n"),
(unsigned long long)tmp_agsize);
} else {
- agsize = tmp_agsize;
- agcount = dblocks/agsize + (dblocks % agsize != 0);
+ params.agsize = tmp_agsize;
+ params.agcount = dblocks/params.agsize +
+ (dblocks % params.agsize != 0);
/*
* If the last AG is too small, reduce the
* filesystem size and drop the blocks.
*/
- if ( dblocks % agsize != 0 &&
- (dblocks % agsize <
+ if ( dblocks % params.agsize != 0 &&
+ (dblocks % params.agsize <
XFS_AG_MIN_BLOCKS(params.blocklog))) {
- dblocks = (xfs_rfsblock_t)((agcount - 1) * agsize);
- agcount--;
- ASSERT(agcount != 0);
+ dblocks = (xfs_rfsblock_t)
+ ((params.agcount - 1) * params.agsize);
+ params.agcount--;
+ ASSERT(params.agcount != 0);
}
}
}
} else {
- if (nodsflag)
- dsunit = dswidth = 0;
+ if (params.nodsflag)
+ params.dsunit = params.dswidth = 0;
else {
fprintf(stderr,
_("%s: Stripe unit(%d) or stripe width(%d) is "
"not a multiple of the block size(%d)\n"),
- progname, BBTOB(dsunit), BBTOB(dswidth),
+ progname, BBTOB(params.dsunit), BBTOB(params.dswidth),
blocksize);
exit(1);
}
@@ -2600,15 +2621,15 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
* If the last AG is too small, reduce the filesystem size
* and drop the blocks.
*/
- if ( dblocks % agsize != 0 &&
- (dblocks % agsize < XFS_AG_MIN_BLOCKS(params.blocklog))) {
- ASSERT(!daflag);
- dblocks = (xfs_rfsblock_t)((agcount - 1) * agsize);
- agcount--;
- ASSERT(agcount != 0);
+ if ( dblocks % params.agsize != 0 &&
+ (dblocks % params.agsize < XFS_AG_MIN_BLOCKS(params.blocklog))) {
+ ASSERT(!params.daflag);
+ dblocks = (xfs_rfsblock_t)((params.agcount - 1) * params.agsize);
+ params.agcount--;
+ ASSERT(params.agcount != 0);
}
- validate_ag_geometry(params.blocklog, dblocks, agsize, agcount);
+ validate_ag_geometry(params.blocklog, dblocks, params.agsize, params.agcount);
if (!imflag)
imaxpct = calc_default_imaxpct(params.blocklog, dblocks);
@@ -2620,9 +2641,9 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
if (lsunit) {
/* convert from 512 byte blocks to fs blocks */
lsunit = DTOBT(lsunit);
- } else if (sb_feat.log_version == 2 && loginternal && dsunit) {
+ } else if (sb_feat.log_version == 2 && loginternal && params.dsunit) {
/* lsunit and dsunit now in fs blocks */
- lsunit = dsunit;
+ lsunit = params.dsunit;
}
if (sb_feat.log_version == 2 && (lsunit * blocksize) > 256 * 1024) {
@@ -2637,23 +2658,25 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
lsunit = (32 * 1024) >> params.blocklog;
}
- min_logblocks = max_trans_res(agsize,
+ min_logblocks = max_trans_res(params.agsize,
sb_feat.crcs_enabled, sb_feat.dir_version,
- sectorlog, params.blocklog, inodelog, dirblocklog,
+ params.sectorlog, params.blocklog,
+ inodelog, dirblocklog,
sb_feat.log_version, lsunit, sb_feat.finobt,
sb_feat.rmapbt, sb_feat.reflink);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
if (!logsize && dblocks >= (1024*1024*1024) >> params.blocklog)
min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>params.blocklog);
- if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
+ if (logsize && params.xi.logBBsize > 0 &&
+ logblocks > DTOBT(params.xi.logBBsize)) {
fprintf(stderr,
_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
- logsize, (long long)DTOBT(xi.logBBsize));
+ logsize, (long long)DTOBT(params.xi.logBBsize));
usage();
- } else if (!logsize && xi.logBBsize > 0) {
- logblocks = DTOBT(xi.logBBsize);
- } else if (logsize && !xi.logdev && !loginternal) {
+ } else if (!logsize && params.xi.logBBsize > 0) {
+ logblocks = DTOBT(params.xi.logBBsize);
+ } else if (logsize && !params.xi.logdev && !loginternal) {
fprintf(stderr,
_("size specified for non-existent log subvolume\n"));
usage();
@@ -2661,7 +2684,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
fprintf(stderr, _("size %lld too large for internal log\n"),
(long long)logblocks);
usage();
- } else if (!loginternal && !xi.logdev) {
+ } else if (!loginternal && !params.xi.logdev) {
logblocks = 0;
} else if (loginternal && !logsize) {
@@ -2692,7 +2715,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
logblocks = MAX(min_logblocks, logblocks);
/* make sure the log fits wholly within an AG */
- if (logblocks >= agsize)
+ if (logblocks >= params.agsize)
logblocks = min_logblocks;
/* and now clamp the size to the maximum supported size */
@@ -2709,9 +2732,10 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp = &mp->m_sb;
memset(mp, 0, sizeof(xfs_mount_t));
sbp->sb_blocklog = (__uint8_t)params.blocklog;
- sbp->sb_sectlog = (__uint8_t)sectorlog;
- sbp->sb_agblklog = (__uint8_t)libxfs_log2_roundup((unsigned int)agsize);
- sbp->sb_agblocks = (xfs_agblock_t)agsize;
+ sbp->sb_sectlog = (__uint8_t)params.sectorlog;
+ sbp->sb_agblklog =
+ (__uint8_t)libxfs_log2_roundup((unsigned int)params.agsize);
+ sbp->sb_agblocks = (xfs_agblock_t)params.agsize;
mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;
mp->m_sectbb_log = sbp->sb_sectlog - BBSHIFT;
@@ -2719,7 +2743,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* sb_versionnum, finobt and rmapbt flags must be set before we use
* libxfs_prealloc_blocks().
*/
- sb_set_features(&mp->m_sb, &sb_feat, sectorsize, lsectorsize, dsunit);
+ sb_set_features(&mp->m_sb, &sb_feat, sectorsize,
+ lsectorsize, params.dsunit);
if (loginternal) {
@@ -2734,7 +2759,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
/* revalidate the log size is valid if we changed it */
validate_log_size(logblocks, params.blocklog, min_logblocks);
}
- if (logblocks > agsize - libxfs_prealloc_blocks(mp)) {
+ if (logblocks > params.agsize - libxfs_prealloc_blocks(mp)) {
fprintf(stderr,
_("internal log size %lld too large, must fit in allocation group\n"),
(long long)logblocks);
@@ -2742,14 +2767,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
}
if (laflag) {
- if (logagno >= agcount) {
+ if (logagno >= params.agcount) {
fprintf(stderr,
_("log ag number %d too large, must be less than %lld\n"),
- logagno, (long long)agcount);
+ logagno, (long long)params.agcount);
usage();
}
} else
- logagno = (xfs_agnumber_t)(agcount / 2);
+ logagno = (xfs_agnumber_t)(params.agcount / 2);
logstart = XFS_AGB_TO_FSB(mp, logagno, libxfs_prealloc_blocks(mp));
/*
@@ -2757,11 +2782,13 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
*/
if (lsunit) {
logstart = fixup_internal_log_stripe(mp,
- lsflag, logstart, agsize, lsunit,
+ lsflag, logstart, params.agsize, lsunit,
&logblocks, params.blocklog, &lalign);
- } else if (dsunit) {
+ } else if (params.dsunit) {
logstart = fixup_internal_log_stripe(mp,
- lsflag, logstart, agsize, dsunit,
+ lsflag, logstart,
+ params.agsize,
+ params.dsunit,
&logblocks, params.blocklog, &lalign);
}
} else {
@@ -2783,13 +2810,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
"log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
" =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
"realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
- dfile, isize, (long long)agcount, (long long)agsize,
+ dfile, isize, (long long)params.agcount,
+ (long long)params.agsize,
"", sectorsize, sb_feat.attr_version,
!sb_feat.projid16bit,
"", sb_feat.crcs_enabled, sb_feat.finobt, sb_feat.spinodes,
sb_feat.rmapbt, sb_feat.reflink,
"", blocksize, (long long)dblocks, imaxpct,
- "", dsunit, dswidth,
+ "", params.dsunit, params.dswidth,
sb_feat.dir_version, dirblocksize, sb_feat.nci,
sb_feat.dirftype,
logfile, 1 << params.blocklog, (long long)logblocks,
@@ -2814,13 +2842,13 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_logstart = logstart;
sbp->sb_rootino = sbp->sb_rbmino = sbp->sb_rsumino = NULLFSINO;
sbp->sb_rextsize = rtextblocks;
- sbp->sb_agcount = (xfs_agnumber_t)agcount;
+ sbp->sb_agcount = (xfs_agnumber_t)params.agcount;
sbp->sb_rbmblocks = nbmblocks;
sbp->sb_logblocks = (xfs_extlen_t)logblocks;
sbp->sb_sectsize = (__uint16_t)sectorsize;
sbp->sb_inodesize = (__uint16_t)isize;
sbp->sb_inopblock = (__uint16_t)(blocksize / isize);
- sbp->sb_sectlog = (__uint8_t)sectorlog;
+ sbp->sb_sectlog = (__uint8_t)params.sectorlog;
sbp->sb_inodelog = (__uint8_t)inodelog;
sbp->sb_inopblog = (__uint8_t)(params.blocklog - inodelog);
sbp->sb_rextslog =
@@ -2830,13 +2858,13 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_imax_pct = imaxpct;
sbp->sb_icount = 0;
sbp->sb_ifree = 0;
- sbp->sb_fdblocks = dblocks - agcount * libxfs_prealloc_blocks(mp) -
+ sbp->sb_fdblocks = dblocks - params.agcount * libxfs_prealloc_blocks(mp) -
(loginternal ? logblocks : 0);
sbp->sb_frextents = 0; /* will do a free later */
sbp->sb_uquotino = sbp->sb_gquotino = sbp->sb_pquotino = 0;
sbp->sb_qflags = 0;
- sbp->sb_unit = dsunit;
- sbp->sb_width = dswidth;
+ sbp->sb_unit = params.dsunit;
+ sbp->sb_width = params.dswidth;
sbp->sb_dirblklog = dirblocklog - params.blocklog;
if (sb_feat.log_version == 2) { /* This is stored in bytes */
lsunit = (lsunit == 0) ? 1 : XFS_FSB_TO_B(mp, lsunit);
@@ -2859,10 +2887,11 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_logsectsize = 0;
}
- sb_set_features(&mp->m_sb, &sb_feat, sectorsize, lsectorsize, dsunit);
+ sb_set_features(&mp->m_sb, &sb_feat, sectorsize,
+ lsectorsize, params.dsunit);
if (force_overwrite)
- zero_old_xfs_structures(&xi, sbp);
+ zero_old_xfs_structures(¶ms.xi, sbp);
/*
* Zero out the beginning of the device, to obliterate any old
@@ -2870,7 +2899,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* swap (somewhere around the page size), jfs (32k),
* ext[2,3] and reiserfs (64k) - and hopefully all else.
*/
- libxfs_buftarg_init(mp, xi.ddev, xi.logdev, xi.rtdev);
+ libxfs_buftarg_init(mp, params.xi.ddev, params.xi.logdev,
+ params.xi.rtdev);
buf = libxfs_getbuf(mp->m_ddev_targp, 0, BTOBB(WHACK_SIZE));
memset(XFS_BUF_PTR(buf), 0, WHACK_SIZE);
libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
@@ -2889,8 +2919,9 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* if needed so that the reads for the end of the device in the mount
* code will succeed.
*/
- if (xi.disfile && xi.dsize * xi.dbsize < dblocks * blocksize) {
- if (ftruncate(xi.dfd, dblocks * blocksize) < 0) {
+ if (params.xi.disfile &&
+ params.xi.dsize * params.xi.dbsize < dblocks * blocksize) {
+ if (ftruncate(params.xi.dfd, dblocks * blocksize) < 0) {
fprintf(stderr,
_("%s: Growing the data section failed\n"),
progname);
@@ -2903,9 +2934,9 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* old MD RAID (or other) metadata at the end of the device.
* (MD sb is ~64k from the end, take out a wider swath to be sure)
*/
- if (!xi.disfile) {
+ if (!params.xi.disfile) {
buf = libxfs_getbuf(mp->m_ddev_targp,
- (xi.dsize - BTOBB(WHACK_SIZE)),
+ (params.xi.dsize - BTOBB(WHACK_SIZE)),
BTOBB(WHACK_SIZE));
memset(XFS_BUF_PTR(buf), 0, WHACK_SIZE);
libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
@@ -2920,7 +2951,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
(xfs_extlen_t)XFS_FSB_TO_BB(mp, logblocks),
&sbp->sb_uuid, sb_feat.log_version, lsunit, XLOG_FMT, XLOG_INIT_CYCLE, false);
- mp = libxfs_mount(mp, sbp, xi.ddev, xi.logdev, xi.rtdev, 0);
+ mp = libxfs_mount(mp, sbp, params.xi.ddev, params.xi.logdev,
+ params.xi.rtdev, 0);
if (mp == NULL) {
fprintf(stderr, _("%s: filesystem failed to initialize\n"),
progname);
@@ -2932,7 +2964,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* These initialisations should be pulled into libxfs to keep the
* kernel/userspace header initialisation code the same.
*/
- for (agno = 0; agno < agcount; agno++) {
+ for (agno = 0; agno < params.agcount; agno++) {
struct xfs_agfl *agfl;
int bucket;
struct xfs_perag *pag = libxfs_perag_get(mp, agno);
@@ -2957,12 +2989,13 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
buf->b_ops = &xfs_agf_buf_ops;
agf = XFS_BUF_TO_AGF(buf);
memset(agf, 0, sectorsize);
- if (agno == agcount - 1)
- agsize = dblocks - (xfs_rfsblock_t)(agno * agsize);
+ if (agno == params.agcount - 1)
+ params.agsize = dblocks -
+ (xfs_rfsblock_t)(agno * params.agsize);
agf->agf_magicnum = cpu_to_be32(XFS_AGF_MAGIC);
agf->agf_versionnum = cpu_to_be32(XFS_AGF_VERSION);
agf->agf_seqno = cpu_to_be32(agno);
- agf->agf_length = cpu_to_be32(agsize);
+ agf->agf_length = cpu_to_be32(params.agsize);
agf->agf_roots[XFS_BTNUM_BNOi] = cpu_to_be32(XFS_BNO_BLOCK(mp));
agf->agf_roots[XFS_BTNUM_CNTi] = cpu_to_be32(XFS_CNT_BLOCK(mp));
agf->agf_levels[XFS_BTNUM_BNOi] = cpu_to_be32(1);
@@ -2984,7 +3017,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
agf->agf_flfirst = 0;
agf->agf_fllast = cpu_to_be32(XFS_AGFL_SIZE(mp) - 1);
agf->agf_flcount = 0;
- nbmblocks = (xfs_extlen_t)(agsize - libxfs_prealloc_blocks(mp));
+ nbmblocks = (xfs_extlen_t)(params.agsize -
+ libxfs_prealloc_blocks(mp));
agf->agf_freeblks = cpu_to_be32(nbmblocks);
agf->agf_longest = cpu_to_be32(nbmblocks);
if (xfs_sb_version_hascrc(&mp->m_sb))
@@ -2992,7 +3026,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
if (loginternal && agno == logagno) {
be32_add_cpu(&agf->agf_freeblks, -logblocks);
- agf->agf_longest = cpu_to_be32(agsize -
+ agf->agf_longest = cpu_to_be32(params.agsize -
XFS_FSB_TO_AGBNO(mp, logstart) - logblocks);
}
if (libxfs_alloc_min_freelist(mp, pag) > worst_freelist)
@@ -3031,7 +3065,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
agi->agi_magicnum = cpu_to_be32(XFS_AGI_MAGIC);
agi->agi_versionnum = cpu_to_be32(XFS_AGI_VERSION);
agi->agi_seqno = cpu_to_be32(agno);
- agi->agi_length = cpu_to_be32((xfs_agblock_t)agsize);
+ agi->agi_length = cpu_to_be32((xfs_agblock_t)params.agsize);
agi->agi_count = 0;
agi->agi_root = cpu_to_be32(XFS_IBT_BLOCK(mp));
agi->agi_level = cpu_to_be32(1);
@@ -3096,7 +3130,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* so, reset the record count to 0 to avoid exposure of an invalid
* record start block.
*/
- arec->ar_blockcount = cpu_to_be32(agsize -
+ arec->ar_blockcount = cpu_to_be32(params.agsize -
be32_to_cpu(arec->ar_startblock));
if (!arec->ar_blockcount)
block->bb_numrecs = 0;
@@ -3141,7 +3175,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* so, reset the record count to 0 to avoid exposure of an invalid
* record start block.
*/
- arec->ar_blockcount = cpu_to_be32(agsize -
+ arec->ar_blockcount = cpu_to_be32(params.agsize -
be32_to_cpu(arec->ar_startblock));
if (!arec->ar_blockcount)
block->bb_numrecs = 0;
@@ -3304,7 +3338,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
/*
* BNO, CNT free block list
*/
- for (agno = 0; agno < agcount; agno++) {
+ for (agno = 0; agno < params.agcount; agno++) {
xfs_alloc_arg_t args;
xfs_trans_t *tp;
struct xfs_trans_res tres = {0};
@@ -3328,7 +3362,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
/*
* Allocate the root inode and anything else in the proto file.
*/
- parse_proto(mp, &fsx, &protostring);
+ parse_proto(mp, ¶ms.fsx, &protostring);
/*
* Protect ourselves against possible stupidity
@@ -3385,11 +3419,11 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
libxfs_umount(mp);
- if (xi.rtdev)
- libxfs_device_close(xi.rtdev);
- if (xi.logdev && xi.logdev != xi.ddev)
- libxfs_device_close(xi.logdev);
- libxfs_device_close(xi.ddev);
+ if (params.xi.rtdev)
+ libxfs_device_close(params.xi.rtdev);
+ if (params.xi.logdev && params.xi.logdev != params.xi.ddev)
+ libxfs_device_close(params.xi.logdev);
+ libxfs_device_close(params.xi.ddev);
return 0;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 3/9] mkfs.xfs: move iopts to to struct mkfs_xfs_opts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 1/9] mkfs.xfs: add helper to parse command line options Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 2/9] mkfs.xfs: move dopts to struct mkfs_xfs_opts Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 4/9] mkfs.xfs: move lopts " Luis R. Rodriguez
` (8 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
This moves all main() iopts to struct mkfs_xfs_opts in
order to help with clutter and later enable re-parsing
options for other purposes.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 342 ++++++++++++++++++++++++++++----------------------------
1 file changed, 173 insertions(+), 169 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 868cab2164d6..4575d7c84f0a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -708,6 +708,23 @@ struct opt_params mopts = {
},
};
+struct sb_feat_args {
+ int log_version;
+ int attr_version;
+ int dir_version;
+ int spinodes;
+ int finobt;
+ bool inode_align;
+ bool nci;
+ bool lazy_sb_counters;
+ bool projid16bit;
+ bool crcs_enabled;
+ bool dirftype;
+ bool parent_pointers;
+ bool rmapbt;
+ bool reflink;
+};
+
struct mkfs_xfs_opts {
int blocklog;
int blflag;
@@ -728,6 +745,16 @@ struct mkfs_xfs_opts {
int nodsflag;
int sectorlog;
int slflag;
+
+ struct sb_feat_args sb_feat;
+ int inodelog;
+ int isize;
+ int ilflag;
+ int imaxpct;
+ int imflag;
+ int ipflag;
+ int isflag;
+ int inopblock;
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -1171,23 +1198,6 @@ discard_blocks(dev_t dev, __uint64_t nsectors)
platform_discard_blocks(fd, 0, nsectors << 9);
}
-struct sb_feat_args {
- int log_version;
- int attr_version;
- int dir_version;
- int spinodes;
- int finobt;
- bool inode_align;
- bool nci;
- bool lazy_sb_counters;
- bool projid16bit;
- bool crcs_enabled;
- bool dirftype;
- bool parent_pointers;
- bool rmapbt;
- bool reflink;
-};
-
static void
sb_set_features(
struct xfs_sb *sbp,
@@ -1518,11 +1528,74 @@ parse_subopts(
}
}
break;
+ case 'i':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)iopts.subopts, &value)) {
+ case I_ALIGN:
+ params->sb_feat.inode_align =
+ getnum(value, &iopts, I_ALIGN);
+ break;
+ case I_LOG:
+ params->inodelog = getnum(value, &iopts, I_LOG);
+ params->isize = 1 << params->inodelog;
+ params->ilflag = 1;
+ break;
+ case I_MAXPCT:
+ params->imaxpct = getnum(value, &iopts,
+ I_MAXPCT);
+ params->imflag = 1;
+ break;
+ case I_PERBLOCK:
+ params->inopblock = getnum(value, &iopts,
+ I_PERBLOCK);
+ params->ipflag = 1;
+ break;
+ case I_SIZE:
+ params->isize = getnum(value, &iopts, I_SIZE);
+ params->inodelog =
+ libxfs_highbit32(params->isize);
+ params->isflag = 1;
+ break;
+ case I_ATTR:
+ params->sb_feat.attr_version =
+ getnum(value, &iopts, I_ATTR);
+ break;
+ case I_PROJID32BIT:
+ params->sb_feat.projid16bit =
+ !getnum(value, &iopts, I_PROJID32BIT);
+ break;
+ case I_SPINODES:
+ params->sb_feat.spinodes =
+ getnum(value, &iopts, I_SPINODES);
+ break;
+ default:
+ unknown('i', value);
+ }
+ }
+ break;
default:
usage();
}
}
+static void init_sb_feat_args_default(struct sb_feat_args *sb_feat)
+{
+ sb_feat->finobt = 1;
+ sb_feat->spinodes = 0;
+ sb_feat->log_version = 2;
+ sb_feat->attr_version = 2;
+ sb_feat->dir_version = XFS_DFL_DIR_VERSION;
+ sb_feat->inode_align = XFS_IFLAG_ALIGN;
+ sb_feat->nci = false;
+ sb_feat->lazy_sb_counters = true;
+ sb_feat->projid16bit = false;
+ sb_feat->crcs_enabled = true;
+ sb_feat->dirftype = true;
+ sb_feat->parent_pointers = false;
+ sb_feat->rmapbt = false;
+ sb_feat->reflink = false;
+}
+
int
main(
int argc,
@@ -1541,14 +1614,6 @@ main(
int dirblocklog;
int dirblocksize;
int force_overwrite;
- int ilflag;
- int imaxpct;
- int imflag;
- int inodelog;
- int inopblock;
- int ipflag;
- int isflag;
- int isize;
char *label = NULL;
int laflag;
int lalign;
@@ -1597,25 +1662,11 @@ main(
uuid_t uuid;
int worst_freelist;
struct fs_topology ft;
- struct sb_feat_args sb_feat = {
- .finobt = 1,
- .spinodes = 0,
- .log_version = 2,
- .attr_version = 2,
- .dir_version = XFS_DFL_DIR_VERSION,
- .inode_align = XFS_IFLAG_ALIGN,
- .nci = false,
- .lazy_sb_counters = true,
- .projid16bit = false,
- .crcs_enabled = true,
- .dirftype = true,
- .parent_pointers = false,
- .rmapbt = false,
- .reflink = false,
- };
struct mkfs_xfs_opts params;
+ struct sb_feat_args *sb_feat = ¶ms.sb_feat;
memset(¶ms, 0, sizeof(params));
+ init_sb_feat_args_default(sb_feat);
platform_uuid_generate(&uuid);
progname = basename(argv[0]);
@@ -1628,14 +1679,12 @@ main(
lsectorlog = 0;
sectorsize = lsectorsize = 0;
dblocks = 0;
- ilflag = imflag = ipflag = isflag = 0;
liflag = laflag = lsflag = lsuflag = lsunitflag = ldflag = lvflag = 0;
loginternal = 1;
logagno = logblocks = rtblocks = rtextblocks = 0;
Nflag = nlflag = nsflag = nvflag = 0;
dirblocklog = dirblocksize = 0;
qflag = 0;
- imaxpct = inodelog = inopblock = isize = 0;
dfile = logfile = rtfile = NULL;
logsize = rtsize = rtextsize = protofile = NULL;
lalign = lsu = lsunit = 0;
@@ -1654,57 +1703,9 @@ main(
break;
case 'b':
case 'd':
- p = optarg;
- parse_subopts(c, p, ¶ms);
- break;
case 'i':
p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)iopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case I_ALIGN:
- sb_feat.inode_align = getnum(value,
- &iopts, I_ALIGN);
- break;
- case I_LOG:
- inodelog = getnum(value, &iopts, I_LOG);
- isize = 1 << inodelog;
- ilflag = 1;
- break;
- case I_MAXPCT:
- imaxpct = getnum(value, &iopts,
- I_MAXPCT);
- imflag = 1;
- break;
- case I_PERBLOCK:
- inopblock = getnum(value, &iopts,
- I_PERBLOCK);
- ipflag = 1;
- break;
- case I_SIZE:
- isize = getnum(value, &iopts, I_SIZE);
- inodelog = libxfs_highbit32(isize);
- isflag = 1;
- break;
- case I_ATTR:
- sb_feat.attr_version =
- getnum(value, &iopts, I_ATTR);
- break;
- case I_PROJID32BIT:
- sb_feat.projid16bit =
- !getnum(value, &iopts,
- I_PROJID32BIT);
- break;
- case I_SPINODES:
- sb_feat.spinodes = getnum(value,
- &iopts, I_SPINODES);
- break;
- default:
- unknown('i', value);
- }
- }
+ parse_subopts(c, p, ¶ms);
break;
case 'l':
p = optarg;
@@ -1742,7 +1743,7 @@ main(
loginternal = 0;
break;
case L_VERSION:
- sb_feat.log_version =
+ sb_feat->log_version =
getnum(value, &lopts, L_VERSION);
lvflag = 1;
break;
@@ -1763,7 +1764,7 @@ main(
lssflag = 1;
break;
case L_LAZYSBCNTR:
- sb_feat.lazy_sb_counters =
+ sb_feat->lazy_sb_counters =
getnum(value, &lopts,
L_LAZYSBCNTR);
break;
@@ -1785,13 +1786,13 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case M_CRC:
- sb_feat.crcs_enabled =
+ sb_feat->crcs_enabled =
getnum(value, &mopts, M_CRC);
- if (sb_feat.crcs_enabled)
- sb_feat.dirftype = true;
+ if (sb_feat->crcs_enabled)
+ sb_feat->dirftype = true;
break;
case M_FINOBT:
- sb_feat.finobt = getnum(
+ sb_feat->finobt = getnum(
value, &mopts, M_FINOBT);
break;
case M_UUID:
@@ -1801,11 +1802,11 @@ main(
illegal(optarg, "m uuid");
break;
case M_RMAPBT:
- sb_feat.rmapbt = getnum(
+ sb_feat->rmapbt = getnum(
value, &mopts, M_RMAPBT);
break;
case M_REFLINK:
- sb_feat.reflink = getnum(
+ sb_feat->reflink = getnum(
value, &mopts, M_REFLINK);
break;
default:
@@ -1837,16 +1838,16 @@ main(
value = getstr(value, &nopts, N_VERSION);
if (!strcasecmp(value, "ci")) {
/* ASCII CI mode */
- sb_feat.nci = true;
+ sb_feat->nci = true;
} else {
- sb_feat.dir_version =
+ sb_feat->dir_version =
getnum(value, &nopts,
N_VERSION);
}
nvflag = 1;
break;
case N_FTYPE:
- sb_feat.dirftype = getnum(value, &nopts,
+ sb_feat->dirftype = getnum(value, &nopts,
N_FTYPE);
break;
default:
@@ -1966,13 +1967,13 @@ main(
fprintf(stderr, _("illegal block size %d\n"), blocksize);
usage();
}
- if (sb_feat.crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
+ if (sb_feat->crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
fprintf(stderr,
_("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
XFS_MIN_CRC_BLOCKSIZE);
usage();
}
- if (sb_feat.crcs_enabled && !sb_feat.dirftype) {
+ if (sb_feat->crcs_enabled && !sb_feat->dirftype) {
fprintf(stderr, _("cannot disable ftype with crcs enabled\n"));
usage();
}
@@ -2073,7 +2074,7 @@ _("block size %d cannot be smaller than logical sector size %d\n"),
usage();
} else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
lsu = blocksize;
- sb_feat.log_version = 2;
+ sb_feat->log_version = 2;
}
/*
@@ -2081,9 +2082,10 @@ _("block size %d cannot be smaller than logical sector size %d\n"),
* no longer optional for CRC enabled filesystems. Catch them up front
* here before doing anything else.
*/
- if (sb_feat.crcs_enabled) {
+ if (sb_feat->crcs_enabled) {
/* minimum inode size is 512 bytes, ipflag checked later */
- if ((isflag || ilflag) && inodelog < XFS_DINODE_DFL_CRC_LOG) {
+ if ((params.isflag || params.ilflag) &&
+ params.inodelog < XFS_DINODE_DFL_CRC_LOG) {
fprintf(stderr,
_("Minimum inode size for CRCs is %d bytes\n"),
1 << XFS_DINODE_DFL_CRC_LOG);
@@ -2091,28 +2093,28 @@ _("Minimum inode size for CRCs is %d bytes\n"),
}
/* inodes always aligned */
- if (!sb_feat.inode_align) {
+ if (!sb_feat->inode_align) {
fprintf(stderr,
_("Inodes always aligned for CRC enabled filesytems\n"));
usage();
}
/* lazy sb counters always on */
- if (!sb_feat.lazy_sb_counters) {
+ if (!sb_feat->lazy_sb_counters) {
fprintf(stderr,
_("Lazy superblock counted always enabled for CRC enabled filesytems\n"));
usage();
}
/* version 2 logs always on */
- if (sb_feat.log_version != 2) {
+ if (sb_feat->log_version != 2) {
fprintf(stderr,
_("V2 logs always enabled for CRC enabled filesytems\n"));
usage();
}
/* attr2 always on */
- if (sb_feat.attr_version != 2) {
+ if (sb_feat->attr_version != 2) {
fprintf(stderr,
_("V2 attribute format always enabled on CRC enabled filesytems\n"));
usage();
@@ -2120,7 +2122,7 @@ _("V2 attribute format always enabled on CRC enabled filesytems\n"));
/* 32 bit project quota always on */
/* attr2 always on */
- if (sb_feat.projid16bit) {
+ if (sb_feat->projid16bit) {
fprintf(stderr,
_("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
usage();
@@ -2135,41 +2137,41 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
* then issue an error.
* The same is also for sparse inodes.
*/
- if (sb_feat.finobt && mopts.subopt_params[M_FINOBT].seen) {
+ if (sb_feat->finobt && mopts.subopt_params[M_FINOBT].seen) {
fprintf(stderr,
_("finobt not supported without CRC support\n"));
usage();
}
- sb_feat.finobt = 0;
+ sb_feat->finobt = 0;
- if (sb_feat.spinodes) {
+ if (sb_feat->spinodes) {
fprintf(stderr,
_("sparse inodes not supported without CRC support\n"));
usage();
}
- sb_feat.spinodes = 0;
+ sb_feat->spinodes = 0;
- if (sb_feat.rmapbt) {
+ if (sb_feat->rmapbt) {
fprintf(stderr,
_("rmapbt not supported without CRC support\n"));
usage();
}
- sb_feat.rmapbt = false;
+ sb_feat->rmapbt = false;
- if (sb_feat.reflink) {
+ if (sb_feat->reflink) {
fprintf(stderr,
_("reflink not supported without CRC support\n"));
usage();
}
- sb_feat.reflink = false;
+ sb_feat->reflink = false;
}
- if (sb_feat.rmapbt && params.xi.rtname) {
+ if (sb_feat->rmapbt && params.xi.rtname) {
fprintf(stderr,
_("rmapbt not supported with realtime devices\n"));
usage();
- sb_feat.rmapbt = false;
+ sb_feat->rmapbt = false;
}
if (nsflag || nlflag) {
@@ -2205,15 +2207,15 @@ _("rmapbt not supported with realtime devices\n"));
(long long)dbytes, blocksize,
(long long)(dblocks << params.blocklog));
}
- if (ipflag) {
- inodelog = params.blocklog - libxfs_highbit32(inopblock);
- isize = 1 << inodelog;
- } else if (!ilflag && !isflag) {
- inodelog = sb_feat.crcs_enabled ? XFS_DINODE_DFL_CRC_LOG
+ if (params.ipflag) {
+ params.inodelog = params.blocklog - libxfs_highbit32(params.inopblock);
+ params.isize = 1 << params.inodelog;
+ } else if (!params.ilflag && !params.isflag) {
+ params.inodelog = sb_feat->crcs_enabled ? XFS_DINODE_DFL_CRC_LOG
: XFS_DINODE_DFL_LOG;
- isize = 1 << inodelog;
+ params.isize = 1 << params.inodelog;
}
- if (sb_feat.crcs_enabled && inodelog < XFS_DINODE_DFL_CRC_LOG) {
+ if (sb_feat->crcs_enabled && params.inodelog < XFS_DINODE_DFL_CRC_LOG) {
fprintf(stderr,
_("Minimum inode size for CRCs is %d bytes\n"),
1 << XFS_DINODE_DFL_CRC_LOG);
@@ -2302,12 +2304,12 @@ _("rmapbt not supported with realtime devices\n"));
/*
* Check some argument sizes against mins, maxes.
*/
- if (isize > blocksize / XFS_MIN_INODE_PERBLOCK ||
- isize < XFS_DINODE_MIN_SIZE ||
- isize > XFS_DINODE_MAX_SIZE) {
+ if (params.isize > blocksize / XFS_MIN_INODE_PERBLOCK ||
+ params.isize < XFS_DINODE_MIN_SIZE ||
+ params.isize > XFS_DINODE_MAX_SIZE) {
int maxsz;
- fprintf(stderr, _("illegal inode size %d\n"), isize);
+ fprintf(stderr, _("illegal inode size %d\n"), params.isize);
maxsz = MIN(blocksize / XFS_MIN_INODE_PERBLOCK,
XFS_DINODE_MAX_SIZE);
if (XFS_DINODE_MIN_SIZE == maxsz)
@@ -2322,10 +2324,10 @@ _("rmapbt not supported with realtime devices\n"));
}
/* if lsu or lsunit was specified, automatically use v2 logs */
- if ((lsu || lsunit) && sb_feat.log_version == 1) {
+ if ((lsu || lsunit) && sb_feat->log_version == 1) {
fprintf(stderr,
_("log stripe unit specified, using v2 logs\n"));
- sb_feat.log_version = 2;
+ sb_feat->log_version = 2;
}
calc_stripe_factors(params.dsu, params.dsw, sectorsize, lsu, lsectorsize,
@@ -2631,8 +2633,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
validate_ag_geometry(params.blocklog, dblocks, params.agsize, params.agcount);
- if (!imflag)
- imaxpct = calc_default_imaxpct(params.blocklog, dblocks);
+ if (!params.imflag)
+ params.imaxpct = calc_default_imaxpct(params.blocklog, dblocks);
/*
* check that log sunit is modulo fsblksize or default it to dsunit.
@@ -2641,12 +2643,12 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
if (lsunit) {
/* convert from 512 byte blocks to fs blocks */
lsunit = DTOBT(lsunit);
- } else if (sb_feat.log_version == 2 && loginternal && params.dsunit) {
+ } else if (sb_feat->log_version == 2 && loginternal && params.dsunit) {
/* lsunit and dsunit now in fs blocks */
lsunit = params.dsunit;
}
- if (sb_feat.log_version == 2 && (lsunit * blocksize) > 256 * 1024) {
+ if (sb_feat->log_version == 2 && (lsunit * blocksize) > 256 * 1024) {
/* Warn only if specified on commandline */
if (lsuflag || lsunitflag) {
fprintf(stderr,
@@ -2659,11 +2661,12 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
}
min_logblocks = max_trans_res(params.agsize,
- sb_feat.crcs_enabled, sb_feat.dir_version,
+ sb_feat->crcs_enabled, sb_feat->dir_version,
params.sectorlog, params.blocklog,
- inodelog, dirblocklog,
- sb_feat.log_version, lsunit, sb_feat.finobt,
- sb_feat.rmapbt, sb_feat.reflink);
+ params.inodelog,
+ dirblocklog,
+ sb_feat->log_version, lsunit, sb_feat->finobt,
+ sb_feat->rmapbt, sb_feat->reflink);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
if (!logsize && dblocks >= (1024*1024*1024) >> params.blocklog)
@@ -2743,7 +2746,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* sb_versionnum, finobt and rmapbt flags must be set before we use
* libxfs_prealloc_blocks().
*/
- sb_set_features(&mp->m_sb, &sb_feat, sectorsize,
+ sb_set_features(&mp->m_sb, sb_feat, sectorsize,
lsectorsize, params.dsunit);
@@ -2810,19 +2813,19 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
"log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
" =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
"realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
- dfile, isize, (long long)params.agcount,
+ dfile, params.isize, (long long)params.agcount,
(long long)params.agsize,
- "", sectorsize, sb_feat.attr_version,
- !sb_feat.projid16bit,
- "", sb_feat.crcs_enabled, sb_feat.finobt, sb_feat.spinodes,
- sb_feat.rmapbt, sb_feat.reflink,
- "", blocksize, (long long)dblocks, imaxpct,
+ "", sectorsize, sb_feat->attr_version,
+ !sb_feat->projid16bit,
+ "", sb_feat->crcs_enabled, sb_feat->finobt, sb_feat->spinodes,
+ sb_feat->rmapbt, sb_feat->reflink,
+ "", blocksize, (long long)dblocks, params.imaxpct,
"", params.dsunit, params.dswidth,
- sb_feat.dir_version, dirblocksize, sb_feat.nci,
- sb_feat.dirftype,
+ sb_feat->dir_version, dirblocksize, sb_feat->nci,
+ sb_feat->dirftype,
logfile, 1 << params.blocklog, (long long)logblocks,
- sb_feat.log_version, "", lsectorsize, lsunit,
- sb_feat.lazy_sb_counters,
+ sb_feat->log_version, "", lsectorsize, lsunit,
+ sb_feat->lazy_sb_counters,
rtfile, rtextblocks << params.blocklog,
(long long)rtblocks, (long long)rtextents);
if (Nflag)
@@ -2846,16 +2849,16 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_rbmblocks = nbmblocks;
sbp->sb_logblocks = (xfs_extlen_t)logblocks;
sbp->sb_sectsize = (__uint16_t)sectorsize;
- sbp->sb_inodesize = (__uint16_t)isize;
- sbp->sb_inopblock = (__uint16_t)(blocksize / isize);
+ sbp->sb_inodesize = (__uint16_t)params.isize;
+ sbp->sb_inopblock = (__uint16_t)(blocksize / params.isize);
sbp->sb_sectlog = (__uint8_t)params.sectorlog;
- sbp->sb_inodelog = (__uint8_t)inodelog;
- sbp->sb_inopblog = (__uint8_t)(params.blocklog - inodelog);
+ sbp->sb_inodelog = (__uint8_t)params.inodelog;
+ sbp->sb_inopblog = (__uint8_t)(params.blocklog - params.inodelog);
sbp->sb_rextslog =
(__uint8_t)(rtextents ?
libxfs_highbit32((unsigned int)rtextents) : 0);
sbp->sb_inprogress = 1; /* mkfs is in progress */
- sbp->sb_imax_pct = imaxpct;
+ sbp->sb_imax_pct = params.imaxpct;
sbp->sb_icount = 0;
sbp->sb_ifree = 0;
sbp->sb_fdblocks = dblocks - params.agcount * libxfs_prealloc_blocks(mp) -
@@ -2866,17 +2869,17 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_unit = params.dsunit;
sbp->sb_width = params.dswidth;
sbp->sb_dirblklog = dirblocklog - params.blocklog;
- if (sb_feat.log_version == 2) { /* This is stored in bytes */
+ if (sb_feat->log_version == 2) { /* This is stored in bytes */
lsunit = (lsunit == 0) ? 1 : XFS_FSB_TO_B(mp, lsunit);
sbp->sb_logsunit = lsunit;
} else
sbp->sb_logsunit = 0;
- if (sb_feat.inode_align) {
+ if (sb_feat->inode_align) {
int cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
- if (sb_feat.crcs_enabled)
- cluster_size *= isize / XFS_DINODE_MIN_SIZE;
+ if (sb_feat->crcs_enabled)
+ cluster_size *= params.isize / XFS_DINODE_MIN_SIZE;
sbp->sb_inoalignmt = cluster_size >> params.blocklog;
- sb_feat.inode_align = sbp->sb_inoalignmt != 0;
+ sb_feat->inode_align = sbp->sb_inoalignmt != 0;
} else
sbp->sb_inoalignmt = 0;
if (lsectorsize != BBSIZE || sectorsize != BBSIZE) {
@@ -2887,7 +2890,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_logsectsize = 0;
}
- sb_set_features(&mp->m_sb, &sb_feat, sectorsize,
+ sb_set_features(&mp->m_sb, sb_feat, sectorsize,
lsectorsize, params.dsunit);
if (force_overwrite)
@@ -2949,7 +2952,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
libxfs_log_clear(mp->m_logdev_targp, NULL,
XFS_FSB_TO_DADDR(mp, logstart),
(xfs_extlen_t)XFS_FSB_TO_BB(mp, logblocks),
- &sbp->sb_uuid, sb_feat.log_version, lsunit, XLOG_FMT, XLOG_INIT_CYCLE, false);
+ &sbp->sb_uuid, sb_feat->log_version, lsunit,
+ XLOG_FMT, XLOG_INIT_CYCLE, false);
mp = libxfs_mount(mp, sbp, params.xi.ddev, params.xi.logdev,
params.xi.rtdev, 0);
@@ -3069,7 +3073,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
agi->agi_count = 0;
agi->agi_root = cpu_to_be32(XFS_IBT_BLOCK(mp));
agi->agi_level = cpu_to_be32(1);
- if (sb_feat.finobt) {
+ if (sb_feat->finobt) {
agi->agi_free_root = cpu_to_be32(XFS_FIBT_BLOCK(mp));
agi->agi_free_level = cpu_to_be32(1);
}
@@ -3220,7 +3224,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
/*
* Free INO btree root block
*/
- if (sb_feat.finobt) {
+ if (sb_feat->finobt) {
buf = libxfs_getbuf(mp->m_ddev_targp,
XFS_AGB_TO_DADDR(mp, agno, XFS_FIBT_BLOCK(mp)),
bsize);
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 4/9] mkfs.xfs: move lopts to struct mkfs_xfs_opts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (2 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 3/9] mkfs.xfs: move iopts to " Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 5/9] mkfs.xfs: move mopts " Luis R. Rodriguez
` (7 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
This moves all main() lopts to struct mkfs_xfs_opts in
order to help with clutter and later enable re-parsing
options for other purposes.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 355 ++++++++++++++++++++++++++++----------------------------
1 file changed, 180 insertions(+), 175 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4575d7c84f0a..cd52b525f824 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -755,6 +755,23 @@ struct mkfs_xfs_opts {
int ipflag;
int isflag;
int inopblock;
+
+ xfs_agnumber_t logagno;
+ int laflag;
+ int liflag;
+ int lsuflag;
+ int lsu;
+ int loginternal;
+ char *logfile;
+ char *logsize;
+ int lsectorlog;
+ int lsectorsize;
+ int lsunit;
+ int lsunitflag;
+ int ldflag;
+ int lvflag;
+ int lslflag;
+ int lssflag;
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -1573,6 +1590,67 @@ parse_subopts(
}
}
break;
+ case 'l':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)lopts.subopts, &value)) {
+ case L_AGNUM:
+ params->logagno = getnum(value, &lopts, L_AGNUM);
+ params->laflag = 1;
+ break;
+ case L_FILE:
+ params->xi.lisfile = getnum(value, &lopts,
+ L_FILE);
+ break;
+ case L_INTERNAL:
+ params->loginternal = getnum(value, &lopts,
+ L_INTERNAL);
+ params->liflag = 1;
+ break;
+ case L_SU:
+ params->lsu = getnum(value, &lopts, L_SU);
+ params->lsuflag = 1;
+ break;
+ case L_SUNIT:
+ params->lsunit = getnum(value, &lopts, L_SUNIT);
+ params->lsunitflag = 1;
+ break;
+ case L_NAME:
+ case L_DEV:
+ params->logfile = getstr(value, &lopts, L_NAME);
+ params->xi.logname = params->logfile;
+ params->ldflag = 1;
+ params->loginternal = 0;
+ break;
+ case L_VERSION:
+ params->sb_feat.log_version =
+ getnum(value, &lopts, L_VERSION);
+ params->lvflag = 1;
+ break;
+ case L_SIZE:
+ params->logsize = getstr(value, &lopts, L_SIZE);
+ break;
+ case L_SECTLOG:
+ params->lsectorlog = getnum(value, &lopts,
+ L_SECTLOG);
+ params->lsectorsize = 1 << params->lsectorlog;
+ params->lslflag = 1;
+ break;
+ case L_SECTSIZE:
+ params->lsectorsize = getnum(value, &lopts,
+ L_SECTSIZE);
+ params->lsectorlog =
+ libxfs_highbit32(params->lsectorsize);
+ params->lssflag = 1;
+ break;
+ case L_LAZYSBCNTR:
+ params->sb_feat.lazy_sb_counters =
+ getnum(value, &lopts, L_LAZYSBCNTR);
+ break;
+ default:
+ unknown('l', value);
+ }
+ }
+ break;
default:
usage();
}
@@ -1615,26 +1693,10 @@ main(
int dirblocksize;
int force_overwrite;
char *label = NULL;
- int laflag;
int lalign;
- int ldflag;
- int liflag;
- xfs_agnumber_t logagno;
xfs_rfsblock_t logblocks;
- char *logfile;
- int loginternal;
- char *logsize;
xfs_fsblock_t logstart;
- int lvflag;
int lsflag;
- int lsuflag;
- int lsunitflag;
- int lsectorlog;
- int lsectorsize;
- int lslflag;
- int lssflag;
- int lsu;
- int lsunit;
int min_logblocks;
xfs_mount_t *mp;
xfs_mount_t mbuf;
@@ -1674,20 +1736,18 @@ main(
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
- lslflag = lssflag = 0;
blocksize = 0;
- lsectorlog = 0;
- sectorsize = lsectorsize = 0;
+ sectorsize = 0;
dblocks = 0;
- liflag = laflag = lsflag = lsuflag = lsunitflag = ldflag = lvflag = 0;
- loginternal = 1;
- logagno = logblocks = rtblocks = rtextblocks = 0;
+ params.loginternal = 1;
+ lsflag = 0;
+ logblocks = rtblocks = rtextblocks = 0;
Nflag = nlflag = nsflag = nvflag = 0;
dirblocklog = dirblocksize = 0;
qflag = 0;
- dfile = logfile = rtfile = NULL;
- logsize = rtsize = rtextsize = protofile = NULL;
- lalign = lsu = lsunit = 0;
+ dfile = rtfile = NULL;
+ rtsize = rtextsize = protofile = NULL;
+ lalign = 0;
norsflag = 0;
force_overwrite = 0;
worst_freelist = 0;
@@ -1704,74 +1764,9 @@ main(
case 'b':
case 'd':
case 'i':
- p = optarg;
- parse_subopts(c, p, ¶ms);
- break;
case 'l':
p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)lopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case L_AGNUM:
- logagno = getnum(value, &lopts, L_AGNUM);
- laflag = 1;
- break;
- case L_FILE:
- params.xi.lisfile = getnum(value, &lopts,
- L_FILE);
- break;
- case L_INTERNAL:
- loginternal = getnum(value, &lopts,
- L_INTERNAL);
- liflag = 1;
- break;
- case L_SU:
- lsu = getnum(value, &lopts, L_SU);
- lsuflag = 1;
- break;
- case L_SUNIT:
- lsunit = getnum(value, &lopts, L_SUNIT);
- lsunitflag = 1;
- break;
- case L_NAME:
- case L_DEV:
- logfile = getstr(value, &lopts, L_NAME);
- params.xi.logname = logfile;
- ldflag = 1;
- loginternal = 0;
- break;
- case L_VERSION:
- sb_feat->log_version =
- getnum(value, &lopts, L_VERSION);
- lvflag = 1;
- break;
- case L_SIZE:
- logsize = getstr(value, &lopts, L_SIZE);
- break;
- case L_SECTLOG:
- lsectorlog = getnum(value, &lopts,
- L_SECTLOG);
- lsectorsize = 1 << lsectorlog;
- lslflag = 1;
- break;
- case L_SECTSIZE:
- lsectorsize = getnum(value, &lopts,
- L_SECTSIZE);
- lsectorlog =
- libxfs_highbit32(lsectorsize);
- lssflag = 1;
- break;
- case L_LAZYSBCNTR:
- sb_feat->lazy_sb_counters =
- getnum(value, &lopts,
- L_LAZYSBCNTR);
- break;
- default:
- unknown('l', value);
- }
- }
+ parse_subopts(c, p, ¶ms);
break;
case 'L':
if (strlen(optarg) > sizeof(sbp->sb_fname))
@@ -1910,28 +1905,28 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case S_LOG:
case S_SECTLOG:
- if (lssflag)
+ if (params.lssflag)
conflict('s', subopts,
S_SECTSIZE, S_SECTLOG);
params.sectorlog = getnum(value, &sopts,
S_SECTLOG);
- lsectorlog = params.sectorlog;
+ params.lsectorlog = params.sectorlog;
sectorsize = 1 << params.sectorlog;
- lsectorsize = sectorsize;
- lslflag = params.slflag = 1;
+ params.lsectorsize = sectorsize;
+ params.lslflag = params.slflag = 1;
break;
case S_SIZE:
case S_SECTSIZE:
- if (lslflag)
+ if (params.lslflag)
conflict('s', subopts, S_SECTLOG,
S_SECTSIZE);
sectorsize = getnum(value, &sopts,
S_SECTSIZE);
- lsectorsize = sectorsize;
+ params.lsectorsize = sectorsize;
params.sectorlog =
libxfs_highbit32(sectorsize);
- lsectorlog = params.sectorlog;
- lssflag = params.ssflag = 1;
+ params.lsectorlog = params.sectorlog;
+ params.lssflag = params.ssflag = 1;
break;
default:
unknown('s', value);
@@ -1982,9 +1977,9 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
params.sectorlog = XFS_MIN_SECTORSIZE_LOG;
sectorsize = XFS_MIN_SECTORSIZE;
}
- if (!lslflag && !lssflag) {
- lsectorlog = params.sectorlog;
- lsectorsize = sectorsize;
+ if (!params.lslflag && !params.lssflag) {
+ params.lsectorlog = params.sectorlog;
+ params.lsectorsize = sectorsize;
}
/*
@@ -1997,10 +1992,10 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
check_device_type(dfile, ¶ms.xi.disfile, !params.dsize, !dfile,
Nflag ? NULL : ¶ms.xi.dcreat,
force_overwrite, "d");
- if (!loginternal)
+ if (!params.loginternal)
check_device_type(params.xi.logname,
¶ms.xi.lisfile,
- !logsize,
+ !params.logsize,
!params.xi.logname,
Nflag ? NULL : ¶ms.xi.lcreat,
force_overwrite, "l");
@@ -2047,9 +2042,9 @@ _("switching to logical sector size %d\n"),
if (!params.ssflag) {
params.sectorlog = libxfs_highbit32(sectorsize);
- if (loginternal) {
- lsectorsize = sectorsize;
- lsectorlog = params.sectorlog;
+ if (params.loginternal) {
+ params.lsectorsize = sectorsize;
+ params.lsectorlog = params.sectorlog;
}
}
@@ -2068,12 +2063,13 @@ _("block size %d cannot be smaller than logical sector size %d\n"),
sectorsize, ft.lsectorsize);
usage();
}
- if (lsectorsize < XFS_MIN_SECTORSIZE ||
- lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
- fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
+ if (params.lsectorsize < XFS_MIN_SECTORSIZE ||
+ params.lsectorsize > XFS_MAX_SECTORSIZE || params.lsectorsize > blocksize) {
+ fprintf(stderr, _("illegal log sector size %d\n"),
+ params.lsectorsize);
usage();
- } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
- lsu = blocksize;
+ } else if (params.lsectorsize > XFS_MIN_SECTORSIZE && !params.lsu && !params.lsunit) {
+ params.lsu = blocksize;
sb_feat->log_version = 2;
}
@@ -2222,10 +2218,10 @@ _("rmapbt not supported with realtime devices\n"));
usage();
}
- if (logsize) {
+ if (params.logsize) {
__uint64_t logbytes;
- logbytes = getnum(logsize, &lopts, L_SIZE);
+ logbytes = getnum(params.logsize, &lopts, L_SIZE);
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal log length %lld, not a multiple of %d\n"),
@@ -2324,14 +2320,16 @@ _("rmapbt not supported with realtime devices\n"));
}
/* if lsu or lsunit was specified, automatically use v2 logs */
- if ((lsu || lsunit) && sb_feat->log_version == 1) {
+ if ((params.lsu || params.lsunit) && sb_feat->log_version == 1) {
fprintf(stderr,
_("log stripe unit specified, using v2 logs\n"));
sb_feat->log_version = 2;
}
- calc_stripe_factors(params.dsu, params.dsw, sectorsize, lsu, lsectorsize,
- ¶ms.dsunit, ¶ms.dswidth, &lsunit);
+ calc_stripe_factors(params.dsu, params.dsw, sectorsize, params.lsu,
+ params.lsectorsize,
+ ¶ms.dsunit, ¶ms.dswidth,
+ ¶ms.lsunit);
params.xi.setblksize = sectorsize;
@@ -2359,8 +2357,8 @@ _("rmapbt not supported with realtime devices\n"));
sector_mask = (__uint64_t)-1 << (MAX(params.sectorlog, 10) - BBSHIFT);
params.xi.dsize &= sector_mask;
params.xi.rtsize &= sector_mask;
- params.xi.logBBsize &= (__uint64_t)-1 << (MAX(lsectorlog, 10) - BBSHIFT);
-
+ params.xi.logBBsize &= (__uint64_t)-1 <<
+ (MAX(params.lsectorlog, 10) - BBSHIFT);
/* don't do discards on print-only runs or on files */
if (discard && !Nflag) {
@@ -2373,15 +2371,15 @@ _("rmapbt not supported with realtime devices\n"));
discard_blocks(params.xi.logdev, params.xi.logBBsize);
}
- if (!liflag && !ldflag)
- loginternal = params.xi.logdev == 0;
+ if (!params.liflag && !params.ldflag)
+ params.loginternal = params.xi.logdev == 0;
if (params.xi.logname)
- logfile = params.xi.logname;
- else if (loginternal)
- logfile = _("internal log");
+ params.logfile = params.xi.logname;
+ else if (params.loginternal)
+ params.logfile = _("internal log");
else if (params.xi.volname && params.xi.logdev)
- logfile = _("volume log");
- else if (!ldflag) {
+ params.logfile = _("volume log");
+ else if (!params.ldflag) {
fprintf(stderr, _("no log subvolume or internal log\n"));
usage();
}
@@ -2412,11 +2410,11 @@ _("rmapbt not supported with realtime devices\n"));
usage();
}
- if (loginternal && params.xi.logdev) {
+ if (params.loginternal && params.xi.logdev) {
fprintf(stderr,
_("can't have both external and internal logs\n"));
usage();
- } else if (loginternal && sectorsize != lsectorsize) {
+ } else if (params.loginternal && sectorsize != params.lsectorsize) {
fprintf(stderr,
_("data and log sector sizes must be equal for internal logs\n"));
usage();
@@ -2428,11 +2426,11 @@ _("rmapbt not supported with realtime devices\n"));
reported by the device (%u).\n"),
sectorsize, params.xi.dbsize);
}
- if (!loginternal && params.xi.lbsize > lsectorsize) {
+ if (!params.loginternal && params.xi.lbsize > params.lsectorsize) {
fprintf(stderr, _(
"Warning: the log subvolume sector size %u is less than the sector size\n\
reported by the device (%u).\n"),
- lsectorsize, params.xi.lbsize);
+ params.lsectorsize, params.xi.lbsize);
}
if (rtsize && params.xi.rtsize > 0 && params.xi.rtbsize > sectorsize) {
fprintf(stderr, _(
@@ -2640,24 +2638,24 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
* check that log sunit is modulo fsblksize or default it to dsunit.
*/
- if (lsunit) {
+ if (params.lsunit) {
/* convert from 512 byte blocks to fs blocks */
- lsunit = DTOBT(lsunit);
- } else if (sb_feat->log_version == 2 && loginternal && params.dsunit) {
+ params.lsunit = DTOBT(params.lsunit);
+ } else if (sb_feat->log_version == 2 && params.loginternal && params.dsunit) {
/* lsunit and dsunit now in fs blocks */
- lsunit = params.dsunit;
+ params.lsunit = params.dsunit;
}
- if (sb_feat->log_version == 2 && (lsunit * blocksize) > 256 * 1024) {
+ if (sb_feat->log_version == 2 && (params.lsunit * blocksize) > 256 * 1024) {
/* Warn only if specified on commandline */
- if (lsuflag || lsunitflag) {
+ if (params.lsuflag || params.lsunitflag) {
fprintf(stderr,
_("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
- (lsunit * blocksize));
+ (params.lsunit * blocksize));
fprintf(stderr,
_("log stripe unit adjusted to 32KiB\n"));
}
- lsunit = (32 * 1024) >> params.blocklog;
+ params.lsunit = (32 * 1024) >> params.blocklog;
}
min_logblocks = max_trans_res(params.agsize,
@@ -2665,31 +2663,32 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
params.sectorlog, params.blocklog,
params.inodelog,
dirblocklog,
- sb_feat->log_version, lsunit, sb_feat->finobt,
+ sb_feat->log_version, params.lsunit,
+ sb_feat->finobt,
sb_feat->rmapbt, sb_feat->reflink);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
- if (!logsize && dblocks >= (1024*1024*1024) >> params.blocklog)
+ if (!params.logsize && dblocks >= (1024*1024*1024) >> params.blocklog)
min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>params.blocklog);
- if (logsize && params.xi.logBBsize > 0 &&
+ if (params.logsize && params.xi.logBBsize > 0 &&
logblocks > DTOBT(params.xi.logBBsize)) {
fprintf(stderr,
_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
- logsize, (long long)DTOBT(params.xi.logBBsize));
+ params.logsize, (long long)DTOBT(params.xi.logBBsize));
usage();
- } else if (!logsize && params.xi.logBBsize > 0) {
+ } else if (!params.logsize && params.xi.logBBsize > 0) {
logblocks = DTOBT(params.xi.logBBsize);
- } else if (logsize && !params.xi.logdev && !loginternal) {
+ } else if (params.logsize && !params.xi.logdev && !params.loginternal) {
fprintf(stderr,
_("size specified for non-existent log subvolume\n"));
usage();
- } else if (loginternal && logsize && logblocks >= dblocks) {
+ } else if (params.loginternal && params.logsize && logblocks >= dblocks) {
fprintf(stderr, _("size %lld too large for internal log\n"),
(long long)logblocks);
usage();
- } else if (!loginternal && !params.xi.logdev) {
+ } else if (!params.loginternal && !params.xi.logdev) {
logblocks = 0;
- } else if (loginternal && !logsize) {
+ } else if (params.loginternal && !params.logsize) {
if (dblocks < GIGABYTES(1, params.blocklog)) {
/* tiny filesystems get minimum sized logs. */
@@ -2747,15 +2746,15 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* libxfs_prealloc_blocks().
*/
sb_set_features(&mp->m_sb, sb_feat, sectorsize,
- lsectorsize, params.dsunit);
+ params.lsectorsize, params.dsunit);
- if (loginternal) {
+ if (params.loginternal) {
/*
* Readjust the log size to fit within an AG if it was sized
* automatically.
*/
- if (!logsize) {
+ if (!params.logsize) {
logblocks = MIN(logblocks,
libxfs_alloc_ag_max_usable(mp));
@@ -2769,23 +2768,26 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
usage();
}
- if (laflag) {
- if (logagno >= params.agcount) {
+ if (params.laflag) {
+ if (params.logagno >= params.agcount) {
fprintf(stderr,
_("log ag number %d too large, must be less than %lld\n"),
- logagno, (long long)params.agcount);
+ params.logagno,
+ (long long)params.agcount);
usage();
}
} else
- logagno = (xfs_agnumber_t)(params.agcount / 2);
+ params.logagno = (xfs_agnumber_t)(params.agcount / 2);
- logstart = XFS_AGB_TO_FSB(mp, logagno, libxfs_prealloc_blocks(mp));
+ logstart = XFS_AGB_TO_FSB(mp, params.logagno,
+ libxfs_prealloc_blocks(mp));
/*
* Align the logstart at stripe unit boundary.
*/
- if (lsunit) {
+ if (params.lsunit) {
logstart = fixup_internal_log_stripe(mp,
- lsflag, logstart, params.agsize, lsunit,
+ lsflag, logstart, params.agsize,
+ params.lsunit,
&logblocks, params.blocklog, &lalign);
} else if (params.dsunit) {
logstart = fixup_internal_log_stripe(mp,
@@ -2796,9 +2798,9 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
}
} else {
logstart = 0;
- if (lsunit)
- fixup_log_stripe_unit(lsflag, lsunit,
- &logblocks, params.blocklog);
+ if (params.lsunit)
+ fixup_log_stripe_unit(lsflag, params.lsunit,
+ &logblocks, params.blocklog);
}
validate_log_size(logblocks, params.blocklog, min_logblocks);
@@ -2823,9 +2825,11 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
"", params.dsunit, params.dswidth,
sb_feat->dir_version, dirblocksize, sb_feat->nci,
sb_feat->dirftype,
- logfile, 1 << params.blocklog, (long long)logblocks,
- sb_feat->log_version, "", lsectorsize, lsunit,
- sb_feat->lazy_sb_counters,
+ params.logfile, 1 << params.blocklog,
+ (long long)logblocks,
+ sb_feat->log_version, "", params.lsectorsize,
+ params.lsunit,
+ sb_feat->lazy_sb_counters,
rtfile, rtextblocks << params.blocklog,
(long long)rtblocks, (long long)rtextents);
if (Nflag)
@@ -2862,7 +2866,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_icount = 0;
sbp->sb_ifree = 0;
sbp->sb_fdblocks = dblocks - params.agcount * libxfs_prealloc_blocks(mp) -
- (loginternal ? logblocks : 0);
+ (params.loginternal ? logblocks : 0);
sbp->sb_frextents = 0; /* will do a free later */
sbp->sb_uquotino = sbp->sb_gquotino = sbp->sb_pquotino = 0;
sbp->sb_qflags = 0;
@@ -2870,8 +2874,9 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_width = params.dswidth;
sbp->sb_dirblklog = dirblocklog - params.blocklog;
if (sb_feat->log_version == 2) { /* This is stored in bytes */
- lsunit = (lsunit == 0) ? 1 : XFS_FSB_TO_B(mp, lsunit);
- sbp->sb_logsunit = lsunit;
+ params.lsunit = (params.lsunit == 0) ? 1 : XFS_FSB_TO_B(mp,
+ params.lsunit);
+ sbp->sb_logsunit = params.lsunit;
} else
sbp->sb_logsunit = 0;
if (sb_feat->inode_align) {
@@ -2882,16 +2887,16 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sb_feat->inode_align = sbp->sb_inoalignmt != 0;
} else
sbp->sb_inoalignmt = 0;
- if (lsectorsize != BBSIZE || sectorsize != BBSIZE) {
- sbp->sb_logsectlog = (__uint8_t)lsectorlog;
- sbp->sb_logsectsize = (__uint16_t)lsectorsize;
+ if (params.lsectorsize != BBSIZE || sectorsize != BBSIZE) {
+ sbp->sb_logsectlog = (__uint8_t)params.lsectorlog;
+ sbp->sb_logsectsize = (__uint16_t) params.lsectorsize;
} else {
sbp->sb_logsectlog = 0;
sbp->sb_logsectsize = 0;
}
sb_set_features(&mp->m_sb, sb_feat, sectorsize,
- lsectorsize, params.dsunit);
+ params.lsectorsize, params.dsunit);
if (force_overwrite)
zero_old_xfs_structures(¶ms.xi, sbp);
@@ -2952,7 +2957,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
libxfs_log_clear(mp->m_logdev_targp, NULL,
XFS_FSB_TO_DADDR(mp, logstart),
(xfs_extlen_t)XFS_FSB_TO_BB(mp, logblocks),
- &sbp->sb_uuid, sb_feat->log_version, lsunit,
+ &sbp->sb_uuid, sb_feat->log_version, params.lsunit,
XLOG_FMT, XLOG_INIT_CYCLE, false);
mp = libxfs_mount(mp, sbp, params.xi.ddev, params.xi.logdev,
@@ -3028,7 +3033,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
if (xfs_sb_version_hascrc(&mp->m_sb))
platform_uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_uuid);
- if (loginternal && agno == logagno) {
+ if (params.loginternal && agno == params.logagno) {
be32_add_cpu(&agf->agf_freeblks, -logblocks);
agf->agf_longest = cpu_to_be32(params.agsize -
XFS_FSB_TO_AGBNO(mp, logstart) - logblocks);
@@ -3104,7 +3109,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
arec = XFS_ALLOC_REC_ADDR(mp, block, 1);
arec->ar_startblock = cpu_to_be32(libxfs_prealloc_blocks(mp));
- if (loginternal && agno == logagno) {
+ if (params.loginternal && agno == params.logagno) {
if (lalign) {
/*
* Have to insert two records
@@ -3159,7 +3164,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
arec = XFS_ALLOC_REC_ADDR(mp, block, 1);
arec->ar_startblock = cpu_to_be32(libxfs_prealloc_blocks(mp));
- if (loginternal && agno == logagno) {
+ if (params.loginternal && agno == params.logagno) {
if (lalign) {
arec->ar_blockcount = cpu_to_be32(
XFS_FSB_TO_AGBNO(mp, logstart) -
@@ -3304,7 +3309,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
}
/* account for the log space */
- if (loginternal && agno == logagno) {
+ if (params.loginternal && agno == params.logagno) {
rrec = XFS_RMAP_REC_ADDR(block,
be16_to_cpu(block->bb_numrecs) + 1);
rrec->rm_startblock = cpu_to_be32(
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 5/9] mkfs.xfs: move mopts to struct mkfs_xfs_opts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (3 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 4/9] mkfs.xfs: move lopts " Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 6/9] mkfs.xfs: move nopts " Luis R. Rodriguez
` (6 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
This moves all main() mopts to struct mkfs_xfs_opts in
order to help with clutter and later enable re-parsing
options for other purposes.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 78 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 38 insertions(+), 40 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index cd52b525f824..52392c4e2c50 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -772,6 +772,7 @@ struct mkfs_xfs_opts {
int lvflag;
int lslflag;
int lssflag;
+ uuid_t uuid;
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -1651,6 +1652,39 @@ parse_subopts(
}
}
break;
+ case 'm':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)mopts.subopts, &value)) {
+ case M_CRC:
+ params->sb_feat.crcs_enabled =
+ getnum(value, &mopts, M_CRC);
+ if (params->sb_feat.crcs_enabled)
+ params->sb_feat.dirftype = true;
+ break;
+ case M_FINOBT:
+ params->sb_feat.finobt =
+ getnum(value, &mopts, M_FINOBT);
+ break;
+ case M_UUID:
+ if (!value || *value == '\0')
+ reqval('m', (char **)mopts.subopts,
+ M_UUID);
+ if (platform_uuid_parse(value, ¶ms->uuid))
+ illegal(optarg, "m uuid");
+ break;
+ case M_RMAPBT:
+ params->sb_feat.rmapbt =
+ getnum(value, &mopts, M_RMAPBT);
+ break;
+ case M_REFLINK:
+ params->sb_feat.reflink =
+ getnum(value, &mopts, M_REFLINK);
+ break;
+ default:
+ unknown('m', value);
+ }
+ }
+ break;
default:
usage();
}
@@ -1721,7 +1755,6 @@ main(
xfs_sb_t *sbp;
__uint64_t sector_mask;
__uint64_t tmp_agsize;
- uuid_t uuid;
int worst_freelist;
struct fs_topology ft;
struct mkfs_xfs_opts params;
@@ -1730,7 +1763,7 @@ main(
memset(¶ms, 0, sizeof(params));
init_sb_feat_args_default(sb_feat);
- platform_uuid_generate(&uuid);
+ platform_uuid_generate(¶ms.uuid);
progname = basename(argv[0]);
setlocale(LC_ALL, "");
bindtextdomain(PACKAGE, LOCALEDIR);
@@ -1765,6 +1798,7 @@ main(
case 'd':
case 'i':
case 'l':
+ case 'm':
p = optarg;
parse_subopts(c, p, ¶ms);
break;
@@ -1773,42 +1807,6 @@ main(
illegal(optarg, "L");
label = optarg;
break;
- case 'm':
- p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)mopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case M_CRC:
- sb_feat->crcs_enabled =
- getnum(value, &mopts, M_CRC);
- if (sb_feat->crcs_enabled)
- sb_feat->dirftype = true;
- break;
- case M_FINOBT:
- sb_feat->finobt = getnum(
- value, &mopts, M_FINOBT);
- break;
- case M_UUID:
- if (!value || *value == '\0')
- reqval('m', subopts, M_UUID);
- if (platform_uuid_parse(value, &uuid))
- illegal(optarg, "m uuid");
- break;
- case M_RMAPBT:
- sb_feat->rmapbt = getnum(
- value, &mopts, M_RMAPBT);
- break;
- case M_REFLINK:
- sb_feat->reflink = getnum(
- value, &mopts, M_REFLINK);
- break;
- default:
- unknown('m', value);
- }
- }
- break;
case 'n':
p = optarg;
while (*p != '\0') {
@@ -2843,9 +2841,9 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_dblocks = dblocks;
sbp->sb_rblocks = rtblocks;
sbp->sb_rextents = rtextents;
- platform_uuid_copy(&sbp->sb_uuid, &uuid);
+ platform_uuid_copy(&sbp->sb_uuid, ¶ms.uuid);
/* Only in memory; libxfs expects this as if read from disk */
- platform_uuid_copy(&sbp->sb_meta_uuid, &uuid);
+ platform_uuid_copy(&sbp->sb_meta_uuid, ¶ms.uuid);
sbp->sb_logstart = logstart;
sbp->sb_rootino = sbp->sb_rbmino = sbp->sb_rsumino = NULLFSINO;
sbp->sb_rextsize = rtextblocks;
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 6/9] mkfs.xfs: move nopts to struct mkfs_xfs_opts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (4 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 5/9] mkfs.xfs: move mopts " Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 7/9] mkfs.xfs: move ropts " Luis R. Rodriguez
` (5 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
This moves all main() X-opts to struct mkfs_xfs_opts in
order to help with clutter and later enable re-parsing
options for other purposes.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 116 ++++++++++++++++++++++++++++----------------------------
1 file changed, 57 insertions(+), 59 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 52392c4e2c50..b2f4495d858d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -773,6 +773,12 @@ struct mkfs_xfs_opts {
int lslflag;
int lssflag;
uuid_t uuid;
+
+ int dirblocklog;
+ int dirblocksize;
+ int nlflag;
+ int nvflag; /* unused */
+ int nsflag;
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -1685,6 +1691,43 @@ parse_subopts(
}
}
break;
+ case 'n':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)nopts.subopts, &value)) {
+ case N_LOG:
+ params->dirblocklog =
+ getnum(value, &nopts, N_LOG);
+ params->dirblocksize = 1 << params->dirblocklog;
+ params->nlflag = 1;
+ break;
+ case N_SIZE:
+ params->dirblocksize =
+ getnum(value, &nopts, N_SIZE);
+ params->dirblocklog =
+ libxfs_highbit32(params->dirblocksize);
+ params->nsflag = 1;
+ break;
+ case N_VERSION:
+ value = getstr(value, &nopts, N_VERSION);
+ if (!strcasecmp(value, "ci")) {
+ /* ASCII CI mode */
+ params->sb_feat.nci = true;
+ } else {
+ params->sb_feat.dir_version =
+ getnum(value, &nopts,
+ N_VERSION);
+ }
+ params->nvflag = 1;
+ break;
+ case N_FTYPE:
+ params->sb_feat.dirftype =
+ getnum(value, &nopts, N_FTYPE);
+ break;
+ default:
+ unknown('n', value);
+ }
+ }
+ break;
default:
usage();
}
@@ -1723,8 +1766,6 @@ main(
int c;
xfs_rfsblock_t dblocks;
char *dfile;
- int dirblocklog;
- int dirblocksize;
int force_overwrite;
char *label = NULL;
int lalign;
@@ -1735,11 +1776,8 @@ main(
xfs_mount_t *mp;
xfs_mount_t mbuf;
xfs_extlen_t nbmblocks;
- int nlflag;
int norsflag;
xfs_alloc_rec_t *nrec;
- int nsflag;
- int nvflag;
int Nflag;
int discard = 1;
char *p;
@@ -1775,8 +1813,7 @@ main(
params.loginternal = 1;
lsflag = 0;
logblocks = rtblocks = rtextblocks = 0;
- Nflag = nlflag = nsflag = nvflag = 0;
- dirblocklog = dirblocksize = 0;
+ Nflag = 0;
qflag = 0;
dfile = rtfile = NULL;
rtsize = rtextsize = protofile = NULL;
@@ -1799,6 +1836,7 @@ main(
case 'i':
case 'l':
case 'm':
+ case 'n':
p = optarg;
parse_subopts(c, p, ¶ms);
break;
@@ -1807,47 +1845,6 @@ main(
illegal(optarg, "L");
label = optarg;
break;
- case 'n':
- p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)nopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case N_LOG:
- dirblocklog = getnum(value, &nopts,
- N_LOG);
- dirblocksize = 1 << dirblocklog;
- nlflag = 1;
- break;
- case N_SIZE:
- dirblocksize = getnum(value, &nopts,
- N_SIZE);
- dirblocklog =
- libxfs_highbit32(dirblocksize);
- nsflag = 1;
- break;
- case N_VERSION:
- value = getstr(value, &nopts, N_VERSION);
- if (!strcasecmp(value, "ci")) {
- /* ASCII CI mode */
- sb_feat->nci = true;
- } else {
- sb_feat->dir_version =
- getnum(value, &nopts,
- N_VERSION);
- }
- nvflag = 1;
- break;
- case N_FTYPE:
- sb_feat->dirftype = getnum(value, &nopts,
- N_FTYPE);
- break;
- default:
- unknown('n', value);
- }
- }
- break;
case 'N':
Nflag = 1;
break;
@@ -2168,19 +2165,19 @@ _("rmapbt not supported with realtime devices\n"));
sb_feat->rmapbt = false;
}
- if (nsflag || nlflag) {
- if (dirblocksize < blocksize ||
- dirblocksize > XFS_MAX_BLOCKSIZE) {
+ if (params.nsflag || params.nlflag) {
+ if (params.dirblocksize < blocksize ||
+ params.dirblocksize > XFS_MAX_BLOCKSIZE) {
fprintf(stderr, _("illegal directory block size %d\n"),
- dirblocksize);
+ params.dirblocksize);
usage();
}
} else {
if (blocksize < (1 << XFS_MIN_REC_DIRSIZE))
- dirblocklog = XFS_MIN_REC_DIRSIZE;
+ params.dirblocklog = XFS_MIN_REC_DIRSIZE;
else
- dirblocklog = params.blocklog;
- dirblocksize = 1 << dirblocklog;
+ params.dirblocklog = params.blocklog;
+ params.dirblocksize = 1 << params.dirblocklog;
}
@@ -2660,7 +2657,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
sb_feat->crcs_enabled, sb_feat->dir_version,
params.sectorlog, params.blocklog,
params.inodelog,
- dirblocklog,
+ params.dirblocklog,
sb_feat->log_version, params.lsunit,
sb_feat->finobt,
sb_feat->rmapbt, sb_feat->reflink);
@@ -2821,8 +2818,9 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sb_feat->rmapbt, sb_feat->reflink,
"", blocksize, (long long)dblocks, params.imaxpct,
"", params.dsunit, params.dswidth,
- sb_feat->dir_version, dirblocksize, sb_feat->nci,
- sb_feat->dirftype,
+ sb_feat->dir_version, params.dirblocksize,
+ sb_feat->nci,
+ sb_feat->dirftype,
params.logfile, 1 << params.blocklog,
(long long)logblocks,
sb_feat->log_version, "", params.lsectorsize,
@@ -2870,7 +2868,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_qflags = 0;
sbp->sb_unit = params.dsunit;
sbp->sb_width = params.dswidth;
- sbp->sb_dirblklog = dirblocklog - params.blocklog;
+ sbp->sb_dirblklog = params.dirblocklog - params.blocklog;
if (sb_feat->log_version == 2) { /* This is stored in bytes */
params.lsunit = (params.lsunit == 0) ? 1 : XFS_FSB_TO_B(mp,
params.lsunit);
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 7/9] mkfs.xfs: move ropts to struct mkfs_xfs_opts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (5 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 6/9] mkfs.xfs: move nopts " Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 8/9] mkfs.xfs: use parse_subopts() to parse sopts Luis R. Rodriguez
` (4 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
This moves all main() ropts to struct mkfs_xfs_opts in
order to help with clutter and later enable re-parsing
options for other purposes.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 98 +++++++++++++++++++++++++++------------------------------
1 file changed, 47 insertions(+), 51 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index b2f4495d858d..39ab08148687 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -779,6 +779,10 @@ struct mkfs_xfs_opts {
int nlflag;
int nvflag; /* unused */
int nsflag;
+
+ char *rtextsize;
+ char *rtsize;
+ int norsflag;
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -1728,6 +1732,34 @@ parse_subopts(
}
}
break;
+ case 'r':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)ropts.subopts, &value)) {
+ case R_EXTSIZE:
+ params->rtextsize =
+ getstr(value, &ropts, R_EXTSIZE);
+ break;
+ case R_FILE:
+ params->xi.risfile =
+ getnum(value, &ropts, R_FILE);
+ break;
+ case R_NAME:
+ case R_DEV:
+ params->xi.rtname =
+ getstr(value, &ropts, R_NAME);
+ break;
+ case R_SIZE:
+ params->rtsize = getstr(value, &ropts, R_SIZE);
+ break;
+ case R_NOALIGN:
+ params->norsflag =
+ getnum(value, &ropts, R_NOALIGN);
+ break;
+ default:
+ unknown('r', value);
+ }
+ }
+ break;
default:
usage();
}
@@ -1776,7 +1808,6 @@ main(
xfs_mount_t *mp;
xfs_mount_t mbuf;
xfs_extlen_t nbmblocks;
- int norsflag;
xfs_alloc_rec_t *nrec;
int Nflag;
int discard = 1;
@@ -1787,9 +1818,7 @@ main(
xfs_rfsblock_t rtblocks;
xfs_extlen_t rtextblocks;
xfs_rtblock_t rtextents;
- char *rtextsize;
char *rtfile;
- char *rtsize;
xfs_sb_t *sbp;
__uint64_t sector_mask;
__uint64_t tmp_agsize;
@@ -1815,10 +1844,8 @@ main(
logblocks = rtblocks = rtextblocks = 0;
Nflag = 0;
qflag = 0;
- dfile = rtfile = NULL;
- rtsize = rtextsize = protofile = NULL;
+ dfile = rtfile = protofile = NULL;
lalign = 0;
- norsflag = 0;
force_overwrite = 0;
worst_freelist = 0;
@@ -1837,6 +1864,7 @@ main(
case 'l':
case 'm':
case 'n':
+ case 'r':
p = optarg;
parse_subopts(c, p, ¶ms);
break;
@@ -1859,38 +1887,6 @@ main(
case 'q':
qflag = 1;
break;
- case 'r':
- p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)ropts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case R_EXTSIZE:
- rtextsize = getstr(value, &ropts,
- R_EXTSIZE);
- break;
- case R_FILE:
- params.xi.risfile = getnum(value, &ropts,
- R_FILE);
- break;
- case R_NAME:
- case R_DEV:
- params.xi.rtname = getstr(value, &ropts,
- R_NAME);
- break;
- case R_SIZE:
- rtsize = getstr(value, &ropts, R_SIZE);
- break;
- case R_NOALIGN:
- norsflag = getnum(value, &ropts,
- R_NOALIGN);
- break;
- default:
- unknown('r', value);
- }
- }
- break;
case 's':
p = optarg;
while (*p != '\0') {
@@ -1997,7 +1993,7 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
if (params.xi.rtname)
check_device_type(params.xi.rtname,
¶ms.xi.risfile,
- !rtsize,
+ !params.rtsize,
!params.xi.rtname,
Nflag ? NULL : ¶ms.xi.rcreat,
force_overwrite, "r");
@@ -2230,10 +2226,10 @@ _("rmapbt not supported with realtime devices\n"));
(long long)logbytes, blocksize,
(long long)(logblocks << params.blocklog));
}
- if (rtsize) {
+ if (params.rtsize) {
__uint64_t rtbytes;
- rtbytes = getnum(rtsize, &ropts, R_SIZE);
+ rtbytes = getnum(params.rtsize, &ropts, R_SIZE);
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal rt length %lld, not a multiple of %d\n"),
@@ -2250,10 +2246,10 @@ _("rmapbt not supported with realtime devices\n"));
/*
* If specified, check rt extent size against its constraints.
*/
- if (rtextsize) {
+ if (params.rtextsize) {
__uint64_t rtextbytes;
- rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
+ rtextbytes = getnum(params.rtextsize, &ropts, R_EXTSIZE);
if (rtextbytes % blocksize) {
fprintf(stderr,
_("illegal rt extent size %lld, not a multiple of %d\n"),
@@ -2270,14 +2266,14 @@ _("rmapbt not supported with realtime devices\n"));
__uint64_t rswidth;
__uint64_t rtextbytes;
- if (!norsflag && !params.xi.risfile &&
- !(!rtsize && params.xi.disfile))
+ if (!params.norsflag && !params.xi.risfile &&
+ !(!params.rtsize && params.xi.disfile))
rswidth = ft.rtswidth;
else
rswidth = 0;
/* check that rswidth is a multiple of fs blocksize */
- if (!norsflag && rswidth && !(BBTOB(rswidth) % blocksize)) {
+ if (!params.norsflag && rswidth && !(BBTOB(rswidth) % blocksize)) {
rswidth = DTOBT(rswidth);
rtextbytes = rswidth << params.blocklog;
if (XFS_MIN_RTEXTSIZE <= rtextbytes &&
@@ -2427,22 +2423,22 @@ reported by the device (%u).\n"),
reported by the device (%u).\n"),
params.lsectorsize, params.xi.lbsize);
}
- if (rtsize && params.xi.rtsize > 0 && params.xi.rtbsize > sectorsize) {
+ if (params.rtsize && params.xi.rtsize > 0 && params.xi.rtbsize > sectorsize) {
fprintf(stderr, _(
"Warning: the realtime subvolume sector size %u is less than the sector size\n\
reported by the device (%u).\n"),
sectorsize, params.xi.rtbsize);
}
- if (rtsize && params.xi.rtsize > 0 && rtblocks > DTOBT(params.xi.rtsize)) {
+ if (params.rtsize && params.xi.rtsize > 0 && rtblocks > DTOBT(params.xi.rtsize)) {
fprintf(stderr,
_("size %s specified for rt subvolume is too large, "
"maximum is %lld blocks\n"),
- rtsize, (long long)DTOBT(params.xi.rtsize));
+ params.rtsize, (long long)DTOBT(params.xi.rtsize));
usage();
- } else if (!rtsize && params.xi.rtsize > 0)
+ } else if (!params.rtsize && params.xi.rtsize > 0)
rtblocks = DTOBT(params.xi.rtsize);
- else if (rtsize && !params.xi.rtdev) {
+ else if (params.rtsize && !params.xi.rtdev) {
fprintf(stderr,
_("size specified for non-existent rt subvolume\n"));
usage();
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 8/9] mkfs.xfs: use parse_subopts() to parse sopts
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (6 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 7/9] mkfs.xfs: move ropts " Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:13 ` [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support Luis R. Rodriguez
` (3 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
All sopt parameters are already part of struct mkfs_xfs_opts, so
all we need to do is use the helper.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 70 +++++++++++++++++++++++++++------------------------------
1 file changed, 33 insertions(+), 37 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 39ab08148687..2a3786ece6b6 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1760,6 +1760,38 @@ parse_subopts(
}
}
break;
+ case 's':
+ while (*p != '\0') {
+ switch (getsubopt(&p, (char **)sopts.subopts, &value)) {
+ case S_LOG:
+ case S_SECTLOG:
+ if (params->lssflag)
+ conflict('s', (char **)sopts.subopts,
+ S_SECTSIZE, S_SECTLOG);
+ params->sectorlog =
+ getnum(value, &sopts, S_SECTLOG);
+ params->lsectorlog = params->sectorlog;
+ sectorsize = 1 << params->sectorlog;
+ params->lsectorsize = sectorsize;
+ params->lslflag = params->slflag = 1;
+ break;
+ case S_SIZE:
+ case S_SECTSIZE:
+ if (params->lslflag)
+ conflict('s', (char **)sopts.subopts,
+ S_SECTLOG, S_SECTSIZE);
+ sectorsize = getnum(value, &sopts, S_SECTSIZE);
+ params->lsectorsize = sectorsize;
+ params->sectorlog =
+ libxfs_highbit32(sectorsize);
+ params->lsectorlog = params->sectorlog;
+ params->lssflag = params->ssflag = 1;
+ break;
+ default:
+ unknown('s', value);
+ }
+ }
+ break;
default:
usage();
}
@@ -1865,6 +1897,7 @@ main(
case 'm':
case 'n':
case 'r':
+ case 's':
p = optarg;
parse_subopts(c, p, ¶ms);
break;
@@ -1887,43 +1920,6 @@ main(
case 'q':
qflag = 1;
break;
- case 's':
- p = optarg;
- while (*p != '\0') {
- char **subopts = (char **)sopts.subopts;
- char *value;
-
- switch (getsubopt(&p, subopts, &value)) {
- case S_LOG:
- case S_SECTLOG:
- if (params.lssflag)
- conflict('s', subopts,
- S_SECTSIZE, S_SECTLOG);
- params.sectorlog = getnum(value, &sopts,
- S_SECTLOG);
- params.lsectorlog = params.sectorlog;
- sectorsize = 1 << params.sectorlog;
- params.lsectorsize = sectorsize;
- params.lslflag = params.slflag = 1;
- break;
- case S_SIZE:
- case S_SECTSIZE:
- if (params.lslflag)
- conflict('s', subopts, S_SECTLOG,
- S_SECTSIZE);
- sectorsize = getnum(value, &sopts,
- S_SECTSIZE);
- params.lsectorsize = sectorsize;
- params.sectorlog =
- libxfs_highbit32(sectorsize);
- params.lsectorlog = params.sectorlog;
- params.lssflag = params.ssflag = 1;
- break;
- default:
- unknown('s', value);
- }
- }
- break;
case 'V':
printf(_("%s version %s\n"), progname, VERSION);
exit(0);
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (7 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 8/9] mkfs.xfs: use parse_subopts() to parse sopts Luis R. Rodriguez
@ 2017-03-03 23:13 ` Luis R. Rodriguez
2017-03-03 23:55 ` Dave Chinner
2017-03-09 5:38 ` Eric Sandeen
2017-03-03 23:24 ` [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (2 subsequent siblings)
11 siblings, 2 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:13 UTC (permalink / raw)
To: sandeen, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek, Luis R. Rodriguez
You may want to stick to specific set of configuration options when
creating filesystems with mkfs.xfs -- sometimes due to pure technical
reasons, but some other times to ensure systems remain compatible as
new features are introduced with older kernels, or if you always want
to take advantage of some new feature which would otherwise typically
be disruptive.
Although mkfs.xfs already uses sensible defaults this adds a configuration
option for parsing defaults settings for mkfs.xfs parsed prior to processing
input arguments.
User input passed to mkfs.xfs overrides defaults founds through the
new optional configuration file, by default:
/etc/mkfs.xfs.conf
To use /etc/ be sure to configure xfsprogs with:
./configure --sysconfdir=/etc/
The build system also allows distributions to override the default
mkfs.xfs.conf defaults with a custom:
etc/mkfs.xfs.conf.custom.in
The default etc/mkfs.xfs.conf.in provides commented out examples.
You can also override the configuration file used either with the
MKFS_XFS_CONFIG environment variable or by using the new -c command
line argument to mkfs.xfs. Only when -c is used will the configuration
file be required to be present.
To verify what configuration file is used on a system use the typical:
mkfs.xfs -N
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
.gitignore | 3 ++
Makefile | 2 +-
etc/Makefile | 21 ++++++++
etc/mkfs.xfs.conf.in | 58 ++++++++++++++++++++
include/builddefs.in | 2 +
include/buildmacros | 6 +++
man/man5/mkfs.xfs.conf | 113 +++++++++++++++++++++++++++++++++++++++
man/man8/mkfs.xfs.8 | 28 ++++++++++
mkfs/xfs_mkfs.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++-
9 files changed, 371 insertions(+), 2 deletions(-)
create mode 100644 etc/Makefile
create mode 100644 etc/mkfs.xfs.conf.in
create mode 100644 man/man5/mkfs.xfs.conf
diff --git a/.gitignore b/.gitignore
index 913b371fce5f..869c59ce30b1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,3 +72,6 @@ cscope.*
/libxfs/crc32selftest
/libxfs/crc32table.h
/libxfs/gen_crc32table
+
+# etc files
+/etc/mkfs.xfs.conf
diff --git a/Makefile b/Makefile
index 6e45733ee6de..02e0e2e7c1ee 100644
--- a/Makefile
+++ b/Makefile
@@ -47,7 +47,7 @@ HDR_SUBDIRS = include libxfs
DLIB_SUBDIRS = libxlog libxcmd libhandle
LIB_SUBDIRS = libxfs $(DLIB_SUBDIRS)
TOOL_SUBDIRS = copy db estimate fsck growfs io logprint mkfs quota \
- mdrestore repair rtcp m4 man doc debian
+ mdrestore repair rtcp m4 man etc doc debian
ifneq ("$(PKG_PLATFORM)","darwin")
TOOL_SUBDIRS += fsr
diff --git a/etc/Makefile b/etc/Makefile
new file mode 100644
index 000000000000..a9b3dd75c343
--- /dev/null
+++ b/etc/Makefile
@@ -0,0 +1,21 @@
+TOPDIR = ..
+include $(TOPDIR)/include/builddefs
+
+ETC_FILES = mkfs.xfs.conf
+ETC_DEST = $(PKG_ETC_DIR)
+
+default : $(ETC_FILES)
+
+include $(BUILDRULES)
+
+install : default
+ $(INSTALL) -m 755 -d $(ETC_DEST)
+ $(INSTALL_ETC)
+install-dev :
+
+mkfs.xfs.conf: mkfs.xfs.conf.in
+ @if test -f mkfs.xfs.conf.custom.in ; then \
+ cp mkfs.xfs.conf.custom.in mkfs.xfs.conf; \
+ else \
+ cp mkfs.xfs.conf.in mkfs.xfs.conf; \
+ fi
diff --git a/etc/mkfs.xfs.conf.in b/etc/mkfs.xfs.conf.in
new file mode 100644
index 000000000000..0999f4ddfc02
--- /dev/null
+++ b/etc/mkfs.xfs.conf.in
@@ -0,0 +1,58 @@
+# Options assigned values mean they are the current mkfs.xfs defaults
+#[block_size_options]
+#log =
+#size =
+
+#[metadata_options]
+#crc = 1
+#finobt = 1
+#uuid =
+#rmapbt = 0
+#reflink = 0
+
+#[data_section_options]
+#agcount =
+#agsize =
+#name =
+#file = 1
+#size=
+#sunit=
+#su=
+#swidth=
+#sw=
+#noalign = 1
+
+#[inode_options]
+#size =
+#log =
+#perblock =
+#maxpct =
+#align =
+#attr =
+#projid32bit = 1
+#sparse = 1
+
+#[log_section_options]
+#internal = 1
+#logdev =
+#size =
+#version =
+#sunit =
+#su =
+#lazy-count = 1111
+
+#[naming_options]
+#size =
+#log =
+#version =
+#ftype = 1
+
+#[realtime_section_options]
+#rtdev =
+#extsize =
+#size =
+#noalign = 1
+
+#[sector_size]
+#size =
+#log =
diff --git a/include/builddefs.in b/include/builddefs.in
index 4d6bb2d8f6c2..a3af06ad0ef8 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -60,6 +60,7 @@ PKG_LIB_DIR = @libdir@@libdirsuffix@
PKG_INC_DIR = @includedir@/xfs
DK_INC_DIR = @includedir@/disk
PKG_MAN_DIR = @mandir@
+PKG_ETC_DIR = @sysconfdir@
PKG_DOC_DIR = @datadir@/doc/@pkg_name@
PKG_LOCALE_DIR = @datadir@/locale
@@ -154,6 +155,7 @@ endif
GCFLAGS = $(DEBUG) \
-DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\" \
+ -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\" \
-DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
ifeq ($(ENABLE_GETTEXT),yes)
diff --git a/include/buildmacros b/include/buildmacros
index a7c5d8ae2649..97b335fa615d 100644
--- a/include/buildmacros
+++ b/include/buildmacros
@@ -117,6 +117,12 @@ INSTALL_MAN = \
done; \
done
+INSTALL_ETC = \
+ @for d in $(ETC_FILES); do \
+ echo $(INSTALL) $d -m 644 $(ETC_DEST); \
+ $(INSTALL) -m 644 $${d} $(ETC_DEST) ; \
+ done
+
ifeq ($(ENABLE_GETTEXT),yes)
INSTALL_LINGUAS = \
@for l in $(LINGUAS) ""; do \
diff --git a/man/man5/mkfs.xfs.conf b/man/man5/mkfs.xfs.conf
new file mode 100644
index 000000000000..38b4ff6763b9
--- /dev/null
+++ b/man/man5/mkfs.xfs.conf
@@ -0,0 +1,113 @@
+.TH mkfs.xfs.conf 5
+.SH NAME
+mkfs.xfs.conf \- Configuration file for mkfs.xfs
+.SH DESCRIPTION
+.BR mkfs.xfs.conf (5)
+is the configuration for
+.BR mkfs.xfs (8).
+It controls the default parameters used by
+.BR mkfs.xfs (8)
+when it is creating an
+.BR xfs (5)
+filesystem.
+.PP
+The
+.BR mkfs.xfs.conf (5)
+file is optional, but if used any command line options passed directly
+.BR mkfs.xfs (8)
+always override configuration file options. The
+.BR mkfs.xfs.conf (5)
+location defaults to /etc/mkfs.xfs.conf, you can however override
+the default configuration file used by either using the environment variable
+MKFS_XFS_CONFIG or using the -c argument to
+.BR mkfs.xfs (8).
+Both the default configuration file and the configuration file searched for
+if MKFS_XFS_CONFIG is set are treated as an optional, if these files are
+present default options set by
+.BR mkfs.xfs (8)
+will be used. If you however use the -c argument to
+.BR mkfs.xfs (8)
+the configuration file will be required to be present. To verify what
+configuration file is used by
+.BR mkfs.xfs (8)
+you can use the -N argument to
+.BR mkfs.xfs (8)
+to look for the config-file path.
+The
+.BR mkfs.xfs.conf (5)
+file uses an INI-style format. Stanzas, or top-level sections, are delimited
+by square braces: [ ]. Within each section, each line defines a relation
+which assigns tags to values. Spaces and tabs are ignored. An example of the
+INI-style format used by this configuration file follows below:
+.P
+ # Some comment
+.br
+ [block_size_options]
+.br
+ log = 10
+.br
+.P
+ # More comments
+.br
+ [data_section_options]
+.br
+ agcount = 20
+.P
+Comments are delimited by a semicolon (';') or a hash ('#') character at the
+beginning of the comment, and are terminated by the end of line character.
+.P
+The following are valid stanzas allowed in the
+.BR mkfs.xfs.conf (5)
+file. All allowed tag and corresponding allowed values for each stanza are
+documented on the
+.BR mkfs.xfs (8)
+manual, each allowed tag corresponds to the allowed tags under each
+corresponding stanza on the manual. There are a few options part of
+.BR mkfs.xfs (8)
+which do not have a dedicated stanza, and as such there is no support
+to specify them in the configuration file. Only the stanzas
+described below are allowed on the configuration file:
+.TP
+.I [block_size_options]
+Contains options which specify the fundamental block size of the filesystem.
+.TP
+.I [global_metadata_options]
+Contains options which specify metadata format options that either apply to
+the entire filesystem or aren't easily characterised by a specific
+functionality group.
+.TP
+.I [data_section_options]
+Contains options which specify the location, size, and other parameters of the
+data section of the filesystem.
+.TP
+.I [inode_options]
+Contains options which specifies the inode size of the filesystem, and other
+inode allocation parameters.
+.TP
+.I [log_section_options]
+Contains options which specify the location, size, and other parameters of the
+log section of the filesystem.
+.TP
+.I [naming_options]
+Contains options which specify the version and size parameters for the naming
+(directory) area of the filesystem.
+.TP
+.I [realtime_section_options]
+Contains options which specify the location, size, and other parameters of the
+real-time section of the filesystem.
+.TP
+.I [sector_size]
+Contains options which specify the fundamental sector size of the filesystem.
+.P
+
+.SH SEE ALSO
+.BR xfs (5)
+.BR mkfs.xfs (8)
+.BR xfs_growfs (8)
+.BR xfs_admin (8),
+.BR xfsdump (8)
+.BR xfsrestore (8)
+.BR xfsctl (3)
+.BR xfs_info (8),
+.BR chattr (1),
+.BR mount (8)
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index b2bc223b0e1f..23606dd8d57b 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -83,6 +83,28 @@ and
.B \-l internal \-l size=10m
are equivalent.
.PP
+An optional configuration file
+.B mkfs.xfs.conf (5)
+can also be used to help fine tune default parameters which should be
+used when calling
+.B mkfs.xfs (8).
+Command line arguments directly passed to
+.B mkfs.xfs (8)
+will always override
+.B mkfs.xfs.conf (5)
+configuration parameters. The default
+.B mkfs.xfs.conf (5)
+used is /etc/mkfs.xfs.conf, you can however override this either
+by using the MKFS_XFS_CONFIG environment variable or using the
+-c option to
+.B mkfs.xfs (8).
+The
+.B mkfs.xfs.conf (8)
+file will be required to be present if you used the -c argument to
+.B mkfs.xfs (8).
+To verify what configuration file is used use the -N option to
+.B mkfs.xfs (8).
+.PP
In the descriptions below, sizes are given in sectors, bytes, blocks,
kilobytes, megabytes, gigabytes, etc.
Sizes are treated as hexadecimal if prefixed by 0x or 0X,
@@ -123,6 +145,11 @@ Many feature options allow an optional argument of 0 or 1, to explicitly
disable or enable the functionality.
.SH OPTIONS
.TP
+.BI \-c " path-to-custom-mkfs.xfs.conf"
+Override the default
+.B mkfs.xfs.conf
+file used, the full path to your custom must be specified.
+.TP
.BI \-b " block_size_options"
This option specifies the fundamental block size of the filesystem.
The valid
@@ -902,6 +929,7 @@ Prints the version number and exits.
.SH SEE ALSO
.BR xfs (5),
.BR mkfs (8),
+.BR mkfs.xfs.conf (5),
.BR mount (8),
.BR xfs_info (8),
.BR xfs_admin (8).
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2a3786ece6b6..aa14ef0cddf6 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -24,6 +24,8 @@
#include "xfs_multidisk.h"
#include "libxcmd.h"
+static const char *mkfs_config = ROOT_SYSCONFDIR "/mkfs.xfs.conf";
+
/*
* Prototypes for internal functions.
*/
@@ -1330,6 +1332,24 @@ illegal_option(
usage();
}
+static void
+reset_opt(
+ struct opt_params *opts,
+ int index)
+{
+ struct subopt_param *sp = &opts->subopt_params[index];
+
+ if (sp->index != index) {
+ fprintf(stderr,
+ ("Developer screwed up option parsing (%d/%d)! Please report!\n"),
+ sp->index, index);
+ reqval(opts->name, (char **)opts->subopts, index);
+ }
+
+ sp->seen = false;
+ sp->str_seen = false;
+}
+
/*
* Check for conflicts and option respecification.
*/
@@ -1467,6 +1487,7 @@ parse_subopts(
while (*p != '\0') {
switch (getsubopt(&p, (char **)bopts.subopts, &value)) {
case B_LOG:
+ reset_opt(&bopts, B_LOG);
params->blocklog = getnum(value, &bopts, B_LOG);
blocksize = 1 << params->blocklog;
params->blflag = 1;
@@ -1797,6 +1818,107 @@ parse_subopts(
}
}
+/* This lets us also add optional funky names for these */
+static int get_subopt_type(char *buffer)
+{
+ if ((strncmp("[bopts]", buffer, 7) == 0) ||
+ (strncmp("[block_size_options]", buffer, 20) == 0))
+ return 'b';
+ if ((strncmp("[dopts]", buffer, 7) == 0) ||
+ (strncmp("[data_section_options]", buffer, 22) == 0))
+ return 'd';
+ if ((strncmp("[iopts]", buffer, 7) == 0) ||
+ (strncmp("[inode_options]", buffer, 15) == 0))
+ return 'i';
+ if ((strncmp("[lopts]", buffer, 7) == 0) ||
+ (strncmp("[log_section_options]", buffer, 21) == 0))
+ return 'l';
+ if ((strncmp("[mopts]", buffer, 7) == 0) ||
+ (strncmp("[metadata_options]", buffer, 18) == 0) ||
+ (strncmp("[global_metadata_options]", buffer, 25) == 0))
+ return 'm';
+ if ((strncmp("[nopts]", buffer, 7) == 0) ||
+ (strncmp("[naming_options]", buffer, 16) == 0))
+ return 'n';
+ if ((strncmp("[ropts]", buffer, 7) == 0) ||
+ (strncmp("[realtime_section_options]", buffer, 26) == 0))
+ return 'r';
+ if ((strncmp("[sopts]", buffer, 7) == 0) ||
+ (strncmp("[sector_size]", buffer, 13) == 0))
+ return 'n';
+ return 0;
+}
+
+static int parse_config(struct mkfs_xfs_opts *params)
+{
+ FILE *fp;
+ char line[80], tag[80], value[80], *buffer = NULL;
+ char *p;
+ int type = 0;
+ int type_set = 0;
+ int ret;
+
+ fp = fopen(mkfs_config, "r");
+ if (!fp)
+ return -ENOENT;
+
+ while (!feof(fp)) {
+ p = fgets(line, sizeof(line), fp);
+ if (!p)
+ continue;
+ if (strlen(line) < 2)
+ continue;
+
+ if (line[0] == '#')
+ continue;
+
+ memset(tag, 0, sizeof(tag));
+ memset(value, 0, sizeof(value));
+
+ if (type_set) {
+ ret = sscanf(line,
+ " %[^ \t=]"
+ " = "
+ "%s ",
+ tag, value);
+ if (ret == EOF)
+ continue;
+ if (ret == 2) {
+ snprintf(line, sizeof(line), "%s=%s", tag, value);
+ buffer = line;
+ } else {
+ ret = sscanf(line, " [%[^]]s]", tag);
+ if (ret == EOF)
+ continue;
+ if (ret == 1) {
+ snprintf(line, sizeof(line), "[%s]", tag);
+ buffer = line;
+ } else
+ continue;
+ }
+ } else {
+ ret = sscanf(line, " [%[^]]s]", tag);
+ if (ret != 1 || ret == EOF)
+ continue;
+ snprintf(line, sizeof(line), "[%s]", tag);
+ buffer = line;
+ }
+
+ type = get_subopt_type(buffer);
+ if (type) {
+ type_set = type;
+ continue;
+ }
+ if (!type_set)
+ continue;
+
+ parse_subopts(type_set, p, params);
+ }
+ fclose(fp);
+
+ return 0;
+}
+
static void init_sb_feat_args_default(struct sb_feat_args *sb_feat)
{
sb_feat->finobt = 1;
@@ -1858,6 +1980,8 @@ main(
struct fs_topology ft;
struct mkfs_xfs_opts params;
struct sb_feat_args *sb_feat = ¶ms.sb_feat;
+ char *tmp_config;
+ int ret;
memset(¶ms, 0, sizeof(params));
init_sb_feat_args_default(sb_feat);
@@ -1884,8 +2008,20 @@ main(
params.xi.isdirect = LIBXFS_DIRECT;
params.xi.isreadonly = LIBXFS_EXCLUSIVELY;
- while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+ tmp_config = getenv("MKFS_XFS_CONFIG");
+ if (tmp_config != NULL)
+ mkfs_config = tmp_config;
+
+ parse_config(¶ms);
+
+ while ((c = getopt(argc, argv, "c:b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
switch (c) {
+ case 'c':
+ mkfs_config = optarg;
+ ret = parse_config(¶ms);
+ if (ret)
+ illegal(optarg, "c");
+ break;
case 'C':
case 'f':
force_overwrite = 1;
@@ -2793,6 +2929,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
if (!qflag || Nflag) {
printf(_(
+ "config-file=%-22s\n"
"meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
" =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
" =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
@@ -2802,6 +2939,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
"log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
" =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
"realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
+ mkfs_config,
dfile, params.isize, (long long)params.agcount,
(long long)params.agsize,
"", sectorsize, sb_feat->attr_version,
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support
2017-03-03 23:13 ` [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support Luis R. Rodriguez
@ 2017-03-03 23:55 ` Dave Chinner
2017-03-09 5:38 ` Eric Sandeen
1 sibling, 0 replies; 40+ messages in thread
From: Dave Chinner @ 2017-03-03 23:55 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: sandeen, linux-xfs, jack, jeffm, okurz, lpechacek
On Fri, Mar 03, 2017 at 03:13:16PM -0800, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
>
> Although mkfs.xfs already uses sensible defaults this adds a configuration
> option for parsing defaults settings for mkfs.xfs parsed prior to processing
> input arguments.
>
> User input passed to mkfs.xfs overrides defaults founds through the
> new optional configuration file, by default:
>
> /etc/mkfs.xfs.conf
>
> To use /etc/ be sure to configure xfsprogs with:
>
> ./configure --sysconfdir=/etc/
>
> The build system also allows distributions to override the default
> mkfs.xfs.conf defaults with a custom:
>
> etc/mkfs.xfs.conf.custom.in
>
> The default etc/mkfs.xfs.conf.in provides commented out examples.
> You can also override the configuration file used either with the
> MKFS_XFS_CONFIG environment variable or by using the new -c command
> line argument to mkfs.xfs. Only when -c is used will the configuration
> file be required to be present.
>
> To verify what configuration file is used on a system use the typical:
>
> mkfs.xfs -N
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
Why roll our own config file format and parsing code? Why not use
something like libconfig which is quite simple, but is much more
extensible and doesn't require us to reinvent the wheel?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support
2017-03-03 23:13 ` [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support Luis R. Rodriguez
2017-03-03 23:55 ` Dave Chinner
@ 2017-03-09 5:38 ` Eric Sandeen
1 sibling, 0 replies; 40+ messages in thread
From: Eric Sandeen @ 2017-03-09 5:38 UTC (permalink / raw)
To: Luis R. Rodriguez, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek
On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
>
> Although mkfs.xfs already uses sensible defaults this adds a configuration
> option for parsing defaults settings for mkfs.xfs parsed prior to processing
> input arguments.
So, a few other points on this, while you look at libconfig?
Thanks for the man pages & updates, but I think they can use a bit
of editing and wordsmithing for clarity; we can work on that if/when
the technical details get sorted.
> User input passed to mkfs.xfs overrides defaults founds through the
> new optional configuration file, by default:
>
> /etc/mkfs.xfs.conf
>
> To use /etc/ be sure to configure xfsprogs with:
>
> ./configure --sysconfdir=/etc/
This should just DTRT by default (today it'd go to /usr/etc by default)
(For what it's worth, xfs_quota just hard-codes "/etc" -
#define PROJID "/etc/projid"
#define PROJECT_PATHS "/etc/projects")
> The build system also allows distributions to override the default
> mkfs.xfs.conf defaults with a custom:
>
> etc/mkfs.xfs.conf.custom.in
I'm not a fan of this "drop untracked special files into $XFSPROGS/etc/
and it'll get renamed and then installed." If you want a custom
config file in etc for the distro, just use the packaging tools to put
it there, IMHO. Filtering it through the source tree && make install just
adds complexity.
> The default etc/mkfs.xfs.conf.in provides commented out examples.
> You can also override the configuration file used either with the
> MKFS_XFS_CONFIG environment variable or by using the new -c command
> line argument to mkfs.xfs. Only when -c is used will the configuration
> file be required to be present.
>
> To verify what configuration file is used on a system use the typical:
>
> mkfs.xfs -N
I'd prefer to drop the env var handling. Nothing else in xfsprogs uses
that for production cases; I don't really see a reason for it, and it
only complicates things. Require -c path if specified, use /etc/ path
if present, otherwise ignore.
Also, printing the config file path at mkfs time will probably break some
scripts that aren't expecting an extra line ...
thanks,
-Eric
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (8 preceding siblings ...)
2017-03-03 23:13 ` [PATCH 9/9] mkfs.xfs: add mkfs.xfs.conf parse support Luis R. Rodriguez
@ 2017-03-03 23:24 ` Luis R. Rodriguez
2017-03-04 3:49 ` Eric Sandeen
2017-03-09 0:16 ` Eric Sandeen
11 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-03 23:24 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: sandeen, linux-xfs, jack, jeffm, okurz, lpechacek
On Fri, Mar 03, 2017 at 03:13:07PM -0800, Luis R. Rodriguez wrote:
> This has been based on top of xfsprogs-dev v4.9.0-rc1.
Actually originally it was, I rebased onto the latest on git master which seems
to be the v4.10.0 release, these patches are based on that.
I do have patches which apply to v4.9.0 if anyone is interested they are here:
http://drvbp1.linux-foundation.org/~mcgrof/patches/2017/03/03/20170303-conf-v4.9.0-rc1/
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (9 preceding siblings ...)
2017-03-03 23:24 ` [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
@ 2017-03-04 3:49 ` Eric Sandeen
2017-03-04 4:56 ` Dave Chinner
2017-03-06 8:50 ` Jan Kara
2017-03-09 0:16 ` Eric Sandeen
11 siblings, 2 replies; 40+ messages in thread
From: Eric Sandeen @ 2017-03-04 3:49 UTC (permalink / raw)
To: Luis R. Rodriguez, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek
On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> This series adds mkfs.xfs.conf support, so that options can now be
> shoved into a configuration file. This enables certain defaults to be
> saved for folks sticking to certain values, but more importantly it
> also enables distributions to override certain defaults so that new
> filesystems remain compatible with older distributions.
>
> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>
> Given we already have an existinsg infrastructure to validate argument
> values this reuses that infrastructure by first adding helpers and porting
> over the argument parsing suppor to use these helpers.
I'm not necessarily the final word on this, but I have to say
I'm not a huge fan of having mkfs config files. I've lived
through that in mke2fs land, and my personal feeling is that
it can lead to confusion when distros start shipping config
files with different defaults than upstream ships in the
code itself.
I guess I can see the argument for shipping old/compatible
defaults with newer progs and older kernels, but by the
time a distro ships a custom old default config file they could
also patch out the new features just as easily... (which
is also confusing, I guess ;) )
After 25+ years of no external config file, I'm concerned
about principal of least surprise when the same xfsprogs version
starts behaving differently on different boxes based on a new
file that popped up in /etc ...
At the very least, I would like to /not/ ship or install any
config file with xfsprogs by default; the code itself should
be the canonical, single point of truth for defaults for a stock
"make && make install" installation.
-Eric
> Luis R. Rodriguez (9):
> mkfs.xfs: add helper to parse command line options
> mkfs.xfs: move dopts to struct mkfs_xfs_opts
> mkfs.xfs: move iopts to to struct mkfs_xfs_opts
> mkfs.xfs: move lopts to struct mkfs_xfs_opts
> mkfs.xfs: move mopts to struct mkfs_xfs_opts
> mkfs.xfs: move nopts to struct mkfs_xfs_opts
> mkfs.xfs: move ropts to struct mkfs_xfs_opts
> mkfs.xfs: use parse_subopts() to parse sopts
> mkfs.xfs: add mkfs.xfs.conf parse support
>
> .gitignore | 3 +
> Makefile | 2 +-
> etc/Makefile | 21 +
> etc/mkfs.xfs.conf.in | 58 ++
> include/builddefs.in | 2 +
> include/buildmacros | 6 +
> man/man5/mkfs.xfs.conf | 113 +++
> man/man8/mkfs.xfs.8 | 28 +
> mkfs/xfs_mkfs.c | 1788 ++++++++++++++++++++++++++----------------------
> 9 files changed, 1220 insertions(+), 801 deletions(-)
> create mode 100644 etc/Makefile
> create mode 100644 etc/mkfs.xfs.conf.in
> create mode 100644 man/man5/mkfs.xfs.conf
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-04 3:49 ` Eric Sandeen
@ 2017-03-04 4:56 ` Dave Chinner
2017-03-06 0:08 ` Eric Sandeen
2017-03-06 8:50 ` Jan Kara
1 sibling, 1 reply; 40+ messages in thread
From: Dave Chinner @ 2017-03-04 4:56 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
On Fri, Mar 03, 2017 at 09:49:58PM -0600, Eric Sandeen wrote:
> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> > This series adds mkfs.xfs.conf support, so that options can now be
> > shoved into a configuration file. This enables certain defaults to be
> > saved for folks sticking to certain values, but more importantly it
> > also enables distributions to override certain defaults so that new
> > filesystems remain compatible with older distributions.
> >
> > This has been based on top of xfsprogs-dev v4.9.0-rc1.
> >
> > Given we already have an existinsg infrastructure to validate argument
> > values this reuses that infrastructure by first adding helpers and porting
> > over the argument parsing suppor to use these helpers.
>
> I'm not necessarily the final word on this, but I have to say
> I'm not a huge fan of having mkfs config files. I've lived
> through that in mke2fs land, and my personal feeling is that
> it can lead to confusion when distros start shipping config
> files with different defaults than upstream ships in the
> code itself.
>
> I guess I can see the argument for shipping old/compatible
> defaults with newer progs and older kernels, but by the
> time a distro ships a custom old default config file they could
> also patch out the new features just as easily... (which
> is also confusing, I guess ;) )
>
> After 25+ years of no external config file, I'm concerned
> about principal of least surprise when the same xfsprogs version
> starts behaving differently on different boxes based on a new
> file that popped up in /etc ...
>
> At the very least, I would like to /not/ ship or install any
> config file with xfsprogs by default; the code itself should
> be the canonical, single point of truth for defaults for a stock
> "make && make install" installation.
Hence my suggestion of libconfig - it can /write config files/ based
on the current mkfs config. So there's no need to ship default
config files - if a user wants a special config they can use mkfs to
generate it and they can install it appropriately.
e.g. 'mkfs -W <options>' outputs a config file to stdout that
matches the config specified on the command line, but does not make
the filesystem (similar to the "-n" option). If no options are
specified, the default config is emitted....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-04 4:56 ` Dave Chinner
@ 2017-03-06 0:08 ` Eric Sandeen
2017-03-07 20:07 ` Jeff Mahoney
0 siblings, 1 reply; 40+ messages in thread
From: Eric Sandeen @ 2017-03-06 0:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
On 3/3/17 10:56 PM, Dave Chinner wrote:
> On Fri, Mar 03, 2017 at 09:49:58PM -0600, Eric Sandeen wrote:
>> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
>>> This series adds mkfs.xfs.conf support, so that options can now be
>>> shoved into a configuration file. This enables certain defaults to be
>>> saved for folks sticking to certain values, but more importantly it
>>> also enables distributions to override certain defaults so that new
>>> filesystems remain compatible with older distributions.
>>>
>>> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>>>
>>> Given we already have an existinsg infrastructure to validate argument
>>> values this reuses that infrastructure by first adding helpers and porting
>>> over the argument parsing suppor to use these helpers.
>>
>> I'm not necessarily the final word on this, but I have to say
>> I'm not a huge fan of having mkfs config files. I've lived
>> through that in mke2fs land, and my personal feeling is that
>> it can lead to confusion when distros start shipping config
>> files with different defaults than upstream ships in the
>> code itself.
>>
>> I guess I can see the argument for shipping old/compatible
>> defaults with newer progs and older kernels, but by the
>> time a distro ships a custom old default config file they could
>> also patch out the new features just as easily... (which
>> is also confusing, I guess ;) )
>>
>> After 25+ years of no external config file, I'm concerned
>> about principal of least surprise when the same xfsprogs version
>> starts behaving differently on different boxes based on a new
>> file that popped up in /etc ...
>>
>> At the very least, I would like to /not/ ship or install any
>> config file with xfsprogs by default; the code itself should
>> be the canonical, single point of truth for defaults for a stock
>> "make && make install" installation.
>
> Hence my suggestion of libconfig - it can /write config files/ based
> on the current mkfs config. So there's no need to ship default
> config files - if a user wants a special config they can use mkfs to
> generate it and they can install it appropriately.
>
> e.g. 'mkfs -W <options>' outputs a config file to stdout that
> matches the config specified on the command line, but does not make
> the filesystem (similar to the "-n" option). If no options are
> specified, the default config is emitted....
That sounds nice, but I guess I'd like to get some other thoughts
on the overall upsides and downsides of having config files which
alter a bare mkfs.xfs command's behavior.
Thanks,
-Eric
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-06 0:08 ` Eric Sandeen
@ 2017-03-07 20:07 ` Jeff Mahoney
2017-03-07 20:09 ` Eric Sandeen
0 siblings, 1 reply; 40+ messages in thread
From: Jeff Mahoney @ 2017-03-07 20:07 UTC (permalink / raw)
To: Eric Sandeen, Dave Chinner
Cc: Luis R. Rodriguez, linux-xfs, jack, okurz, lpechacek
[-- Attachment #1.1: Type: text/plain, Size: 3745 bytes --]
On 3/5/17 7:08 PM, Eric Sandeen wrote:
> On 3/3/17 10:56 PM, Dave Chinner wrote:
>> On Fri, Mar 03, 2017 at 09:49:58PM -0600, Eric Sandeen wrote:
>>> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
>>>> This series adds mkfs.xfs.conf support, so that options can now be
>>>> shoved into a configuration file. This enables certain defaults to be
>>>> saved for folks sticking to certain values, but more importantly it
>>>> also enables distributions to override certain defaults so that new
>>>> filesystems remain compatible with older distributions.
>>>>
>>>> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>>>>
>>>> Given we already have an existinsg infrastructure to validate argument
>>>> values this reuses that infrastructure by first adding helpers and porting
>>>> over the argument parsing suppor to use these helpers.
>>>
>>> I'm not necessarily the final word on this, but I have to say
>>> I'm not a huge fan of having mkfs config files. I've lived
>>> through that in mke2fs land, and my personal feeling is that
>>> it can lead to confusion when distros start shipping config
>>> files with different defaults than upstream ships in the
>>> code itself.
>>>
>>> I guess I can see the argument for shipping old/compatible
>>> defaults with newer progs and older kernels, but by the
>>> time a distro ships a custom old default config file they could
>>> also patch out the new features just as easily... (which
>>> is also confusing, I guess ;) )
>>>
>>> After 25+ years of no external config file, I'm concerned
>>> about principal of least surprise when the same xfsprogs version
>>> starts behaving differently on different boxes based on a new
>>> file that popped up in /etc ...
>>>
>>> At the very least, I would like to /not/ ship or install any
>>> config file with xfsprogs by default; the code itself should
>>> be the canonical, single point of truth for defaults for a stock
>>> "make && make install" installation.
>>
>> Hence my suggestion of libconfig - it can /write config files/ based
>> on the current mkfs config. So there's no need to ship default
>> config files - if a user wants a special config they can use mkfs to
>> generate it and they can install it appropriately.
>>
>> e.g. 'mkfs -W <options>' outputs a config file to stdout that
>> matches the config specified on the command line, but does not make
>> the filesystem (similar to the "-n" option). If no options are
>> specified, the default config is emitted....
>
> That sounds nice, but I guess I'd like to get some other thoughts
> on the overall upsides and downsides of having config files which
> alter a bare mkfs.xfs command's behavior.
The biggest upside is that we can package up the latest xfsprogs on any
distro release and have it work for the user without any surprises. If
we do that now, we end up with a mkfs that creates file systems that the
kernel can't actually use. The most obvious example is using a modern
mkfs.xfs on a system that doesn't support v5 superblocks. There's
actually nothing in the man page that will tell a user how to interpret
the error message and create a usable file system. Defaults changing
over time is good practice, but having a way to keep the old defaults on
older systems is useful. Being able to dump the defaults to a file is
helpful so that we can check whether the defaults have changed
programmatically without having to know about new options ahead of time.
That said, by default, we shouldn't install a config file from 'make
install.' If distros want to add one, they're welcome to do that either
in their xfsprogs package or via some per-release mechanism.
-Jeff
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-07 20:07 ` Jeff Mahoney
@ 2017-03-07 20:09 ` Eric Sandeen
0 siblings, 0 replies; 40+ messages in thread
From: Eric Sandeen @ 2017-03-07 20:09 UTC (permalink / raw)
To: Jeff Mahoney, Dave Chinner
Cc: Luis R. Rodriguez, linux-xfs, jack, okurz, lpechacek
[-- Attachment #1.1: Type: text/plain, Size: 3910 bytes --]
On 3/7/17 2:07 PM, Jeff Mahoney wrote:
> On 3/5/17 7:08 PM, Eric Sandeen wrote:
>> On 3/3/17 10:56 PM, Dave Chinner wrote:
>>> On Fri, Mar 03, 2017 at 09:49:58PM -0600, Eric Sandeen wrote:
>>>> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
>>>>> This series adds mkfs.xfs.conf support, so that options can now be
>>>>> shoved into a configuration file. This enables certain defaults to be
>>>>> saved for folks sticking to certain values, but more importantly it
>>>>> also enables distributions to override certain defaults so that new
>>>>> filesystems remain compatible with older distributions.
>>>>>
>>>>> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>>>>>
>>>>> Given we already have an existinsg infrastructure to validate argument
>>>>> values this reuses that infrastructure by first adding helpers and porting
>>>>> over the argument parsing suppor to use these helpers.
>>>>
>>>> I'm not necessarily the final word on this, but I have to say
>>>> I'm not a huge fan of having mkfs config files. I've lived
>>>> through that in mke2fs land, and my personal feeling is that
>>>> it can lead to confusion when distros start shipping config
>>>> files with different defaults than upstream ships in the
>>>> code itself.
>>>>
>>>> I guess I can see the argument for shipping old/compatible
>>>> defaults with newer progs and older kernels, but by the
>>>> time a distro ships a custom old default config file they could
>>>> also patch out the new features just as easily... (which
>>>> is also confusing, I guess ;) )
>>>>
>>>> After 25+ years of no external config file, I'm concerned
>>>> about principal of least surprise when the same xfsprogs version
>>>> starts behaving differently on different boxes based on a new
>>>> file that popped up in /etc ...
>>>>
>>>> At the very least, I would like to /not/ ship or install any
>>>> config file with xfsprogs by default; the code itself should
>>>> be the canonical, single point of truth for defaults for a stock
>>>> "make && make install" installation.
>>>
>>> Hence my suggestion of libconfig - it can /write config files/ based
>>> on the current mkfs config. So there's no need to ship default
>>> config files - if a user wants a special config they can use mkfs to
>>> generate it and they can install it appropriately.
>>>
>>> e.g. 'mkfs -W <options>' outputs a config file to stdout that
>>> matches the config specified on the command line, but does not make
>>> the filesystem (similar to the "-n" option). If no options are
>>> specified, the default config is emitted....
>>
>> That sounds nice, but I guess I'd like to get some other thoughts
>> on the overall upsides and downsides of having config files which
>> alter a bare mkfs.xfs command's behavior.
>
> The biggest upside is that we can package up the latest xfsprogs on any
> distro release and have it work for the user without any surprises. If
> we do that now, we end up with a mkfs that creates file systems that the
> kernel can't actually use. The most obvious example is using a modern
> mkfs.xfs on a system that doesn't support v5 superblocks. There's
> actually nothing in the man page that will tell a user how to interpret
> the error message and create a usable file system. Defaults changing
> over time is good practice, but having a way to keep the old defaults on
> older systems is useful. Being able to dump the defaults to a file is
> helpful so that we can check whether the defaults have changed
> programmatically without having to know about new options ahead of time.
Ok, I'm getting convinced. Thanks for convincing. :)
-Eric
> That said, by default, we shouldn't install a config file from 'make
> install.' If distros want to add one, they're welcome to do that either
> in their xfsprogs package or via some per-release mechanism.
>
> -Jeff
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 867 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-04 3:49 ` Eric Sandeen
2017-03-04 4:56 ` Dave Chinner
@ 2017-03-06 8:50 ` Jan Kara
1 sibling, 0 replies; 40+ messages in thread
From: Jan Kara @ 2017-03-06 8:50 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
On Fri 03-03-17 21:49:58, Eric Sandeen wrote:
> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> > This series adds mkfs.xfs.conf support, so that options can now be
> > shoved into a configuration file. This enables certain defaults to be
> > saved for folks sticking to certain values, but more importantly it
> > also enables distributions to override certain defaults so that new
> > filesystems remain compatible with older distributions.
> >
> > This has been based on top of xfsprogs-dev v4.9.0-rc1.
> >
> > Given we already have an existinsg infrastructure to validate argument
> > values this reuses that infrastructure by first adding helpers and porting
> > over the argument parsing suppor to use these helpers.
>
> I'm not necessarily the final word on this, but I have to say
> I'm not a huge fan of having mkfs config files. I've lived
> through that in mke2fs land, and my personal feeling is that
> it can lead to confusion when distros start shipping config
> files with different defaults than upstream ships in the
> code itself.
>
> I guess I can see the argument for shipping old/compatible
> defaults with newer progs and older kernels, but by the
> time a distro ships a custom old default config file they could
> also patch out the new features just as easily... (which
> is also confusing, I guess ;) )
Yeah, patching sources is IMO more confusing than changing config files.
You can at least easily ask for the contents of the config file... Also
when analysing XFS issue, you ask for xfs_info output anyway as that is the
only way how to be *sure* which features the fs has enabled and mkfs config
file does not invalidate that.
> After 25+ years of no external config file, I'm concerned
> about principal of least surprise when the same xfsprogs version
> starts behaving differently on different boxes based on a new
> file that popped up in /etc ...
>
> At the very least, I would like to /not/ ship or install any
> config file with xfsprogs by default; the code itself should
> be the canonical, single point of truth for defaults for a stock
> "make && make install" installation.
Agree with that.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-03 23:13 [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support Luis R. Rodriguez
` (10 preceding siblings ...)
2017-03-04 3:49 ` Eric Sandeen
@ 2017-03-09 0:16 ` Eric Sandeen
2017-03-09 0:51 ` Luis R. Rodriguez
11 siblings, 1 reply; 40+ messages in thread
From: Eric Sandeen @ 2017-03-09 0:16 UTC (permalink / raw)
To: Luis R. Rodriguez, linux-xfs; +Cc: jack, jeffm, okurz, lpechacek
On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> This series adds mkfs.xfs.conf support, so that options can now be
> shoved into a configuration file. This enables certain defaults to be
> saved for folks sticking to certain values, but more importantly it
> also enables distributions to override certain defaults so that new
> filesystems remain compatible with older distributions.
>
> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>
> Given we already have an existinsg infrastructure to validate argument
> values this reuses that infrastructure by first adding helpers and porting
> over the argument parsing suppor to use these helpers.
Hm, one functional problem with this, aside from Dave's concerns and
suggestions, is that many options in the config file can't actually
be overridden on the commandline because they are treated as having
been respecified, which is not allowed:
# mkfs.xfs -m crc=1 -m crc=0 -f fsfile
-m crc option respecified
Usage: mkfs.xfs
...
and:
# grep -B1 crc /etc/mkfs.xfs.conf
[metadata_options]
crc = 1
# mkfs/mkfs.xfs -c /etc/mkfs.xfs.conf -m crc=0 -f fsfile
-m crc option respecified
Usage: mkfs.xfs
...
-Eric
> Luis R. Rodriguez (9):
> mkfs.xfs: add helper to parse command line options
> mkfs.xfs: move dopts to struct mkfs_xfs_opts
> mkfs.xfs: move iopts to to struct mkfs_xfs_opts
> mkfs.xfs: move lopts to struct mkfs_xfs_opts
> mkfs.xfs: move mopts to struct mkfs_xfs_opts
> mkfs.xfs: move nopts to struct mkfs_xfs_opts
> mkfs.xfs: move ropts to struct mkfs_xfs_opts
> mkfs.xfs: use parse_subopts() to parse sopts
> mkfs.xfs: add mkfs.xfs.conf parse support
>
> .gitignore | 3 +
> Makefile | 2 +-
> etc/Makefile | 21 +
> etc/mkfs.xfs.conf.in | 58 ++
> include/builddefs.in | 2 +
> include/buildmacros | 6 +
> man/man5/mkfs.xfs.conf | 113 +++
> man/man8/mkfs.xfs.8 | 28 +
> mkfs/xfs_mkfs.c | 1788 ++++++++++++++++++++++++++----------------------
> 9 files changed, 1220 insertions(+), 801 deletions(-)
> create mode 100644 etc/Makefile
> create mode 100644 etc/mkfs.xfs.conf.in
> create mode 100644 man/man5/mkfs.xfs.conf
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 0:16 ` Eric Sandeen
@ 2017-03-09 0:51 ` Luis R. Rodriguez
2017-03-09 4:41 ` Eric Sandeen
0 siblings, 1 reply; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-09 0:51 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
On Wed, Mar 08, 2017 at 06:16:57PM -0600, Eric Sandeen wrote:
> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> > This series adds mkfs.xfs.conf support, so that options can now be
> > shoved into a configuration file. This enables certain defaults to be
> > saved for folks sticking to certain values, but more importantly it
> > also enables distributions to override certain defaults so that new
> > filesystems remain compatible with older distributions.
> >
> > This has been based on top of xfsprogs-dev v4.9.0-rc1.
> >
> > Given we already have an existinsg infrastructure to validate argument
> > values this reuses that infrastructure by first adding helpers and porting
> > over the argument parsing suppor to use these helpers.
>
> Hm, one functional problem with this, aside from Dave's concerns and
> suggestions, is that many options in the config file can't actually
> be overridden on the commandline because they are treated as having
> been respecified, which is not allowed:
>
> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile
> -m crc option respecified
> Usage: mkfs.xfs
This was dealt by enabling the last option taken to override, and
this mechanism was also taken to enable the config file to take
the first value but let the command line to override. Refer to
usage of reset_opt(). Granted I had only done this on B_LOG but
this can easily be made to enable us to reset for all options.
If we don't want to enable subsequent command line entries to
override (to keep old behaviour) but still allow at least the
command line to override the config file options, that's also
doable.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 0:51 ` Luis R. Rodriguez
@ 2017-03-09 4:41 ` Eric Sandeen
2017-03-09 10:12 ` Jan Tulak
2017-03-09 17:57 ` Luis R. Rodriguez
0 siblings, 2 replies; 40+ messages in thread
From: Eric Sandeen @ 2017-03-09 4:41 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs, jack, jeffm, okurz, lpechacek, Jan Tulak
On 3/8/17 6:51 PM, Luis R. Rodriguez wrote:
> On Wed, Mar 08, 2017 at 06:16:57PM -0600, Eric Sandeen wrote:
>> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
>>> This series adds mkfs.xfs.conf support, so that options can now be
>>> shoved into a configuration file. This enables certain defaults to be
>>> saved for folks sticking to certain values, but more importantly it
>>> also enables distributions to override certain defaults so that new
>>> filesystems remain compatible with older distributions.
>>>
>>> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>>>
>>> Given we already have an existinsg infrastructure to validate argument
>>> values this reuses that infrastructure by first adding helpers and porting
>>> over the argument parsing suppor to use these helpers.
>>
>> Hm, one functional problem with this, aside from Dave's concerns and
>> suggestions, is that many options in the config file can't actually
>> be overridden on the commandline because they are treated as having
>> been respecified, which is not allowed:
>>
>> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile
>> -m crc option respecified
>> Usage: mkfs.xfs
>
> This was dealt by enabling the last option taken to override, and
> this mechanism was also taken to enable the config file to take
> the first value but let the command line to override. Refer to
> usage of reset_opt(). Granted I had only done this on B_LOG but
> this can easily be made to enable us to reset for all options.
Well, the above test was with your full patchset applied,
so my point is that it's not currently working properly as posted...
But are you proposing adding this reset_opt() to /every/ option?
That would undo all of the respecification checks, which were
put there for a reason (I assume?) ;) I don't really remember
how all of the respecification and compatibility checking works
tbh, I'd have to dig back into it. Maybe jtulak can help...
But it makes little sense to have a framework to prevent
respecification but then render it useless with reset_opt()
after each option gets parsed. Or do I misunderstand?
> If we don't want to enable subsequent command line entries to
> override (to keep old behaviour) but still allow at least the
> command line to override the config file options, that's also
> doable.
Well, I'm going to need to refamiliarize myself with how the
conflict checking works, and why respecification is prohibited.
If respecification matters, it matters just as much whether the
first specification came from the config file or from the command
line.
-Eric
> Luis
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 4:41 ` Eric Sandeen
@ 2017-03-09 10:12 ` Jan Tulak
2017-03-09 14:31 ` Eric Sandeen
2017-03-09 17:57 ` Luis R. Rodriguez
1 sibling, 1 reply; 40+ messages in thread
From: Jan Tulak @ 2017-03-09 10:12 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
Hi,
Eric, thanks for pinging me, I wanted to look at this and forgot... :-/
First of all, this patchset has a lot of common with my advanced
conflict checking patchset. And AFAIK, if my patchset was already in
place, this set could be reduced to one new function that loads the
file and changes the default values in a structure.
See this http://www.spinics.net/lists/linux-xfs/msg02728.html
I wanted to fix the few mentioned issues I got for that RFC and submit
it this week, but with the flu I got, I'm doing things that don't
cause me a headache when I spread my mind over too many LOC at once.
:-) And now, looking at this, I would like to go through this patchset
in more detail and see if you (Luis) got some things better than me
before I send it, so it will take some more time.
Some other info is bellow between quotes.
On Thu, Mar 9, 2017 at 5:41 AM, Eric Sandeen <sandeen@sandeen.net> wrote:
>
> On 3/8/17 6:51 PM, Luis R. Rodriguez wrote:
>> On Wed, Mar 08, 2017 at 06:16:57PM -0600, Eric Sandeen wrote:
>>> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
>>>> This series adds mkfs.xfs.conf support, so that options can now be
>>>> shoved into a configuration file. This enables certain defaults to be
>>>> saved for folks sticking to certain values, but more importantly it
>>>> also enables distributions to override certain defaults so that new
>>>> filesystems remain compatible with older distributions.
>>>>
>>>> This has been based on top of xfsprogs-dev v4.9.0-rc1.
>>>>
>>>> Given we already have an existinsg infrastructure to validate argument
>>>> values this reuses that infrastructure by first adding helpers and porting
>>>> over the argument parsing suppor to use these helpers.
>>>
>>> Hm, one functional problem with this, aside from Dave's concerns and
>>> suggestions, is that many options in the config file can't actually
>>> be overridden on the commandline because they are treated as having
>>> been respecified, which is not allowed:
>>>
>>> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile
>>> -m crc option respecified
>>> Usage: mkfs.xfs
>>
>> This was dealt by enabling the last option taken to override, and
>> this mechanism was also taken to enable the config file to take
>> the first value but let the command line to override. Refer to
>> usage of reset_opt(). Granted I had only done this on B_LOG but
>> this can easily be made to enable us to reset for all options.
>
> Well, the above test was with your full patchset applied,
> so my point is that it's not currently working properly as posted...
>
> But are you proposing adding this reset_opt() to /every/ option?
> That would undo all of the respecification checks, which were
> put there for a reason (I assume?) ;) I don't really remember
> how all of the respecification and compatibility checking works
> tbh, I'd have to dig back into it. Maybe jtulak can help...
>
> But it makes little sense to have a framework to prevent
> respecification but then render it useless with reset_opt()
> after each option gets parsed. Or do I misunderstand?
The reason why respec is forbidden (other than "what the hell the user
wants, or is this a typo?" which could be decided as "simply take the
last one, it's a feature") is that some options depend on other ones
in "must be specified first" way. What should we do if we get -b
size=X -d size=1000b -b size=Y? Values are parsed at the time of their
occurrence, so suddenly, we have size in blocks but it is not matching
the blocksize that would get written to the device...
So we need to keep the respec limit in place.
>
>> If we don't want to enable subsequent command line entries to
>> override (to keep old behaviour) but still allow at least the
>> command line to override the config file options, that's also
>> doable.
>
> Well, I'm going to need to refamiliarize myself with how the
> conflict checking works, and why respecification is prohibited.
> If respecification matters, it matters just as much whether the
> first specification came from the config file or from the command
> line.
>
Regarding the last sentence:
On a general level, not really, I think. Change from the config file
can be seen as a change of defaults. It doesn't matter whether these
values were put in during compilation or loaded from a config file.
Multiple occurrences of the same option on the CLI is the only trouble
here because it is at this time when further values are computed and
it can go wrong. I didn't read these patches careful enough to answer
whether it matters in this specific case, though.
Saying this, putting -d size in blocks into config file maybe could
still cause trouble, but this can be solved by limiting what values or
options can be in the config file. For example: "all sizes has to be
in bytes". And I think that building on my patchset, it should be
possible with a reasonable amount of work to get a kind of lazy
evaluation. So this issue would disappear because the "1000b" value
would remain a string until all entries from CLI are parsed.
Cheers,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 10:12 ` Jan Tulak
@ 2017-03-09 14:31 ` Eric Sandeen
2017-03-09 15:21 ` Jan Tulak
0 siblings, 1 reply; 40+ messages in thread
From: Eric Sandeen @ 2017-03-09 14:31 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
On 3/9/17 4:12 AM, Jan Tulak wrote:
> Hi,
>
> Eric, thanks for pinging me, I wanted to look at this and forgot... :-/
>
> First of all, this patchset has a lot of common with my advanced
> conflict checking patchset. And AFAIK, if my patchset was already in
> place, this set could be reduced to one new function that loads the
> file and changes the default values in a structure.
>
> See this http://www.spinics.net/lists/linux-xfs/msg02728.html
Still on the todo :( ;)
> I wanted to fix the few mentioned issues I got for that RFC and submit
> it this week, but with the flu I got, I'm doing things that don't
> cause me a headache when I spread my mind over too many LOC at once.
> :-) And now, looking at this, I would like to go through this patchset
> in more detail and see if you (Luis) got some things better than me
> before I send it, so it will take some more time.
>
> Some other info is bellow between quotes.
...
>>>> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile
>>>> -m crc option respecified
>>>> Usage: mkfs.xfs
>>>
>>> This was dealt by enabling the last option taken to override, and
>>> this mechanism was also taken to enable the config file to take
>>> the first value but let the command line to override. Refer to
>>> usage of reset_opt(). Granted I had only done this on B_LOG but
>>> this can easily be made to enable us to reset for all options.
>>
>> Well, the above test was with your full patchset applied,
>> so my point is that it's not currently working properly as posted...
>>
>> But are you proposing adding this reset_opt() to /every/ option?
>> That would undo all of the respecification checks, which were
>> put there for a reason (I assume?) ;) I don't really remember
>> how all of the respecification and compatibility checking works
>> tbh, I'd have to dig back into it. Maybe jtulak can help...
>>
>> But it makes little sense to have a framework to prevent
>> respecification but then render it useless with reset_opt()
>> after each option gets parsed. Or do I misunderstand?
>
> The reason why respec is forbidden (other than "what the hell the user
> wants, or is this a typo?" which could be decided as "simply take the
> last one, it's a feature") is that some options depend on other ones
> in "must be specified first" way. What should we do if we get -b
> size=X -d size=1000b -b size=Y? Values are parsed at the time of their
> occurrence, so suddenly, we have size in blocks but it is not matching
> the blocksize that would get written to the device...
>
> So we need to keep the respec limit in place.
Let me ask you this, is the respec prohibition only for functional
dependencies (i.e. sizes which depend on previously-specified blocksize)
or is it also used for conflict checking (i.e. reflink requires crc?)
>>
>>> If we don't want to enable subsequent command line entries to
>>> override (to keep old behaviour) but still allow at least the
>>> command line to override the config file options, that's also
>>> doable.
>>
>> Well, I'm going to need to refamiliarize myself with how the
>> conflict checking works, and why respecification is prohibited.
>> If respecification matters, it matters just as much whether the
>> first specification came from the config file or from the command
>> line.
>>
>
> Regarding the last sentence:
>
> On a general level, not really, I think. Change from the config file
> can be seen as a change of defaults. It doesn't matter whether these
> values were put in during compilation or loaded from a config file.
Actually, I might disagree. Defaults during compilation are controlled
and tested by upstream developers, and are guaranteed (?) to be consistent
and correct. Defaults from a config file are subject to the whims
of the config file editor, and may not be consistent at all, and require
just as much checking as something from the commandline. Isn't
this an important difference?
> Multiple occurrences of the same option on the CLI is the only trouble
> here because it is at this time when further values are computed and
> it can go wrong. I didn't read these patches careful enough to answer
> whether it matters in this specific case, though.
Well, at this point "this specific case" is, I think, "any option can
be specified in the config file, and will be parsed exactly like any valid
command line option would be parsed."
> Saying this, putting -d size in blocks into config file maybe could
> still cause trouble, but this can be solved by limiting what values or
> options can be in the config file. For example: "all sizes has to be
> in bytes". And I think that building on my patchset, it should be
> possible with a reasonable amount of work to get a kind of lazy
> evaluation. So this issue would disappear because the "1000b" value
> would remain a string until all entries from CLI are parsed.
So this would be sort of a "collect all options, respecified options
means last one wins, and all evaluation is done at the end of parsing"
sort of routine?
-Eric
> Cheers,
> Jan
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 14:31 ` Eric Sandeen
@ 2017-03-09 15:21 ` Jan Tulak
0 siblings, 0 replies; 40+ messages in thread
From: Jan Tulak @ 2017-03-09 15:21 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek
On Thu, Mar 9, 2017 at 3:31 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 3/9/17 4:12 AM, Jan Tulak wrote:
>> Hi,
>>
>> Eric, thanks for pinging me, I wanted to look at this and forgot... :-/
>>
>> First of all, this patchset has a lot of common with my advanced
>> conflict checking patchset. And AFAIK, if my patchset was already in
>> place, this set could be reduced to one new function that loads the
>> file and changes the default values in a structure.
>>
>> See this http://www.spinics.net/lists/linux-xfs/msg02728.html
>
> Still on the todo :( ;)
>
>> I wanted to fix the few mentioned issues I got for that RFC and submit
>> it this week, but with the flu I got, I'm doing things that don't
>> cause me a headache when I spread my mind over too many LOC at once.
>> :-) And now, looking at this, I would like to go through this patchset
>> in more detail and see if you (Luis) got some things better than me
>> before I send it, so it will take some more time.
>>
>> Some other info is bellow between quotes.
>
> ...
>
>>>>> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile
>>>>> -m crc option respecified
>>>>> Usage: mkfs.xfs
>>>>
>>>> This was dealt by enabling the last option taken to override, and
>>>> this mechanism was also taken to enable the config file to take
>>>> the first value but let the command line to override. Refer to
>>>> usage of reset_opt(). Granted I had only done this on B_LOG but
>>>> this can easily be made to enable us to reset for all options.
>>>
>>> Well, the above test was with your full patchset applied,
>>> so my point is that it's not currently working properly as posted...
>>>
>>> But are you proposing adding this reset_opt() to /every/ option?
>>> That would undo all of the respecification checks, which were
>>> put there for a reason (I assume?) ;) I don't really remember
>>> how all of the respecification and compatibility checking works
>>> tbh, I'd have to dig back into it. Maybe jtulak can help...
>>>
>>> But it makes little sense to have a framework to prevent
>>> respecification but then render it useless with reset_opt()
>>> after each option gets parsed. Or do I misunderstand?
>>
>> The reason why respec is forbidden (other than "what the hell the user
>> wants, or is this a typo?" which could be decided as "simply take the
>> last one, it's a feature") is that some options depend on other ones
>> in "must be specified first" way. What should we do if we get -b
>> size=X -d size=1000b -b size=Y? Values are parsed at the time of their
>> occurrence, so suddenly, we have size in blocks but it is not matching
>> the blocksize that would get written to the device...
>>
>> So we need to keep the respec limit in place.
>
> Let me ask you this, is the respec prohibition only for functional
> dependencies (i.e. sizes which depend on previously-specified blocksize)
> or is it also used for conflict checking (i.e. reflink requires crc?)
>
Only functional dependencies, AFAIK. These conflict checks shouldn't
be an issue. In the current code, they are happening without much
logic (wherever someone put them) after all CLI options were parsed.
In my patchset, they are more organised but still are done after all
the values are parsed. Exceptions in the current code can happen,
though these are not a blocker - I think that some options do conflict
checks in the CLI loop that could wait for later and my patchset moves
them.
>>>
>>>> If we don't want to enable subsequent command line entries to
>>>> override (to keep old behaviour) but still allow at least the
>>>> command line to override the config file options, that's also
>>>> doable.
>>>
>>> Well, I'm going to need to refamiliarize myself with how the
>>> conflict checking works, and why respecification is prohibited.
>>> If respecification matters, it matters just as much whether the
>>> first specification came from the config file or from the command
>>> line.
>>>
>>
>> Regarding the last sentence:
>>
>> On a general level, not really, I think. Change from the config file
>> can be seen as a change of defaults. It doesn't matter whether these
>> values were put in during compilation or loaded from a config file.
>
> Actually, I might disagree. Defaults during compilation are controlled
> and tested by upstream developers, and are guaranteed (?) to be consistent
> and correct. Defaults from a config file are subject to the whims
> of the config file editor, and may not be consistent at all, and require
> just as much checking as something from the commandline. Isn't
> this an important difference?
True, I didn't realise this. But still, with my patchset, you can use
the CLI-designated functions, load it into the struct, then run a loop
that will set all .seen = false and voila, you have fully validated
the config file and you are ready to run another round for CLI as if
the values were defaults. So you don't break the respec as it will be
evaluated independently both for the config and CLI.
>
>> Multiple occurrences of the same option on the CLI is the only trouble
>> here because it is at this time when further values are computed and
>> it can go wrong. I didn't read these patches careful enough to answer
>> whether it matters in this specific case, though.
>
> Well, at this point "this specific case" is, I think, "any option can
> be specified in the config file, and will be parsed exactly like any valid
> command line option would be parsed."
I speak about how exactly the code handles it.
>
>> Saying this, putting -d size in blocks into config file maybe could
>> still cause trouble, but this can be solved by limiting what values or
>> options can be in the config file. For example: "all sizes has to be
>> in bytes". And I think that building on my patchset, it should be
>> possible with a reasonable amount of work to get a kind of lazy
>> evaluation. So this issue would disappear because the "1000b" value
>> would remain a string until all entries from CLI are parsed.
>
> So this would be sort of a "collect all options, respecified options
> means last one wins, and all evaluation is done at the end of parsing"
> sort of routine?
Yes, exactly. I can't think a case where this would cause a trouble.
Jan
>
> -Eric
>
>> Cheers,
>> Jan
>>
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 4:41 ` Eric Sandeen
2017-03-09 10:12 ` Jan Tulak
@ 2017-03-09 17:57 ` Luis R. Rodriguez
2017-03-09 22:34 ` Dave Chinner
1 sibling, 1 reply; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-03-09 17:57 UTC (permalink / raw)
To: Eric Sandeen
Cc: Luis R. Rodriguez, linux-xfs, jack, jeffm, okurz, lpechacek, Jan Tulak
On Wed, Mar 08, 2017 at 10:41:52PM -0600, Eric Sandeen wrote:
>
> On 3/8/17 6:51 PM, Luis R. Rodriguez wrote:
> > On Wed, Mar 08, 2017 at 06:16:57PM -0600, Eric Sandeen wrote:
> >> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote:
> >>> This series adds mkfs.xfs.conf support, so that options can now be
> >>> shoved into a configuration file. This enables certain defaults to be
> >>> saved for folks sticking to certain values, but more importantly it
> >>> also enables distributions to override certain defaults so that new
> >>> filesystems remain compatible with older distributions.
> >>>
> >>> This has been based on top of xfsprogs-dev v4.9.0-rc1.
> >>>
> >>> Given we already have an existinsg infrastructure to validate argument
> >>> values this reuses that infrastructure by first adding helpers and porting
> >>> over the argument parsing suppor to use these helpers.
> >>
> >> Hm, one functional problem with this, aside from Dave's concerns and
> >> suggestions, is that many options in the config file can't actually
> >> be overridden on the commandline because they are treated as having
> >> been respecified, which is not allowed:
> >>
> >> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile
> >> -m crc option respecified
> >> Usage: mkfs.xfs
> >
> > This was dealt by enabling the last option taken to override, and
> > this mechanism was also taken to enable the config file to take
> > the first value but let the command line to override. Refer to
> > usage of reset_opt(). Granted I had only done this on B_LOG but
> > this can easily be made to enable us to reset for all options.
>
> Well, the above test was with your full patchset applied,
> so my point is that it's not currently working properly as posted...
Sorry yes I failed to reset_opt() for each config entry on the config
file. I had meant to move reset_opt() generically on parse_subopts()
so that "last entry specified wins" always. I had only added this
for B_LOG.
> But are you proposing adding this reset_opt() to /every/ option?
> That would undo all of the respecification checks, which were
> put there for a reason (I assume?) ;) I don't really remember
> how all of the respecification and compatibility checking works
> tbh, I'd have to dig back into it. Maybe jtulak can help...
>
> But it makes little sense to have a framework to prevent
> respecification but then render it useless with reset_opt()
> after each option gets parsed. Or do I misunderstand?
I used reset_opt() and went with "last entry specified wins". From my
review the goal of the respecification was to ensure each opt param
parsed would not reset a prior set param, a paranoid measure, however
this clearly does not work well if we want to allow for "last entry
specified wins", or re-use the validators for a config file parsing
for a first shot a parsing entries.
*Proper validation* is and should be done *after* all parameters are
set, and it sounds like jtulak's work helps shove this into helpers
*as should be done* to clear cruft out of main(). Having these params
in structs should help both efforts.
> > If we don't want to enable subsequent command line entries to
> > override (to keep old behaviour) but still allow at least the
> > command line to override the config file options, that's also
> > doable.
>
> Well, I'm going to need to refamiliarize myself with how the
> conflict checking works, and why respecification is prohibited.
> If respecification matters, it matters just as much whether the
> first specification came from the config file or from the command
> line.
Agreed. The above is what I concluded based on my inspection of the
code. It would be of course great to get more eyeballs on it.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 17:57 ` Luis R. Rodriguez
@ 2017-03-09 22:34 ` Dave Chinner
2017-04-24 5:00 ` Luis R. Rodriguez
0 siblings, 1 reply; 40+ messages in thread
From: Dave Chinner @ 2017-03-09 22:34 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Eric Sandeen, linux-xfs, jack, jeffm, okurz, lpechacek, Jan Tulak
On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
> On Wed, Mar 08, 2017 at 10:41:52PM -0600, Eric Sandeen wrote:
> > But are you proposing adding this reset_opt() to /every/ option?
> > That would undo all of the respecification checks, which were
> > put there for a reason (I assume?) ;) I don't really remember
> > how all of the respecification and compatibility checking works
> > tbh, I'd have to dig back into it. Maybe jtulak can help...
> >
> > But it makes little sense to have a framework to prevent
> > respecification but then render it useless with reset_opt()
> > after each option gets parsed. Or do I misunderstand?
>
> I used reset_opt() and went with "last entry specified wins". From my
> review the goal of the respecification was to ensure each opt param
> parsed would not reset a prior set param, a paranoid measure, however
> this clearly does not work well if we want to allow for "last entry
> specified wins", or re-use the validators for a config file parsing
> for a first shot a parsing entries.
Which is essentially broken, because doing something like:
-m crc =1 -m reflink=1 -m crc=0
leaves you with an /invalid config/ because of the respecification
of -m crc=0 and the order in which options are parsed and verified.
Indeed, things like block and sector sizes are particularly nasty in
this respect, because other options can be specified in block or
sector units. SO things like:
-s size=4k -b size=1s -s size=512 -d size=1000000s
were considered valid. respecification of options like this is just
borken, and even if we take "last specification wins" it still means
that the block size specification is ambiguous and potentially
incorrect depending on other options. Hence respecification of
options is simply not allowed and post-processing of the options
doesn't change that.
i.e., the biggest issue with reusing the existing parsing code for
the "default config" is that is doesn't just set default values - it
prevents other options from being used. IOWs, the config file should
set the default values in the option table, not set the options
directly as happens on the command line....
> *Proper validation* is and should be done *after* all parameters are
> set, and it sounds like jtulak's work helps shove this into helpers
> *as should be done* to clear cruft out of main(). Having these params
> in structs should help both efforts.
Right, that was the whole point of the table based option parsing I
started on and Jan has been continuing - factoring the option
parsing code and getting rid of all the whacky variables used to say
"this option is set" and move all the config state into the option
table. Then all the whacky variables can be replaced with option
table lookups, and so if the option has not been set on the command
line, the default value can be picked up from the table (which was
set from the config file).
Only the first step (table based parsing) has been done so
far - there's still plenty of work needed to get the option table
and code to the point where we use it exclusively for config
lookups. Once we get there, then using a config file for default
values is easy to add...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-03-09 22:34 ` Dave Chinner
@ 2017-04-24 5:00 ` Luis R. Rodriguez
2017-04-24 7:26 ` Jan Tulak
0 siblings, 1 reply; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-04-24 5:00 UTC (permalink / raw)
To: Dave Chinner, Jan Tulak
Cc: Luis R. Rodriguez, Eric Sandeen, linux-xfs, jack, jeffm, okurz,
lpechacek
On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
> > I used reset_opt() and went with "last entry specified wins". From my
> > review the goal of the respecification was to ensure each opt param
> > parsed would not reset a prior set param, a paranoid measure, however
> > this clearly does not work well if we want to allow for "last entry
> > specified wins", or re-use the validators for a config file parsing
> > for a first shot a parsing entries.
>
> Which is essentially broken, because doing something like:
>
> -m crc =1 -m reflink=1 -m crc=0
>
> leaves you with an /invalid config/ because of the respecification
> of -m crc=0 and the order in which options are parsed and verified.
>
> Indeed, things like block and sector sizes are particularly nasty in
> this respect, because other options can be specified in block or
> sector units. SO things like:
>
> -s size=4k -b size=1s -s size=512 -d size=1000000s
>
> were considered valid. respecification of options like this is just
> borken, and even if we take "last specification wins" it still means
> that the block size specification is ambiguous and potentially
> incorrect depending on other options. Hence respecification of
> options is simply not allowed and post-processing of the options
> doesn't change that.
We have to pick an approach and stick with, the above seems sensible.
> i.e., the biggest issue with reusing the existing parsing code for
> the "default config" is that is doesn't just set default values - it
> prevents other options from being used.
Right as per original design.
> IOWs, the config file should
> set the default values in the option table, not set the options
> directly as happens on the command line....
As I respin my patches addressing concerns an issue I see with this is current
semantics for "defaultval" is not that they will be the defaults, but rather
they will be the defaults *iff* the user did specify the option on the command
line but did not provide an explicit value. This for example would not allow
distributions to disable a feature which perhaps is enabled by default in
future versions xfsprogs, or enable one which perhaps is by default disabled.
Even if the config file would have values specified, they will be ignored
unless the user passed a user input value with no value for it.
If this is fine by everyone then great.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-04-24 5:00 ` Luis R. Rodriguez
@ 2017-04-24 7:26 ` Jan Tulak
2017-04-24 8:25 ` Luis R. Rodriguez
0 siblings, 1 reply; 40+ messages in thread
From: Jan Tulak @ 2017-04-24 7:26 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Dave Chinner, Eric Sandeen, linux-xfs, jack, Jeff Mahoney, okurz,
lpechacek
On Mon, Apr 24, 2017 at 7:00 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
>> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
>> > I used reset_opt() and went with "last entry specified wins". From my
>> > review the goal of the respecification was to ensure each opt param
>> > parsed would not reset a prior set param, a paranoid measure, however
>> > this clearly does not work well if we want to allow for "last entry
>> > specified wins", or re-use the validators for a config file parsing
>> > for a first shot a parsing entries.
>>
>> Which is essentially broken, because doing something like:
>>
>> -m crc =1 -m reflink=1 -m crc=0
>>
>> leaves you with an /invalid config/ because of the respecification
>> of -m crc=0 and the order in which options are parsed and verified.
>>
>> Indeed, things like block and sector sizes are particularly nasty in
>> this respect, because other options can be specified in block or
>> sector units. SO things like:
>>
>> -s size=4k -b size=1s -s size=512 -d size=1000000s
>>
>> were considered valid. respecification of options like this is just
>> borken, and even if we take "last specification wins" it still means
>> that the block size specification is ambiguous and potentially
>> incorrect depending on other options. Hence respecification of
>> options is simply not allowed and post-processing of the options
>> doesn't change that.
>
> We have to pick an approach and stick with, the above seems sensible.
>
>> i.e., the biggest issue with reusing the existing parsing code for
>> the "default config" is that is doesn't just set default values - it
>> prevents other options from being used.
>
> Right as per original design.
>
>> IOWs, the config file should
>> set the default values in the option table, not set the options
>> directly as happens on the command line....
>
> As I respin my patches addressing concerns an issue I see with this is current
> semantics for "defaultval" is not that they will be the defaults, but rather
> they will be the defaults *iff* the user did specify the option on the command
> line but did not provide an explicit value. This for example would not allow
Yes, a confusing name. I'm aware of it and have a fix to rename it to .flagval.
It was part of my big set before and now I moved it into the smaller
set I submitted
on this Sunday. The same set adds a new field .value, which can be used to
specify default as in "if the user does not specify this option at all".
Jan
> distributions to disable a feature which perhaps is enabled by default in
> future versions xfsprogs, or enable one which perhaps is by default disabled.
> Even if the config file would have values specified, they will be ignored
> unless the user passed a user input value with no value for it.
>
> If this is fine by everyone then great.
>
> Luis
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-04-24 7:26 ` Jan Tulak
@ 2017-04-24 8:25 ` Luis R. Rodriguez
2017-05-11 22:46 ` Luis R. Rodriguez
0 siblings, 1 reply; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-04-24 8:25 UTC (permalink / raw)
To: Jan Tulak
Cc: Dave Chinner, Eric Sandeen, linux-xfs, jack, Jeff Mahoney, okurz,
Libor Pechacek
On Mon, Apr 24, 2017 at 12:26 AM, Jan Tulak <jtulak@redhat.com> wrote:
> On Mon, Apr 24, 2017 at 7:00 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
>>> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
>>> > I used reset_opt() and went with "last entry specified wins". From my
>>> > review the goal of the respecification was to ensure each opt param
>>> > parsed would not reset a prior set param, a paranoid measure, however
>>> > this clearly does not work well if we want to allow for "last entry
>>> > specified wins", or re-use the validators for a config file parsing
>>> > for a first shot a parsing entries.
>>>
>>> Which is essentially broken, because doing something like:
>>>
>>> -m crc =1 -m reflink=1 -m crc=0
>>>
>>> leaves you with an /invalid config/ because of the respecification
>>> of -m crc=0 and the order in which options are parsed and verified.
>>>
>>> Indeed, things like block and sector sizes are particularly nasty in
>>> this respect, because other options can be specified in block or
>>> sector units. SO things like:
>>>
>>> -s size=4k -b size=1s -s size=512 -d size=1000000s
>>>
>>> were considered valid. respecification of options like this is just
>>> borken, and even if we take "last specification wins" it still means
>>> that the block size specification is ambiguous and potentially
>>> incorrect depending on other options. Hence respecification of
>>> options is simply not allowed and post-processing of the options
>>> doesn't change that.
>>
>> We have to pick an approach and stick with, the above seems sensible.
>>
>>> i.e., the biggest issue with reusing the existing parsing code for
>>> the "default config" is that is doesn't just set default values - it
>>> prevents other options from being used.
>>
>> Right as per original design.
>>
>>> IOWs, the config file should
>>> set the default values in the option table, not set the options
>>> directly as happens on the command line....
>>
>> As I respin my patches addressing concerns an issue I see with this is current
>> semantics for "defaultval" is not that they will be the defaults, but rather
>> they will be the defaults *iff* the user did specify the option on the command
>> line but did not provide an explicit value. This for example would not allow
>
> Yes, a confusing name. I'm aware of it and have a fix to rename it to .flagval.
> It was part of my big set before and now I moved it into the smaller
> set I submitted
> on this Sunday. The same set adds a new field .value, which can be used to
> specify default as in "if the user does not specify this option at all".
Terrific, thanks will use that.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-04-24 8:25 ` Luis R. Rodriguez
@ 2017-05-11 22:46 ` Luis R. Rodriguez
2017-05-11 22:57 ` Eric Sandeen
2017-05-11 23:00 ` Darrick J. Wong
0 siblings, 2 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-05-11 22:46 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jan Tulak, Dave Chinner, Eric Sandeen, linux-xfs, jack,
Jeff Mahoney, okurz, Libor Pechacek
On Mon, Apr 24, 2017 at 01:25:03AM -0700, Luis R. Rodriguez wrote:
> On Mon, Apr 24, 2017 at 12:26 AM, Jan Tulak <jtulak@redhat.com> wrote:
> > On Mon, Apr 24, 2017 at 7:00 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
> >>> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
> >>> > I used reset_opt() and went with "last entry specified wins". From my
> >>> > review the goal of the respecification was to ensure each opt param
> >>> > parsed would not reset a prior set param, a paranoid measure, however
> >>> > this clearly does not work well if we want to allow for "last entry
> >>> > specified wins", or re-use the validators for a config file parsing
> >>> > for a first shot a parsing entries.
> >>>
> >>> Which is essentially broken, because doing something like:
> >>>
> >>> -m crc =1 -m reflink=1 -m crc=0
> >>>
> >>> leaves you with an /invalid config/ because of the respecification
> >>> of -m crc=0 and the order in which options are parsed and verified.
> >>>
> >>> Indeed, things like block and sector sizes are particularly nasty in
> >>> this respect, because other options can be specified in block or
> >>> sector units. SO things like:
> >>>
> >>> -s size=4k -b size=1s -s size=512 -d size=1000000s
> >>>
> >>> were considered valid. respecification of options like this is just
> >>> borken, and even if we take "last specification wins" it still means
> >>> that the block size specification is ambiguous and potentially
> >>> incorrect depending on other options. Hence respecification of
> >>> options is simply not allowed and post-processing of the options
> >>> doesn't change that.
> >>
> >> We have to pick an approach and stick with, the above seems sensible.
> >>
> >>> i.e., the biggest issue with reusing the existing parsing code for
> >>> the "default config" is that is doesn't just set default values - it
> >>> prevents other options from being used.
> >>
> >> Right as per original design.
> >>
> >>> IOWs, the config file should
> >>> set the default values in the option table, not set the options
> >>> directly as happens on the command line....
> >>
> >> As I respin my patches addressing concerns an issue I see with this is current
> >> semantics for "defaultval" is not that they will be the defaults, but rather
> >> they will be the defaults *iff* the user did specify the option on the command
> >> line but did not provide an explicit value. This for example would not allow
> >
> > Yes, a confusing name. I'm aware of it and have a fix to rename it to .flagval.
> > It was part of my big set before and now I moved it into the smaller
> > set I submitted
> > on this Sunday. The same set adds a new field .value, which can be used to
> > specify default as in "if the user does not specify this option at all".
>
> Terrific, thanks will use that.
FWIW, I've looked at ways to address this without your future work Jan, ie
backporting this feature, and ultimately have decided to *not* allow any
command line overwrite for options specified in the configuration file. So
for the backported versions of this feature a user will only be able to
overwrite if the config file is commented out or removed.
How we end up doing this upstream may differ given we may have a way to
properly do sanity checks overall and treat "defaults" as real "defaults".
But without such mechanisms implementing a sensible way to overwrite things
in a compatible way was just crap.
As such for the backported versions of this feature I'll make this big note
on the man page:
"""
One of the uses of the configuration file is to enable distributions
to provide mkfs.xfs(8) updates from a base distribution release and enable to
create filesystems which are sure to remain supported and compatible. As such
systems with a mkfs.xfs.conf(5) file present have very likely been well thought
out, and overriding configuration file defaults is discouraged unless you
know what you are doing and are familiar with the associated risks. If you
know what you are doing, wish to waive compatibility, and wish to overwrite the
configuration file provided the best option is to either remove or uncomment
the configuration file completely as options cannot be overwritten on the
command line.
"""
Also FWIW, I've been thinking more about the option checks and I think its
worth clarifying we currently do things one way and if we really wish to
enable command line overwrites (I'm not sure now its worth it) then we
should reconsider how we do checks a bit differently.
I. We currently have a set of options possible, list conflicts to check for
if this option is set, and set min / max values for it. We also only
allow setting an option once (respecification check).
II. As we parse the command line we do sanity checks for respecification,
conflicts, max/min, in that order.
III. Once we are done parsing all options we do our own internal checks
within main() which are beyond the options checks.
Without considering Jan's work, if we wanted to allow overwrites, one example
issue is that if you wanted to keep the respecification check is you'd only
enable overwrites by enabling to disable a feature using the same option. This
sort of makes sense as otherwise things may be implicit but from a documentation
and user experience perspective this seems very odd.
It seems more sensible for us to:
I. Use the same options, but extracting system defaults and checking these
defaults against conflicts checks, min/max checks (It seems Jan's work is
moving in this direction).
II. Do a full sanity check to ensure all mix of options makes sense, have
this as a helper we can re-suse, verify_all_opts() or whatever. This would
move a lot of checks we have after main() into a helper.
III. Parse config file and after all options are parsed also verify_all_opts()
IV. Parse cmd line options and after all options are parsed also verify_all_opts()
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-11 22:46 ` Luis R. Rodriguez
@ 2017-05-11 22:57 ` Eric Sandeen
2017-05-11 23:08 ` Luis R. Rodriguez
2017-05-11 23:00 ` Darrick J. Wong
1 sibling, 1 reply; 40+ messages in thread
From: Eric Sandeen @ 2017-05-11 22:57 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jan Tulak, Dave Chinner, linux-xfs, jack, Jeff Mahoney, okurz,
Libor Pechacek
On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
> FWIW, I've looked at ways to address this without your future work Jan, ie
> backporting this feature, and ultimately have decided to *not* allow any
> command line overwrite for options specified in the configuration file. So
> for the backported versions of this feature a user will only be able to
> overwrite if the config file is commented out or removed.
>
> How we end up doing this upstream may differ given we may have a way to
> properly do sanity checks overall and treat "defaults" as real "defaults".
> But without such mechanisms implementing a sensible way to overwrite things
> in a compatible way was just crap.
>
> As such for the backported versions of this feature I'll make this big note
> on the man page:
I'm a little confused - backported from where to where? I'm not sure what
a "backport" means in this context, when there is no upstream solution at this
time.
> """
> One of the uses of the configuration file is to enable distributions
> to provide mkfs.xfs(8) updates from a base distribution release and enable to
> create filesystems which are sure to remain supported and compatible. As such
> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
> out, and overriding configuration file defaults is discouraged unless you
> know what you are doing and are familiar with the associated risks. If you
> know what you are doing, wish to waive compatibility, and wish to overwrite the
> configuration file provided the best option is to either remove or uncomment
> the configuration file completely as options cannot be overwritten on the
> command line.
> """
So are you planning a forked, non-upstream behavior for your distro?
I think that disallowing commandline overrides of configfile settings is a
mistake, and not what we'd want upstream. If you do it as a fork, mkfs should
fail if conflicting options are specified, IMHO. The worst of all possible
worlds is an admin typing an otherwise valid mkfs command, and getting a
"successful" result which is /not/ what was specified by the user.
Honestly, until an upstream solution is found, simply patching in new defaults
seems safest (and least-element-of-surprise) for a distro.
Thanks,
-Eric
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-11 22:57 ` Eric Sandeen
@ 2017-05-11 23:08 ` Luis R. Rodriguez
2017-05-12 0:48 ` Darrick J. Wong
2017-05-12 16:05 ` Eric Sandeen
0 siblings, 2 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-05-11 23:08 UTC (permalink / raw)
To: Eric Sandeen
Cc: Jan Tulak, Dave Chinner, linux-xfs, jack, Jeff Mahoney, okurz,
Libor Pechacek
On Thu, May 11, 2017 at 3:57 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
>
>> FWIW, I've looked at ways to address this without your future work Jan, ie
>> backporting this feature, and ultimately have decided to *not* allow any
>> command line overwrite for options specified in the configuration file. So
>> for the backported versions of this feature a user will only be able to
>> overwrite if the config file is commented out or removed.
>>
>> How we end up doing this upstream may differ given we may have a way to
>> properly do sanity checks overall and treat "defaults" as real "defaults".
>> But without such mechanisms implementing a sensible way to overwrite things
>> in a compatible way was just crap.
>>
>> As such for the backported versions of this feature I'll make this big note
>> on the man page:
>
> I'm a little confused - backported from where to where? I'm not sure what
> a "backport" means in this context, when there is no upstream solution at this
> time.
Since we're still waiting for a bit of delta before I can push this
work then from my development tree to a stable older release.
>> """
>> One of the uses of the configuration file is to enable distributions
>> to provide mkfs.xfs(8) updates from a base distribution release and enable to
>> create filesystems which are sure to remain supported and compatible. As such
>> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
>> out, and overriding configuration file defaults is discouraged unless you
>> know what you are doing and are familiar with the associated risks. If you
>> know what you are doing, wish to waive compatibility, and wish to overwrite the
>> configuration file provided the best option is to either remove or uncomment
>> the configuration file completely as options cannot be overwritten on the
>> command line.
>> """
>
> So are you planning a forked, non-upstream behavior for your distro?
Right.
> I think that disallowing commandline overrides of configfile settings is a
> mistake, and not what we'd want upstream.
For upstream I agree. Its how config files typically work after all.
> If you do it as a fork, mkfs should fail if conflicting options are specified, IMHO.
Sure, conflict check will be retained given the same command line
option mechanism would be used to set whatever is in the config file.
> The worst of all possible
> worlds is an admin typing an otherwise valid mkfs command, and getting a
> "successful" result which is /not/ what was specified by the user.
Right.
> Honestly, until an upstream solution is found, simply patching in new defaults
> seems safest (and least-element-of-surprise) for a distro.
Thing is we have no such easy "default" mechanism today. In the future
it sounds like this will change so what you describe should be much
easier. Sure, today a few options are set to 0 on main(), but trying
to see when an option actually gets set to a default value if no other
options are set is non trivial.
As for the backported approach, I don't expect the config file to
retain many options after all, only what is necessary to retain
compatibility with a base distro release.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-11 23:08 ` Luis R. Rodriguez
@ 2017-05-12 0:48 ` Darrick J. Wong
2017-05-12 16:05 ` Eric Sandeen
1 sibling, 0 replies; 40+ messages in thread
From: Darrick J. Wong @ 2017-05-12 0:48 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Eric Sandeen, Jan Tulak, Dave Chinner, linux-xfs, jack,
Jeff Mahoney, okurz, Libor Pechacek
On Thu, May 11, 2017 at 04:08:59PM -0700, Luis R. Rodriguez wrote:
> On Thu, May 11, 2017 at 3:57 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> > On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
> >
> >> FWIW, I've looked at ways to address this without your future work Jan, ie
> >> backporting this feature, and ultimately have decided to *not* allow any
> >> command line overwrite for options specified in the configuration file. So
> >> for the backported versions of this feature a user will only be able to
> >> overwrite if the config file is commented out or removed.
> >>
> >> How we end up doing this upstream may differ given we may have a way to
> >> properly do sanity checks overall and treat "defaults" as real "defaults".
> >> But without such mechanisms implementing a sensible way to overwrite things
> >> in a compatible way was just crap.
> >>
> >> As such for the backported versions of this feature I'll make this big note
> >> on the man page:
> >
> > I'm a little confused - backported from where to where? I'm not sure what
> > a "backport" means in this context, when there is no upstream solution at this
> > time.
>
> Since we're still waiting for a bit of delta before I can push this
> work then from my development tree to a stable older release.
>
> >> """
> >> One of the uses of the configuration file is to enable distributions
> >> to provide mkfs.xfs(8) updates from a base distribution release and enable to
> >> create filesystems which are sure to remain supported and compatible. As such
> >> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
> >> out, and overriding configuration file defaults is discouraged unless you
> >> know what you are doing and are familiar with the associated risks. If you
> >> know what you are doing, wish to waive compatibility, and wish to overwrite the
> >> configuration file provided the best option is to either remove or uncomment
> >> the configuration file completely as options cannot be overwritten on the
> >> command line.
> >> """
> >
> > So are you planning a forked, non-upstream behavior for your distro?
>
> Right.
Correct me if I'm wrong, but with this proposal we end up with the SuSE
mkfs.xfs where if I write "rmapbt=0" in /etc/xfs.conf then I cannot
later run `mkfs.xfs -m rmapbt=1` and have rmapbt turned on; whereas
with the upstream mkfs.xfs if I write "rmapbt=0" in /etc/xfs.conf I
/can/ later run `mkfs.fs -m rmapbt=1` and rmapbt gets turned on?
That will create a lot of user script pain when command line options
cause mkfs to fail in SuSE but they work fine everywhere else, right?
--D
>
> > I think that disallowing commandline overrides of configfile settings is a
> > mistake, and not what we'd want upstream.
>
> For upstream I agree. Its how config files typically work after all.
>
> > If you do it as a fork, mkfs should fail if conflicting options are specified, IMHO.
>
> Sure, conflict check will be retained given the same command line
> option mechanism would be used to set whatever is in the config file.
>
> > The worst of all possible
> > worlds is an admin typing an otherwise valid mkfs command, and getting a
> > "successful" result which is /not/ what was specified by the user.
>
> Right.
>
> > Honestly, until an upstream solution is found, simply patching in new defaults
> > seems safest (and least-element-of-surprise) for a distro.
>
> Thing is we have no such easy "default" mechanism today. In the future
> it sounds like this will change so what you describe should be much
> easier. Sure, today a few options are set to 0 on main(), but trying
> to see when an option actually gets set to a default value if no other
> options are set is non trivial.
>
> As for the backported approach, I don't expect the config file to
> retain many options after all, only what is necessary to retain
> compatibility with a base distro release.
>
> Luis
> --
> 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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-11 23:08 ` Luis R. Rodriguez
2017-05-12 0:48 ` Darrick J. Wong
@ 2017-05-12 16:05 ` Eric Sandeen
2017-05-12 17:03 ` Luis R. Rodriguez
2017-05-12 17:05 ` Jeff Mahoney
1 sibling, 2 replies; 40+ messages in thread
From: Eric Sandeen @ 2017-05-12 16:05 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jan Tulak, Dave Chinner, linux-xfs, jack, Jeff Mahoney, okurz,
Libor Pechacek
On 5/11/17 6:08 PM, Luis R. Rodriguez wrote:
> On Thu, May 11, 2017 at 3:57 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
>>
>>> FWIW, I've looked at ways to address this without your future work Jan, ie
>>> backporting this feature, and ultimately have decided to *not* allow any
>>> command line overwrite for options specified in the configuration file. So
>>> for the backported versions of this feature a user will only be able to
>>> overwrite if the config file is commented out or removed.
>>>
>>> How we end up doing this upstream may differ given we may have a way to
>>> properly do sanity checks overall and treat "defaults" as real "defaults".
>>> But without such mechanisms implementing a sensible way to overwrite things
>>> in a compatible way was just crap.
>>>
>>> As such for the backported versions of this feature I'll make this big note
>>> on the man page:
>>
>> I'm a little confused - backported from where to where? I'm not sure what
>> a "backport" means in this context, when there is no upstream solution at this
>> time.
>
> Since we're still waiting for a bit of delta before I can push this
> work then from my development tree to a stable older release.
>
>>> """
>>> One of the uses of the configuration file is to enable distributions
>>> to provide mkfs.xfs(8) updates from a base distribution release and enable to
>>> create filesystems which are sure to remain supported and compatible. As such
>>> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
>>> out, and overriding configuration file defaults is discouraged unless you
>>> know what you are doing and are familiar with the associated risks. If you
>>> know what you are doing, wish to waive compatibility, and wish to overwrite the
>>> configuration file provided the best option is to either remove or uncomment
>>> the configuration file completely as options cannot be overwritten on the
>>> command line.
>>> """
>>
>> So are you planning a forked, non-upstream behavior for your distro?
>
> Right.
I'm not really in a position to tell a distro what to do, other than out of
concern for polluting user expectations with non-upstream behaviors. Which
/is/ a concern; I'm not looking forward to complaints from your users that
upstream has "broken" behavior w.r.t. your fork when we (finally...?) ship a
config-file capable mkfs.xfs.
i.e. you go that route, your forked behavior will differ from any behavior
that we ever ship upstream. Your users will eventually need to adapt to
considerably different behavior which is unique to your distro, and just to
be clear: "but we already shipped it!" will not hold any weight whatsoever
in future upstream behavior discussions...
If you want to ship newer progs w/ older defaults, I really don't understand
why you can't just revert the patches that added the new defaults.
-Eric
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-12 16:05 ` Eric Sandeen
@ 2017-05-12 17:03 ` Luis R. Rodriguez
2017-05-12 17:05 ` Jeff Mahoney
1 sibling, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-05-12 17:03 UTC (permalink / raw)
To: Eric Sandeen
Cc: Luis R. Rodriguez, Jan Tulak, Dave Chinner, linux-xfs, jack,
Jeff Mahoney, okurz, Libor Pechacek
On Fri, May 12, 2017 at 11:05:13AM -0500, Eric Sandeen wrote:
> On 5/11/17 6:08 PM, Luis R. Rodriguez wrote:
> > On Thu, May 11, 2017 at 3:57 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> >> On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
> >>
> >>> FWIW, I've looked at ways to address this without your future work Jan, ie
> >>> backporting this feature, and ultimately have decided to *not* allow any
> >>> command line overwrite for options specified in the configuration file. So
> >>> for the backported versions of this feature a user will only be able to
> >>> overwrite if the config file is commented out or removed.
> >>>
> >>> How we end up doing this upstream may differ given we may have a way to
> >>> properly do sanity checks overall and treat "defaults" as real "defaults".
> >>> But without such mechanisms implementing a sensible way to overwrite things
> >>> in a compatible way was just crap.
> >>>
> >>> As such for the backported versions of this feature I'll make this big note
> >>> on the man page:
> >>
> >> I'm a little confused - backported from where to where? I'm not sure what
> >> a "backport" means in this context, when there is no upstream solution at this
> >> time.
> >
> > Since we're still waiting for a bit of delta before I can push this
> > work then from my development tree to a stable older release.
> >
> >>> """
> >>> One of the uses of the configuration file is to enable distributions
> >>> to provide mkfs.xfs(8) updates from a base distribution release and enable to
> >>> create filesystems which are sure to remain supported and compatible. As such
> >>> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
> >>> out, and overriding configuration file defaults is discouraged unless you
> >>> know what you are doing and are familiar with the associated risks. If you
> >>> know what you are doing, wish to waive compatibility, and wish to overwrite the
> >>> configuration file provided the best option is to either remove or uncomment
> >>> the configuration file completely as options cannot be overwritten on the
> >>> command line.
> >>> """
> >>
> >> So are you planning a forked, non-upstream behavior for your distro?
> >
> > Right.
>
> I'm not really in a position to tell a distro what to do, other than out of
> concern for polluting user expectations with non-upstream behaviors. Which
> /is/ a concern; I'm not looking forward to complaints from your users that
> upstream has "broken" behavior w.r.t. your fork when we (finally...?) ship a
> config-file capable mkfs.xfs.
Its a fair concern, and I will try once more to see if I can figure out a clean
way to enable overwrites without any of Jan's work. The point of my email was
to give a heads up its currently a mess to do properly, if you want to retain
respecification checks, conflict checks, min/max checks. My original reset_opt()
approach which this thread demo'd clearly was not sufficient given the respec/
conflict check issues pointed out.
> i.e. you go that route, your forked behavior will differ from any behavior
> that we ever ship upstream. Your users will eventually need to adapt to
> considerably different behavior which is unique to your distro, and just to
> be clear: "but we already shipped it!" will not hold any weight whatsoever
> in future upstream behavior discussions...
>
> If you want to ship newer progs w/ older defaults, I really don't understand
> why you can't just revert the patches that added the new defaults.
Its not that easy, we want the ability to support new features but remain
compatible with a base distribution release. As noted earlier in this thread
currently "defaultval" is a complete misnomer, its the used default value but
only if the user specified the option without a value. If you want to figure
out what the real default is you have to go through the code and figure each
value out.
Another point to my email was to highlight how important Jan's work is to
moving forward with anything sensible.
If I do come up with a sensible way to retain overwrites I'll let you'll know.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-12 16:05 ` Eric Sandeen
2017-05-12 17:03 ` Luis R. Rodriguez
@ 2017-05-12 17:05 ` Jeff Mahoney
2017-05-12 17:30 ` Luis R. Rodriguez
1 sibling, 1 reply; 40+ messages in thread
From: Jeff Mahoney @ 2017-05-12 17:05 UTC (permalink / raw)
To: Eric Sandeen, Luis R. Rodriguez
Cc: Jan Tulak, Dave Chinner, linux-xfs, jack, okurz, Libor Pechacek
[-- Attachment #1.1: Type: text/plain, Size: 4607 bytes --]
On 5/12/17 12:05 PM, Eric Sandeen wrote:
> On 5/11/17 6:08 PM, Luis R. Rodriguez wrote:
>> On Thu, May 11, 2017 at 3:57 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>>> On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
>>>
>>>> FWIW, I've looked at ways to address this without your future work Jan, ie
>>>> backporting this feature, and ultimately have decided to *not* allow any
>>>> command line overwrite for options specified in the configuration file. So
>>>> for the backported versions of this feature a user will only be able to
>>>> overwrite if the config file is commented out or removed.
>>>>
>>>> How we end up doing this upstream may differ given we may have a way to
>>>> properly do sanity checks overall and treat "defaults" as real "defaults".
>>>> But without such mechanisms implementing a sensible way to overwrite things
>>>> in a compatible way was just crap.
>>>>
>>>> As such for the backported versions of this feature I'll make this big note
>>>> on the man page:
>>>
>>> I'm a little confused - backported from where to where? I'm not sure what
>>> a "backport" means in this context, when there is no upstream solution at this
>>> time.
>>
>> Since we're still waiting for a bit of delta before I can push this
>> work then from my development tree to a stable older release.
>>
>>>> """
>>>> One of the uses of the configuration file is to enable distributions
>>>> to provide mkfs.xfs(8) updates from a base distribution release and enable to
>>>> create filesystems which are sure to remain supported and compatible. As such
>>>> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
>>>> out, and overriding configuration file defaults is discouraged unless you
>>>> know what you are doing and are familiar with the associated risks. If you
>>>> know what you are doing, wish to waive compatibility, and wish to overwrite the
>>>> configuration file provided the best option is to either remove or uncomment
>>>> the configuration file completely as options cannot be overwritten on the
>>>> command line.
>>>> """
>>>
>>> So are you planning a forked, non-upstream behavior for your distro?
>>
>> Right.
>
> I'm not really in a position to tell a distro what to do, other than out of
> concern for polluting user expectations with non-upstream behaviors. Which
> /is/ a concern; I'm not looking forward to complaints from your users that
> upstream has "broken" behavior w.r.t. your fork when we (finally...?) ship a
> config-file capable mkfs.xfs.
>
> i.e. you go that route, your forked behavior will differ from any behavior
> that we ever ship upstream. Your users will eventually need to adapt to
> considerably different behavior which is unique to your distro, and just to
> be clear: "but we already shipped it!" will not hold any weight whatsoever
> in future upstream behavior discussions...
>
> If you want to ship newer progs w/ older defaults, I really don't understand
> why you can't just revert the patches that added the new defaults.
The goal is to have one package that we can distribute and update across
releases and /not/ impact the experience for the user. We'd ship the
config file with the release as a separate package and could update
xfsprogs at will.
I don't like the idea that the config file is the "one true" config
file, though. The user should still be able to override options on the
command line. The divergence from the upstream package should be
limited *only* to the defaults, not to the user experience.
I'm fine having this depend on Jan's work to make the defaults more
sane. This is a wish-list item for us and it doesn't need to land tomorrow.
I really think the way it should work is as if the options from the
config file override the built-in options in the same way specifying
them on the command line would. Subsequently, options on the actual
command line override those. This seems to be the path to least user
surprise. The weirdness with specifying units on the command line
affecting other options may make this a little more tricky than it's
laid out here, but the principle should be to surprise the user the
least. Having this weird divergent "sorry you have a config file and
can't override anything anymore" violates that. mkfs should issue a
message like "Using built-in defaults" or "Using defaults from <config
path>" so it's clear, especially for user reports, where the defaults
are coming from if there is unexpected behavior.
-Jeff
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-12 17:05 ` Jeff Mahoney
@ 2017-05-12 17:30 ` Luis R. Rodriguez
0 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-05-12 17:30 UTC (permalink / raw)
To: Jeff Mahoney
Cc: Eric Sandeen, Jan Tulak, Dave Chinner, linux-xfs, jack, okurz,
Libor Pechacek
On Fri, May 12, 2017 at 10:05 AM, Jeff Mahoney <jeffm@suse.com> wrote:
> On 5/12/17 12:05 PM, Eric Sandeen wrote:
>> On 5/11/17 6:08 PM, Luis R. Rodriguez wrote:
>>> On Thu, May 11, 2017 at 3:57 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>>>> On 5/11/17 5:46 PM, Luis R. Rodriguez wrote:
>>>>
>>>>> FWIW, I've looked at ways to address this without your future work Jan, ie
>>>>> backporting this feature, and ultimately have decided to *not* allow any
>>>>> command line overwrite for options specified in the configuration file. So
>>>>> for the backported versions of this feature a user will only be able to
>>>>> overwrite if the config file is commented out or removed.
>>>>>
>>>>> How we end up doing this upstream may differ given we may have a way to
>>>>> properly do sanity checks overall and treat "defaults" as real "defaults".
>>>>> But without such mechanisms implementing a sensible way to overwrite things
>>>>> in a compatible way was just crap.
>>>>>
>>>>> As such for the backported versions of this feature I'll make this big note
>>>>> on the man page:
>>>>
>>>> I'm a little confused - backported from where to where? I'm not sure what
>>>> a "backport" means in this context, when there is no upstream solution at this
>>>> time.
>>>
>>> Since we're still waiting for a bit of delta before I can push this
>>> work then from my development tree to a stable older release.
>>>
>>>>> """
>>>>> One of the uses of the configuration file is to enable distributions
>>>>> to provide mkfs.xfs(8) updates from a base distribution release and enable to
>>>>> create filesystems which are sure to remain supported and compatible. As such
>>>>> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
>>>>> out, and overriding configuration file defaults is discouraged unless you
>>>>> know what you are doing and are familiar with the associated risks. If you
>>>>> know what you are doing, wish to waive compatibility, and wish to overwrite the
>>>>> configuration file provided the best option is to either remove or uncomment
>>>>> the configuration file completely as options cannot be overwritten on the
>>>>> command line.
>>>>> """
>>>>
>>>> So are you planning a forked, non-upstream behavior for your distro?
>>>
>>> Right.
>>
>> I'm not really in a position to tell a distro what to do, other than out of
>> concern for polluting user expectations with non-upstream behaviors. Which
>> /is/ a concern; I'm not looking forward to complaints from your users that
>> upstream has "broken" behavior w.r.t. your fork when we (finally...?) ship a
>> config-file capable mkfs.xfs.
>>
>> i.e. you go that route, your forked behavior will differ from any behavior
>> that we ever ship upstream. Your users will eventually need to adapt to
>> considerably different behavior which is unique to your distro, and just to
>> be clear: "but we already shipped it!" will not hold any weight whatsoever
>> in future upstream behavior discussions...
>>
>> If you want to ship newer progs w/ older defaults, I really don't understand
>> why you can't just revert the patches that added the new defaults.
>
> The goal is to have one package that we can distribute and update across
> releases and /not/ impact the experience for the user. We'd ship the
> config file with the release as a separate package and could update
> xfsprogs at will.
>
> I don't like the idea that the config file is the "one true" config
> file, though. The user should still be able to override options on the
> command line. The divergence from the upstream package should be
> limited *only* to the defaults, not to the user experience.
>
> I'm fine having this depend on Jan's work to make the defaults more
> sane. This is a wish-list item for us and it doesn't need to land tomorrow.
Perfect!
> I really think the way it should work is as if the options from the
> config file override the built-in options in the same way specifying
> them on the command line would.
I'm glad we all seem to agree with this.
> Subsequently, options on the actual
> command line override those.
Likewise.
> This seems to be the path to least user
> surprise. The weirdness with specifying units on the command line
> affecting other options may make this a little more tricky than it's
> laid out here,
Indeed, also, other than the built-in structural sub params checks
(respec, conflicts, min, max), we have other sanity checks which run
after command line processing, my point was to highlight that these
probably should be helpers and we should be able to run this at any
given time after any new input is given.
> but the principle should be to surprise the user the
> least.
Great.
> Having this weird divergent "sorry you have a config file and
> can't override anything anymore" violates that. mkfs should issue a
> message like "Using built-in defaults" or "Using defaults from <config
> path>" so it's clear, especially for user reports, where the defaults
> are coming from if there is unexpected behavior.
Makes sense. I'm afraid then to get this right and do it properly
xfsprogs is just a bit long way before it can be done and this will
just have to wait a bit more.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-11 22:46 ` Luis R. Rodriguez
2017-05-11 22:57 ` Eric Sandeen
@ 2017-05-11 23:00 ` Darrick J. Wong
2017-05-11 23:19 ` Luis R. Rodriguez
1 sibling, 1 reply; 40+ messages in thread
From: Darrick J. Wong @ 2017-05-11 23:00 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Jan Tulak, Dave Chinner, Eric Sandeen, linux-xfs, jack,
Jeff Mahoney, okurz, Libor Pechacek
On Fri, May 12, 2017 at 12:46:03AM +0200, Luis R. Rodriguez wrote:
> On Mon, Apr 24, 2017 at 01:25:03AM -0700, Luis R. Rodriguez wrote:
> > On Mon, Apr 24, 2017 at 12:26 AM, Jan Tulak <jtulak@redhat.com> wrote:
> > > On Mon, Apr 24, 2017 at 7:00 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > >> On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
> > >>> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
> > >>> > I used reset_opt() and went with "last entry specified wins". From my
> > >>> > review the goal of the respecification was to ensure each opt param
> > >>> > parsed would not reset a prior set param, a paranoid measure, however
> > >>> > this clearly does not work well if we want to allow for "last entry
> > >>> > specified wins", or re-use the validators for a config file parsing
> > >>> > for a first shot a parsing entries.
> > >>>
> > >>> Which is essentially broken, because doing something like:
> > >>>
> > >>> -m crc =1 -m reflink=1 -m crc=0
> > >>>
> > >>> leaves you with an /invalid config/ because of the respecification
> > >>> of -m crc=0 and the order in which options are parsed and verified.
> > >>>
> > >>> Indeed, things like block and sector sizes are particularly nasty in
> > >>> this respect, because other options can be specified in block or
> > >>> sector units. SO things like:
> > >>>
> > >>> -s size=4k -b size=1s -s size=512 -d size=1000000s
> > >>>
> > >>> were considered valid. respecification of options like this is just
> > >>> borken, and even if we take "last specification wins" it still means
> > >>> that the block size specification is ambiguous and potentially
> > >>> incorrect depending on other options. Hence respecification of
> > >>> options is simply not allowed and post-processing of the options
> > >>> doesn't change that.
> > >>
> > >> We have to pick an approach and stick with, the above seems sensible.
> > >>
> > >>> i.e., the biggest issue with reusing the existing parsing code for
> > >>> the "default config" is that is doesn't just set default values - it
> > >>> prevents other options from being used.
> > >>
> > >> Right as per original design.
> > >>
> > >>> IOWs, the config file should
> > >>> set the default values in the option table, not set the options
> > >>> directly as happens on the command line....
> > >>
> > >> As I respin my patches addressing concerns an issue I see with this is current
> > >> semantics for "defaultval" is not that they will be the defaults, but rather
> > >> they will be the defaults *iff* the user did specify the option on the command
> > >> line but did not provide an explicit value. This for example would not allow
> > >
> > > Yes, a confusing name. I'm aware of it and have a fix to rename it to .flagval.
> > > It was part of my big set before and now I moved it into the smaller
> > > set I submitted
> > > on this Sunday. The same set adds a new field .value, which can be used to
> > > specify default as in "if the user does not specify this option at all".
> >
> > Terrific, thanks will use that.
>
> FWIW, I've looked at ways to address this without your future work Jan, ie
> backporting this feature, and ultimately have decided to *not* allow any
> command line overwrite for options specified in the configuration file. So
> for the backported versions of this feature a user will only be able to
> overwrite if the config file is commented out or removed.
Waitaminute, config file directives lock out command line options??
I suppose that would make sense if one of the config file options was:
disable_overrides = true
...but I think you're talking about /never/ allowing overrides, right?
I am under the impression that we have (a) mkfs defaults in the source
code that can be overridden in (b) the config file which in turn can be
overridden by the administrator via (c) the command line.
In other words, we (upstream) set whatever defaults we think are sane,
then distros can set the defaults they want to support, and the admin
can change things as they see fit for their site. If the administrator
wants to use non-default settings, they're welcome to support that
themselves (or hire someone to do it for them). We don't prhoibit that.
The way I describe is the way that mke2fs works, and afaict most other
programs operate that way too. mkfs.xfs has a long history of "things
you put on the command line are what you get in the fs", and changing it
now is breaking peoples' mental models of how mkfs.xfs works, and in a
way that runs counter to most other programs.
No thank you to this.
--D
>
> How we end up doing this upstream may differ given we may have a way to
> properly do sanity checks overall and treat "defaults" as real "defaults".
> But without such mechanisms implementing a sensible way to overwrite things
> in a compatible way was just crap.
>
> As such for the backported versions of this feature I'll make this big note
> on the man page:
>
> """
> One of the uses of the configuration file is to enable distributions
> to provide mkfs.xfs(8) updates from a base distribution release and enable to
> create filesystems which are sure to remain supported and compatible. As such
> systems with a mkfs.xfs.conf(5) file present have very likely been well thought
> out, and overriding configuration file defaults is discouraged unless you
> know what you are doing and are familiar with the associated risks. If you
> know what you are doing, wish to waive compatibility, and wish to overwrite the
> configuration file provided the best option is to either remove or uncomment
> the configuration file completely as options cannot be overwritten on the
> command line.
> """
>
> Also FWIW, I've been thinking more about the option checks and I think its
> worth clarifying we currently do things one way and if we really wish to
> enable command line overwrites (I'm not sure now its worth it) then we
> should reconsider how we do checks a bit differently.
>
> I. We currently have a set of options possible, list conflicts to check for
> if this option is set, and set min / max values for it. We also only
> allow setting an option once (respecification check).
>
> II. As we parse the command line we do sanity checks for respecification,
> conflicts, max/min, in that order.
>
> III. Once we are done parsing all options we do our own internal checks
> within main() which are beyond the options checks.
>
> Without considering Jan's work, if we wanted to allow overwrites, one example
> issue is that if you wanted to keep the respecification check is you'd only
> enable overwrites by enabling to disable a feature using the same option. This
> sort of makes sense as otherwise things may be implicit but from a documentation
> and user experience perspective this seems very odd.
>
> It seems more sensible for us to:
>
> I. Use the same options, but extracting system defaults and checking these
> defaults against conflicts checks, min/max checks (It seems Jan's work is
> moving in this direction).
>
> II. Do a full sanity check to ensure all mix of options makes sense, have
> this as a helper we can re-suse, verify_all_opts() or whatever. This would
> move a lot of checks we have after main() into a helper.
>
> III. Parse config file and after all options are parsed also verify_all_opts()
>
> IV. Parse cmd line options and after all options are parsed also verify_all_opts()
>
> Luis
> --
> 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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support
2017-05-11 23:00 ` Darrick J. Wong
@ 2017-05-11 23:19 ` Luis R. Rodriguez
0 siblings, 0 replies; 40+ messages in thread
From: Luis R. Rodriguez @ 2017-05-11 23:19 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Jan Tulak, Dave Chinner, Eric Sandeen, linux-xfs, jack,
Jeff Mahoney, okurz, Libor Pechacek
On Thu, May 11, 2017 at 4:00 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Fri, May 12, 2017 at 12:46:03AM +0200, Luis R. Rodriguez wrote:
>> On Mon, Apr 24, 2017 at 01:25:03AM -0700, Luis R. Rodriguez wrote:
>> > On Mon, Apr 24, 2017 at 12:26 AM, Jan Tulak <jtulak@redhat.com> wrote:
>> > > On Mon, Apr 24, 2017 at 7:00 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > >> On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
>> > >>> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
>> > >>> > I used reset_opt() and went with "last entry specified wins". From my
>> > >>> > review the goal of the respecification was to ensure each opt param
>> > >>> > parsed would not reset a prior set param, a paranoid measure, however
>> > >>> > this clearly does not work well if we want to allow for "last entry
>> > >>> > specified wins", or re-use the validators for a config file parsing
>> > >>> > for a first shot a parsing entries.
>> > >>>
>> > >>> Which is essentially broken, because doing something like:
>> > >>>
>> > >>> -m crc =1 -m reflink=1 -m crc=0
>> > >>>
>> > >>> leaves you with an /invalid config/ because of the respecification
>> > >>> of -m crc=0 and the order in which options are parsed and verified.
>> > >>>
>> > >>> Indeed, things like block and sector sizes are particularly nasty in
>> > >>> this respect, because other options can be specified in block or
>> > >>> sector units. SO things like:
>> > >>>
>> > >>> -s size=4k -b size=1s -s size=512 -d size=1000000s
>> > >>>
>> > >>> were considered valid. respecification of options like this is just
>> > >>> borken, and even if we take "last specification wins" it still means
>> > >>> that the block size specification is ambiguous and potentially
>> > >>> incorrect depending on other options. Hence respecification of
>> > >>> options is simply not allowed and post-processing of the options
>> > >>> doesn't change that.
>> > >>
>> > >> We have to pick an approach and stick with, the above seems sensible.
>> > >>
>> > >>> i.e., the biggest issue with reusing the existing parsing code for
>> > >>> the "default config" is that is doesn't just set default values - it
>> > >>> prevents other options from being used.
>> > >>
>> > >> Right as per original design.
>> > >>
>> > >>> IOWs, the config file should
>> > >>> set the default values in the option table, not set the options
>> > >>> directly as happens on the command line....
>> > >>
>> > >> As I respin my patches addressing concerns an issue I see with this is current
>> > >> semantics for "defaultval" is not that they will be the defaults, but rather
>> > >> they will be the defaults *iff* the user did specify the option on the command
>> > >> line but did not provide an explicit value. This for example would not allow
>> > >
>> > > Yes, a confusing name. I'm aware of it and have a fix to rename it to .flagval.
>> > > It was part of my big set before and now I moved it into the smaller
>> > > set I submitted
>> > > on this Sunday. The same set adds a new field .value, which can be used to
>> > > specify default as in "if the user does not specify this option at all".
>> >
>> > Terrific, thanks will use that.
>>
>> FWIW, I've looked at ways to address this without your future work Jan, ie
>> backporting this feature, and ultimately have decided to *not* allow any
>> command line overwrite for options specified in the configuration file. So
>> for the backported versions of this feature a user will only be able to
>> overwrite if the config file is commented out or removed.
>
> Waitaminute, config file directives lock out command line options??
Not for upstream, IMHO command line should always be able to overwrite
a config file as is traditionally done.
> I suppose that would make sense if one of the config file options was:
>
> disable_overrides = true
>
> ...but I think you're talking about /never/ allowing overrides, right?
Not for upstream, the question is how to backport this feature without
Jan's big replacement of the kitchen sink, and bathroom decor going
on.
> I am under the impression that we have (a) mkfs defaults in the source
> code that can be overridden in
Not clearly, the variables on the struct are actually "defaults if the
user supplies an argument but does not specify a value"! Without an
command line argument specified its actually a bit tricky to decipher
what is the default option used for a parameter.
With Jan's work this should be clearer later upstream, buts coming in
after the bathroom decor gets a facelift. Without Jan's work this is a
code mystery.
> (b) the config file
Only once the bathroom decor goes in.
> which in turn can be overridden by the administrator via (c) the command line.
Sure, that should be the case upstream later. Modulo I think we some
additional checks typically done today only after main() should be
done after each (a), (b) and (c).
> In other words, we (upstream) set whatever defaults we think are sane,
> then distros can set the defaults they want to support, and the admin
> can change things as they see fit for their site.
Right, upstream-wise I agree.
> If the administrator
> wants to use non-default settings, they're welcome to support that
> themselves (or hire someone to do it for them). We don't prohibit that.
Sure.
> The way I describe is the way that mke2fs works, and afaict most other
> programs operate that way too. mkfs.xfs has a long history of "things
> you put on the command line are what you get in the fs", and changing it
> now is breaking peoples' mental models of how mkfs.xfs works, and in a
> way that runs counter to most other programs.
Sure, we don't want to change that.
Luis
^ permalink raw reply [flat|nested] 40+ messages in thread