On 10/8/17 7:42 PM, Dave Chinner wrote: > On Fri, Oct 06, 2017 at 01:01:39PM -0500, Eric Sandeen wrote: >> On 10/3/17 3:06 AM, Dave Chinner wrote: >>> Hi Eric, >>> >>> I've put the latest mkfs refactor code that I have up in place >>> you can pull it from. I've rebased it against the current for-next >>> tree (4.13.1 release) and fixed all the problems that xfstests >>> exposes. The only thing I haven't fixed is xfs/191 that does mkfs >>> command line behaviour verification because the refactored version >>> fixes several problems that the old mkfs didn't handle correctly >>> (e.g. being able to specify certain things like agsize in blocks or >>> sectors). >>> >>> There's a small filter patch needed for xfstests that I'll post in >>> a reply to this pull request that will filter out the new "defaults >>> sourced from ..." output and so prevent spurious xfstests failures. >>> >>> If you want I can tag the branch with a signed tag for you to pull >>> from (same process as Linus prefers) rather than just a branch in a >>> tree. If you'd prefer that I post this as patches instead, then let >>> me know and I'll bomb the list instead. >> >> Thanks Dave. Trying to un-swamp from other things. >> >> Only advantage to patchbombing is having a permanent record of discussions, >> but maybe it can happen in reference to patches as follows.... mostly >> minor nits. > > Yup, but when it's been posted before, multiple ppl have said > "tested ok" but nobody is looking like they are going to review it, > I'm going to send a pull request rather than another patchbomb. :/ That's fine. I've been swamped, sorry. (OTOH it never stopped djwong) ;) >> >>> mkfs: can't specify sector size of internal log >> >> commit log is a bit vague/misleading; I think you mean that >> it should be /disallowed/ because it must match the filesystem's >> /sector/ size when it's internal? It's not actually clear to me why >> an external log can have a "mismatched" sector size but internal can't. > > I thought that was obvious. If we have a 512 byte data device (e.g. > HDD) but a 4k sector external log device (e.g. pmem) then we'll have > the situation where the log device requires a different sector size > to the data device. And we allow this, because the on disk log > format is only dependent on the underlying sector size, not the > configuration of the filesystem. > > However, if the log is on the data device (i.e. internal) then it > should match the sector size the data device is using, otherwise > one or the other doesn't have atomic sector writes. Not to mention > that it's simply wrong to have two different sector sizes for the > same device. Presumably with a 512e drive, you /could/ have 512 vs 4k on fs vs log ... but sure, it's not a good idea. But that wasn't really my main point. > If you need to change the write size (i.e. padding) characteristics > of the log for an internal/external, then use the log stripe unit > because that's exactly what it does... So anyway, I was being a bit pedantic; my main concern is with the wording.. Don't say "can't specify," say "disallow specification" - that was my main point about the changelog. i.e. instead of: mkfs: can't specify sector size of internal log Because it's fixed by the underlying device size. say: mkfs: disallow specification of sector size of internal log To guarantee atomic writes, the log sector size must match the underlying device's advertised sector size. which IMHO more clearly says what you're doing and why. >>> mkfs: introduce a structure to hold CLI options >> >> "between th einput" (the input) >> >> Comment mentions initialization to -1 or 0 but I don't see that this >> gets done, at least not in this patch? (Am I missing it?) I took >> this to mean that before anything happens this would be initialized >> so we know what was and was not specified after everything was parsed... > > Stale comment, updated. We initialise it all to zero, if we need > flags to see if the option was set we look at the option table > "seen" value. > >>> mkfs: introduce a structure to hold CLI options >> >> +/* quick way of checking is a parameter was set on the CLI */ >> "if" >> >>> mkfs: factor naming subopts parser >> >> no "nvflag" in "temp don't break" code? Oh, nice - write-only var. Ok. >> do we still test it for respec, or care? Will check later. > > I was only making the code build with the temp code. Stuff like > crappy write-only vars got dropped at the first point they were > found to be crap. > > And, FYI, with code that does this: > > nvflag = nlflag = foo = bar = 0; > > gcc does not report set-but-unused or other such warnings because > it seems to stop checking the moment there's a multiple assignment > statement like the above. Hence shit code like in mkfs basically > prevented gcc from warning about how the shit code was dead code.... Yeah, and I confirmed that respecs still get caught, so that's fine. I was wondering out loud. > >>> mkfs: Introduce mkfs configuration structure >> >> would be nice to proofread commit log a bit ;) > > Seriously, after several hours of tedious, boring patch splitting > I only cared that the code was correct. Commit logs were write once, > read never. :/ I do appreciate all those tedious hours. > Fixed. Thanks. I only mentioned the ones that look like the dog might have helped write them ;) >>> mkfs: Introduce mkfs configuration structure >> >> s/teh/the/ >> >> <ok stopping here for now, will send more later> > > Thanks! "You're welcome." ;) Will try to get through more soon. We'll get this into the next point release, I'm sure. -Eric -- 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
