| Message ID | 20170815150812.32237-5-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 8/15/17 10:08 AM, Jan Tulak wrote: > This patch adds infrastructure that will be used in subsequent patches. > > The Value field is the actual value used in computations for creating > the filesystem. This is initialized with sensible default values. If > initialized to 0, the value is considered disabled. User input will > override default values. If the field is a string and not a number, the > value is set to a positive non-zero number when user input has been > supplied. > > Add functions that can be used to get/set values to opts table. Ok, I need to back up here a bit and look at this conceptually. (Again, though, if this is infra for some other patchset, I'd just send it with /that/ patchset, not this one ...) But anyway, things like this confuse and worry me: > + case OPT_S: > + switch (subopt) { > + case S_LOG: > + case S_SECTLOG: > + set_conf_val(OPT_S, S_LOG, val); > + set_conf_val(OPT_D, D_SECTLOG, val); > + set_conf_val(OPT_L, L_SECTLOG, val); > + set_conf_val(OPT_D, D_SECTSIZE, 1 << val); > + set_conf_val(OPT_S, S_SIZE, 1 << val); > + set_conf_val(OPT_S, S_SECTSIZE, 1 << val); > + set_conf_val(OPT_L, L_SECTLOG, val); > + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); > + set_conf_val(OPT_L, L_SECTSIZE, val);> + break; > + case S_SIZE: > + case S_SECTSIZE: > + set_conf_val(OPT_S, S_SIZE, val); > + set_conf_val(OPT_D, D_SECTSIZE, val); > + set_conf_val(OPT_D, D_SECTLOG, libxfs_highbit32(val)); > + set_conf_val(OPT_S, S_LOG, libxfs_highbit32(val)); > + set_conf_val(OPT_S, S_SECTLOG, libxfs_highbit32(val)); > + set_conf_val(OPT_L, L_SECTSIZE, val); > + set_conf_val(OPT_L, L_SECTLOG, libxfs_highbit32(val)); > + break; > + } > + break; > + } Partly because this seems to be going opposite of the simplicity we were aiming for - before all this work, if we set teh data sector size, via any of these options, it got stored in a variable - "sectorsize". Now, if we issue "-s size=4k" the code will calculate and set that same value (or its log) into 7 to (or 9?) different internal variables? Why is that needed? There is/are only one (or two) sector size(s) in the filesystem, so should there not be one point of truth here? But also because the above is wrong; it is possible for the filesystem data portion and log portion to have different sector sizes, if the log is external. [1] The above would seem to break that, always setting data & log sizes together. On top of that, it's getting so complicated that it seems to be difficult to get right; i.e. this: > + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); > + set_conf_val(OPT_L, L_SECTSIZE, val); surely isn't correct. I found that when noticing that 1 block sets 9 vals, while the other only 7, and wondered why. So that accounts for one. After another minute of scrutiny I see that OPT_S, S_SECTSIZE isn't set in the 2nd chunk, so that's a bug as well. This makes me fear fragility in this approach. One goal of all this work, I thought, was to clearly describe interdependencies between options, but the above seems to add nasty, hidden, implicit, and wrong dependencies between log & data sector sizes (for example). If we have several commandline options that all set the same fundamental property, (i.e. data sector size), then it seems that should somehow be stored in one single point of truth within mkfs as it was before. -Eric [1] # mkfs.xfs -dfile,name=fsfile,size=1g,sectsize=4k -lfile,name=logfile,size=512m,sectsize=512 -- 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
On Wed, Aug 16, 2017 at 04:13:52PM -0500, Eric Sandeen wrote: > On 8/15/17 10:08 AM, Jan Tulak wrote: > > This patch adds infrastructure that will be used in subsequent patches. > > > > The Value field is the actual value used in computations for creating > > the filesystem. This is initialized with sensible default values. If > > initialized to 0, the value is considered disabled. User input will > > override default values. If the field is a string and not a number, the > > value is set to a positive non-zero number when user input has been > > supplied. > > > > Add functions that can be used to get/set values to opts table. > > Ok, I need to back up here a bit and look at this conceptually. > > (Again, though, if this is infra for some other patchset, I'd just send > it with /that/ patchset, not this one ...) > > But anyway, things like this confuse and worry me: > > > + case OPT_S: > > + switch (subopt) { > > + case S_LOG: > > + case S_SECTLOG: > > + set_conf_val(OPT_S, S_LOG, val); > > + set_conf_val(OPT_D, D_SECTLOG, val); > > + set_conf_val(OPT_L, L_SECTLOG, val); > > + set_conf_val(OPT_D, D_SECTSIZE, 1 << val); > > + set_conf_val(OPT_S, S_SIZE, 1 << val); > > + set_conf_val(OPT_S, S_SECTSIZE, 1 << val); > > + set_conf_val(OPT_L, L_SECTLOG, val); > > + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); > > + set_conf_val(OPT_L, L_SECTSIZE, val);> + break; > > + case S_SIZE: > > + case S_SECTSIZE: > > + set_conf_val(OPT_S, S_SIZE, val); > > + set_conf_val(OPT_D, D_SECTSIZE, val); > > + set_conf_val(OPT_D, D_SECTLOG, libxfs_highbit32(val)); > > + set_conf_val(OPT_S, S_LOG, libxfs_highbit32(val)); > > + set_conf_val(OPT_S, S_SECTLOG, libxfs_highbit32(val)); > > + set_conf_val(OPT_L, L_SECTSIZE, val); > > + set_conf_val(OPT_L, L_SECTLOG, libxfs_highbit32(val)); > > + break; > > + } > > + break; > > + } > > Partly because this seems to be going opposite of the simplicity > we were aiming for - before all this work, if we set teh data sector size, > via any of these options, it got stored in a variable - "sectorsize". > > Now, if we issue "-s size=4k" the code will calculate and set that same > value (or its log) into 7 to (or 9?) different internal variables? > Why is that needed? There is/are only one (or two) sector size(s) in the > filesystem, so should there not be one point of truth here? > > But also because the above is wrong; it is possible for the filesystem data > portion and log portion to have different sector sizes, if the log is external. [1] > The above would seem to break that, always setting data & log sizes together. > > On top of that, it's getting so complicated that it seems to be difficult to get > right; i.e. this: > > > + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); > > + set_conf_val(OPT_L, L_SECTSIZE, val); > > surely isn't correct. I found that when noticing that 1 block sets 9 vals, while > the other only 7, and wondered why. So that accounts for one. After another minute > of scrutiny I see that OPT_S, S_SECTSIZE isn't set in the 2nd chunk, so that's a bug > as well. > > This makes me fear fragility in this approach. > > One goal of all this work, I thought, was to clearly describe interdependencies between > options, but the above seems to add nasty, hidden, implicit, and wrong dependencies > between log & data sector sizes (for example). FWIW I thought all this really did was replace the dozens of local variables holding geometry info with a huge nested struct, which is a reasonable start on adding support for a config file (where is that, anyway?) but didn't make any functional changes. Ofc I didn't realize that xfs/191-input-validation isn't a totally thorough exerciser of all the mkfs options. > If we have several commandline options that all set the same fundamental property, > (i.e. data sector size), then it seems that should somehow be stored in one single > point of truth within mkfs as it was before. > > -Eric > > [1] # mkfs.xfs -dfile,name=fsfile,size=1g,sectsize=4k -lfile,name=logfile,size=512m,sectsize=512 I see -d sectsize is in the --help screen but not the manpage. Can we fix that? --D > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 16, 2017 at 11:38 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Wed, Aug 16, 2017 at 04:13:52PM -0500, Eric Sandeen wrote: >> On 8/15/17 10:08 AM, Jan Tulak wrote: >> > This patch adds infrastructure that will be used in subsequent patches. >> > >> > The Value field is the actual value used in computations for creating >> > the filesystem. This is initialized with sensible default values. If >> > initialized to 0, the value is considered disabled. User input will >> > override default values. If the field is a string and not a number, the >> > value is set to a positive non-zero number when user input has been >> > supplied. >> > >> > Add functions that can be used to get/set values to opts table. >> >> Ok, I need to back up here a bit and look at this conceptually. >> >> (Again, though, if this is infra for some other patchset, I'd just send >> it with /that/ patchset, not this one ...) Ehh... yes, of course. I moved the patch for which this is infrastructure to the other set but did not move this one. >> >> But anyway, things like this confuse and worry me: >> >> > + case OPT_S: >> > + switch (subopt) { >> > + case S_LOG: >> > + case S_SECTLOG: >> > + set_conf_val(OPT_S, S_LOG, val); >> > + set_conf_val(OPT_D, D_SECTLOG, val); >> > + set_conf_val(OPT_L, L_SECTLOG, val); >> > + set_conf_val(OPT_D, D_SECTSIZE, 1 << val); >> > + set_conf_val(OPT_S, S_SIZE, 1 << val); >> > + set_conf_val(OPT_S, S_SECTSIZE, 1 << val); >> > + set_conf_val(OPT_L, L_SECTLOG, val); >> > + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); >> > + set_conf_val(OPT_L, L_SECTSIZE, val);> + break; >> > + case S_SIZE: >> > + case S_SECTSIZE: >> > + set_conf_val(OPT_S, S_SIZE, val); >> > + set_conf_val(OPT_D, D_SECTSIZE, val); >> > + set_conf_val(OPT_D, D_SECTLOG, libxfs_highbit32(val)); >> > + set_conf_val(OPT_S, S_LOG, libxfs_highbit32(val)); >> > + set_conf_val(OPT_S, S_SECTLOG, libxfs_highbit32(val)); >> > + set_conf_val(OPT_L, L_SECTSIZE, val); >> > + set_conf_val(OPT_L, L_SECTLOG, libxfs_highbit32(val)); >> > + break; >> > + } >> > + break; >> > + } >> >> Partly because this seems to be going opposite of the simplicity >> we were aiming for - before all this work, if we set teh data sector size, >> via any of these options, it got stored in a variable - "sectorsize". >> >> Now, if we issue "-s size=4k" the code will calculate and set that same >> value (or its log) into 7 to (or 9?) different internal variables? >> Why is that needed? There is/are only one (or two) sector size(s) in the >> filesystem, so should there not be one point of truth here? >> >> But also because the above is wrong; it is possible for the filesystem data >> portion and log portion to have different sector sizes, if the log is external. [1] >> The above would seem to break that, always setting data & log sizes together. But in the current for-next tree mkfs does set data and log sector size at the same time, this patch is not doing anything new: case S_SECTSIZE: if (lslflag) conflict('s', subopts, S_SECTLOG, S_SECTSIZE); sectorsize = getnum(value, &sopts, S_SECTSIZE); lsectorsize = sectorsize; sectorlog = libxfs_highbit32(sectorsize); lsectorlog = sectorlog; lssflag = ssflag = 1; break; The only difference here is that sectorsize was one variable into which multiple options assigned a value, while now we fill every related member of the structure with the same value. Which is is cumbersome, I admit that. I had an idea which could limit this duplicated assignments, but it has its own disadvantages: if the values of -d sectlog and sectize and -s size and log options were all pointers to the same variable, it would be enough to set only one. But then we are unknowingly modifying the other options whenever we touch any of those, which is not nice as well. But I will give it another though, maybe I will came up with an idea... Anyway, back to the data/log sector size - when I was making this change, my understanding of the code was that -s size will set both data and log sector size, and -d/-l sectsize will set only one of them. If this behaviour is not the desired one, then it needs to be fixed anyway, no matter if my patch is there or not. But it is perhaps obscured by the fact that here I'm setting both S_SIZE and D_SECTSIZE, which adds a noise. >> >> On top of that, it's getting so complicated that it seems to be difficult to get >> right; i.e. this: >> >> > + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); >> > + set_conf_val(OPT_L, L_SECTSIZE, val); >> >> surely isn't correct. I found that when noticing that 1 block sets 9 vals, while >> the other only 7, and wondered why. So that accounts for one. After another minute >> of scrutiny I see that OPT_S, S_SECTSIZE isn't set in the 2nd chunk, so that's a bug >> as well. Yup, these are bugs. >> >> This makes me fear fragility in this approach. >> >> One goal of all this work, I thought, was to clearly describe interdependencies between >> options, but the above seems to add nasty, hidden, implicit, and wrong dependencies >> between log & data sector sizes (for example). The fragility is the reason why I moved it out of the main() loop where these correlated options were set until now into this specific function, so at least we know what to watch for. But the dependency between log & data sector sizes was there before. The issue is that S_SIZE and D_SECTSIZE have the same meaning, but are not automatically connected. > > FWIW I thought all this really did was replace the dozens of local > variables holding geometry info with a huge nested struct, which is a > reasonable start on adding support for a config file (where is that, > anyway?) but didn't make any functional changes. That is the goal, yes. Any functional changes in this are a bug. > > Ofc I didn't realize that xfs/191-input-validation isn't a totally > thorough exerciser of all the mkfs options. Missing combinations can be added, but it only tests whether mkfs accepts or refuses the combination. I have another test in the making, which really makes the filesystem and then tests if its geometry and properties are all right, but I have difficulties with evaluating correct sizes when the test device can be of any size and have different sector size, so I can't diff a correct output from xfs_info. The only usable way I found so far is to limit it to fs images only, where I have some control over the size. > >> If we have several commandline options that all set the same fundamental property, >> (i.e. data sector size), then it seems that should somehow be stored in one single >> point of truth within mkfs as it was before. >> >> -Eric >> >> [1] # mkfs.xfs -dfile,name=fsfile,size=1g,sectsize=4k -lfile,name=logfile,size=512m,sectsize=512 > > I see -d sectsize is in the --help screen but not the manpage. Can we > fix that? I made it, but Dave would rather see the -d sectsize option removed. Which I'm not sure about... See "[PATCH] xfsprogs: add sectsize/sectlog to the man page" Cheers, Jan -- 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
On 8/17/17 5:08 AM, Jan Tulak wrote: >>> But also because the above is wrong; it is possible for the filesystem data >>> portion and log portion to have different sector sizes, if the log is external. [1] >>> The above would seem to break that, always setting data & log sizes together. > But in the current for-next tree mkfs does set data and log sector > size at the same time, > this patch is not doing anything new Yes, you are right about this - I lost my train of thought on that one, sorry. -Eric -- 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
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 1aedc804..17ee46a7 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -117,6 +117,13 @@ unsigned int sectorsize; * Filled raw string from the user, so we never lose that information e.g. * to print it back in case of an issue. * + * value OPTIONAL + * The actual value used in computations for creating the filesystem. + * This is initialized with sensible default values. If initialized to 0, + * the value is considered disabled. User input will override default + * values. If the field is a string and not a number, the value is set to + * a positive non-zero number when user input has been supplied. + * */ struct opt_params { int index; @@ -133,6 +140,7 @@ struct opt_params { long long minval; long long maxval; long long flagval; + long long value; char *raw_input; } subopt_params[MAX_SUBOPTS]; } opts[MAX_OPTS] = { @@ -750,6 +758,162 @@ struct opt_params { #define WHACK_SIZE (128 * 1024) /* + * Get and set values to the opts table. + */ +static int +get_conf_val_unsafe(int opt, int subopt, uint64_t *output) +{ + if (subopt < 0 || subopt >= MAX_SUBOPTS || + opt < 0 || opt >= MAX_OPTS) { + fprintf(stderr, + _("This is a bug: get_conf_val called with invalid " + "opt/subopt: %d/%d\n"), + opt, subopt); + return -EINVAL; + } + *output = opts[opt].subopt_params[subopt].value; + return 0; +} + +static long long +get_conf_val(int opt, int subopt) +{ + uint64_t res; + if (get_conf_val_unsafe(opt, subopt, &res) != 0) + exit(1); + return res; +} + +static int +set_conf_val_unsafe(int opt, int subopt, uint64_t val) +{ + if (subopt < 0 || subopt >= MAX_SUBOPTS || + opt < 0 || opt >= MAX_OPTS) { + fprintf(stderr, + _("This is a bug: set_conf_val called with invalid " + "opt/subopt: %d/%d\n"), + opt, subopt); + return -EINVAL; + } + struct subopt_param *sp = &opts[opt].subopt_params[subopt]; + sp->value = val; + + return 0; +} + +static void +set_conf_val(int opt, int subopt, uint64_t val) +{ + if (set_conf_val_unsafe(opt, subopt, val) != 0) + exit(1); +} + +/* + * A wrapper around set_conf_val which updates also connected values. + * E.g. when changing S_LOG, update S_SIZE too. + */ +static void +set_conf_val_connected(int opt, int subopt, uint64_t val) +{ + set_conf_val(opt, subopt, val); + + switch (opt) { + case OPT_B: + switch (subopt) { + case B_LOG: + set_conf_val(OPT_B, B_SIZE, 1 << val); + break; + case B_SIZE: + set_conf_val(OPT_B, B_LOG, libxfs_highbit32(val)); + break; + } + break; + case OPT_D: + switch (subopt) { + case D_SECTLOG: + set_conf_val(OPT_S, S_LOG, val); + set_conf_val(OPT_S, S_SECTLOG, val); + set_conf_val(OPT_D, D_SECTSIZE, 1 << val); + set_conf_val(OPT_S, S_SECTSIZE, 1 << val); + set_conf_val(OPT_S, S_SIZE, 1 << val); + break; + case D_SECTSIZE: + set_conf_val(OPT_S, S_SIZE, val); + set_conf_val(OPT_S, S_SECTSIZE, val); + set_conf_val(OPT_D, D_SECTLOG, libxfs_highbit32(val)); + set_conf_val(OPT_S, S_LOG, libxfs_highbit32(val)); + set_conf_val(OPT_S, S_SECTLOG, libxfs_highbit32(val)); + break; + } + break; + case OPT_I: + switch (subopt) { + case I_LOG: + set_conf_val(OPT_I, I_SIZE, 1 << val); + break; + case I_SIZE: + set_conf_val(OPT_I, I_LOG, libxfs_highbit32(val)); + break; + } + break; + case OPT_L: + switch (subopt) { + case L_SECTLOG: + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); + break; + case L_SECTSIZE: + set_conf_val(OPT_L, L_SECTLOG, libxfs_highbit32(val)); + break; + } + break; + case OPT_M: + switch (subopt) { + } + break; + case OPT_N: + switch (subopt) { + case N_LOG: + set_conf_val(OPT_N, N_SIZE, 1 << val); + break; + case N_SIZE: + set_conf_val(OPT_N, N_LOG, libxfs_highbit32(val)); + break; + } + break; + case OPT_R: + switch (subopt) { + } + break; + case OPT_S: + switch (subopt) { + case S_LOG: + case S_SECTLOG: + set_conf_val(OPT_S, S_LOG, val); + set_conf_val(OPT_D, D_SECTLOG, val); + set_conf_val(OPT_L, L_SECTLOG, val); + set_conf_val(OPT_D, D_SECTSIZE, 1 << val); + set_conf_val(OPT_S, S_SIZE, 1 << val); + set_conf_val(OPT_S, S_SECTSIZE, 1 << val); + set_conf_val(OPT_L, L_SECTLOG, val); + set_conf_val(OPT_L, L_SECTSIZE, 1 << val); + set_conf_val(OPT_L, L_SECTSIZE, val); + break; + case S_SIZE: + case S_SECTSIZE: + set_conf_val(OPT_S, S_SIZE, val); + set_conf_val(OPT_D, D_SECTSIZE, val); + set_conf_val(OPT_D, D_SECTLOG, libxfs_highbit32(val)); + set_conf_val(OPT_S, S_LOG, libxfs_highbit32(val)); + set_conf_val(OPT_S, S_SECTLOG, libxfs_highbit32(val)); + set_conf_val(OPT_L, L_SECTSIZE, val); + set_conf_val(OPT_L, L_SECTLOG, libxfs_highbit32(val)); + break; + } + break; + } +} + +/* * Return 0 on success, -ENOMEM if it could not allocate enough memory for * the string to be saved. */ @@ -951,6 +1115,18 @@ getnum( } /* + * A wrapper for getnum and set_conf_val. + */ +static inline long long +parse_conf_val(int opt, int subopt, char *value) +{ + long long num = getnum(value, &opts[opt], subopt); + + set_conf_val_connected(opt, subopt, num); + return num; +} + +/* * Convert lsu to lsunit for 512 bytes blocks and check validity of the values. */ static void
This patch adds infrastructure that will be used in subsequent patches. The Value field is the actual value used in computations for creating the filesystem. This is initialized with sensible default values. If initialized to 0, the value is considered disabled. User input will override default values. If the field is a string and not a number, the value is set to a positive non-zero number when user input has been supplied. Add functions that can be used to get/set values to opts table. Signed-off-by: Jan Tulak <jtulak@redhat.com> --- Change: * whitespaces * rename "collateral" -> "connected" * reorder subopt_params to avoid padding issues * translation and indentation for fprintf --- mkfs/xfs_mkfs.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+)