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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html