[GIT PULL] xfsprogs: mkfs refactor

[GIT PULL] xfsprogs: mkfs refactor

[Dave Chinner]

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.

Cheers,

Dave.

The following changes since commit d4a36331dc383c7c7747e244b3ae20155ae92c98:

  xfsprogs: Release v4.13.1 (2017-09-26 20:45:05 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dgc/xfsprogs-dev mkfs-refactor

for you to fetch changes up to a4bc6d3c7bb5babc51f7341039dafcff5fcc6c7e:

  mkfs: tidy up definitions (2017-09-29 08:44:30 +1000)

----------------------------------------------------------------
Dave Chinner (42):
      mkfs: can't specify sector size of internal log
      mkfs: make subopt table const
      mkfs: introduce a structure to hold CLI options
      mkfs: add generic subopt parsing table
      mkfs: factor block subopts parser
      mkfs: factor data subopts parser
      mkfs: factor inode subopts parser
      mkfs: factor log subopts parser
      mkfs: factor meta subopts parser
      mkfs: factor naming subopts parser
      mkfs: factor rt subopts parser
      mkfs: factor sector subopts parser
      mkfs: Introduce mkfs configuration structure
      mkfs: factor printing of mkfs config
      mkfs: factor in memory superblock setup
      mkfs: factor out device preparation
      mkfs: factor writing AG headers
      mkfs: factor secondary superblock updates
      mkfs: introduce default configuration structure
      mkfs: rename top level CLI parameters
      mkfs: factor sectorsize validation
      mkfs: factor blocksize validation
      mkfs: factor log sector size validation
      mkfs: factor superblock feature validation
      mkfs: factor directory blocksize validation
      mkfs: factor inode size validation
      mkfs: factor out device size calculations
      mkfs: fix hidden parameter in DTOBT()
      mkfs: factor rtdev extent size validation
      mkfs: rework stripe calculations
      mkfs: factor device opening
      mkfs: factor data device validation
      mkfs: factor log device validation
      mkfs: factor rt device validation
      mkfs: factor AG geometry calculations
      mkfs: factor AG alignment
      mkfs: rework imaxpct calculation
      mkfs: factor initial mount setup
      mkfs: factor log size calculations
      mkfs: cleanup redundant temporary code
      mkfs: move error functions
      mkfs: tidy up definitions

 include/libxfs.h |    2 +-
 mkfs/xfs_mkfs.c  | 4645 ++++++++++++++++++++++++++++++------------------------
 2 files changed, 2602 insertions(+), 2045 deletions(-)
-- 
Dave Chinner
david@fromorbit.com

fstests: update mkfs.xfs filters for new refactoring

[Dave Chinner]

fstests: update mkfs.xfs filters for new refactoring

From: Dave Chinner <dchinner@redhat.com>

The new mkfs code adds some output to indicate where the defaults
were sourced from, so filter that out so it doesn't contaminate
tests unnecessarily.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 common/xfs    | 3 ++-
 tests/xfs/206 | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/xfs b/common/xfs
index def28bfc7309..603792092060 100644
--- a/common/xfs
+++ b/common/xfs
@@ -81,7 +81,8 @@ _scratch_mkfs_xfs()
 {
 	local mkfs_cmd="`_scratch_mkfs_xfs_opts`"
 	local mkfs_filter="sed -e '/less than device physical sector/d' \
-			       -e '/switching to logical sector/d'"
+			       -e '/switching to logical sector/d' \
+			       -e '/Default configuration/d'"
 	local tmp=`mktemp -u`
 	local mkfs_status
 
diff --git a/tests/xfs/206 b/tests/xfs/206
index 70997e3fe83e..01782b7b93a9 100755
--- a/tests/xfs/206
+++ b/tests/xfs/206
@@ -84,7 +84,8 @@ mkfs_filter()
 	    -e "s/\(sectsz\)\(=[0-9]* *\)/\1=512   /" \
 	    -e "s/\(sunit=\)\([0-9]* blks,\)/\10 blks,/" \
 	    -e "s/, lazy-count=[0-9]//" \
-	    -e "/.*crc=/d"
+	    -e "/.*crc=/d" \
+	    -e "/^Default configuration/d"
 }
 
 # mkfs slightly smaller than that, small log for speed.

Re: [GIT PULL] xfsprogs: mkfs refactor

[Darrick J. Wong]

On Tue, Oct 03, 2017 at 07:06:07PM +1100, 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.

I had a look at mkfs-refactor.  It looks ok to me (I defer to Eric on
the question of pull req. vs. patchbomb) though I have one question:

calculate_log_size calls max_trans_res, and max_trans_res assembles a
fake struct xfs_mount in order to call libxfs_log_calc_minimum_size.
I've fixed a few mkfs bugs over the past couple of years that all stem
from us forgetting to propagate superblock settings from the
configuration we're building in main() into the fake xfs_mount->m_sb
that we use to calculate the minimum log size, which results in a
disagreement between the kernel and mkfs as to what is the minimum log
size for a given fs configuration.  This disagreement pops up in the
form of a freshly mkfs'd 500MB filesystem immediately failing to mount.

With this branch applied it looks like we've nearly finished filling out
the real xfs_mount->m_sb when we call calculate_log_size, so could we
refactor setup_superblock to set all the non-log superblock fields in
the real m_sb and then pass that directly into max_trans_res so that we
can memcpy the real superblock settings into the fake struct xfs_mount?

Doing that will eliminate a whole class of "we forgot that we have to
set sb_newfield in setup_superblock /and/ in max_trans_res and now mkfs
creates broken filesystems" bugs.  Even now there are small
discrepancies between (for example) tr_itruncate.tr_logres in the kernel
and in mkfs, which make me nervous.  AFAICT the discrepancies result in
mkfs using a minimum log size that is larger than what the kernel
calculates, so there's no user-visible badness.

--D

> 
> Cheers,
> 
> Dave.
> 
> The following changes since commit d4a36331dc383c7c7747e244b3ae20155ae92c98:
> 
>   xfsprogs: Release v4.13.1 (2017-09-26 20:45:05 -0500)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/dgc/xfsprogs-dev mkfs-refactor
> 
> for you to fetch changes up to a4bc6d3c7bb5babc51f7341039dafcff5fcc6c7e:
> 
>   mkfs: tidy up definitions (2017-09-29 08:44:30 +1000)
> 
> ----------------------------------------------------------------
> Dave Chinner (42):
>       mkfs: can't specify sector size of internal log
>       mkfs: make subopt table const
>       mkfs: introduce a structure to hold CLI options
>       mkfs: add generic subopt parsing table
>       mkfs: factor block subopts parser
>       mkfs: factor data subopts parser
>       mkfs: factor inode subopts parser
>       mkfs: factor log subopts parser
>       mkfs: factor meta subopts parser
>       mkfs: factor naming subopts parser
>       mkfs: factor rt subopts parser
>       mkfs: factor sector subopts parser
>       mkfs: Introduce mkfs configuration structure
>       mkfs: factor printing of mkfs config
>       mkfs: factor in memory superblock setup
>       mkfs: factor out device preparation
>       mkfs: factor writing AG headers
>       mkfs: factor secondary superblock updates
>       mkfs: introduce default configuration structure
>       mkfs: rename top level CLI parameters
>       mkfs: factor sectorsize validation
>       mkfs: factor blocksize validation
>       mkfs: factor log sector size validation
>       mkfs: factor superblock feature validation
>       mkfs: factor directory blocksize validation
>       mkfs: factor inode size validation
>       mkfs: factor out device size calculations
>       mkfs: fix hidden parameter in DTOBT()
>       mkfs: factor rtdev extent size validation
>       mkfs: rework stripe calculations
>       mkfs: factor device opening
>       mkfs: factor data device validation
>       mkfs: factor log device validation
>       mkfs: factor rt device validation
>       mkfs: factor AG geometry calculations
>       mkfs: factor AG alignment
>       mkfs: rework imaxpct calculation
>       mkfs: factor initial mount setup
>       mkfs: factor log size calculations
>       mkfs: cleanup redundant temporary code
>       mkfs: move error functions
>       mkfs: tidy up definitions
> 
>  include/libxfs.h |    2 +-
>  mkfs/xfs_mkfs.c  | 4645 ++++++++++++++++++++++++++++++------------------------
>  2 files changed, 2602 insertions(+), 2045 deletions(-)
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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

Re: [GIT PULL] xfsprogs: mkfs refactor

[Dave Chinner]

On Tue, Oct 03, 2017 at 10:16:04AM -0700, Darrick J. Wong wrote:
> On Tue, Oct 03, 2017 at 07:06:07PM +1100, 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.
> 
> I had a look at mkfs-refactor.  It looks ok to me (I defer to Eric on
> the question of pull req. vs. patchbomb) though I have one question:
> 
> calculate_log_size calls max_trans_res, and max_trans_res assembles a
> fake struct xfs_mount in order to call libxfs_log_calc_minimum_size.
> I've fixed a few mkfs bugs over the past couple of years that all stem
> from us forgetting to propagate superblock settings from the
> configuration we're building in main() into the fake xfs_mount->m_sb
> that we use to calculate the minimum log size, which results in a
> disagreement between the kernel and mkfs as to what is the minimum log
> size for a given fs configuration.  This disagreement pops up in the
> form of a freshly mkfs'd 500MB filesystem immediately failing to mount.
> 
> With this branch applied it looks like we've nearly finished filling out
> the real xfs_mount->m_sb when we call calculate_log_size, so could we
> refactor setup_superblock to set all the non-log superblock fields in
> the real m_sb and then pass that directly into max_trans_res so that we
> can memcpy the real superblock settings into the fake struct xfs_mount?

Yes, I plan on making further cleanups like that. There are a few
others, like the remaining uses of the global block size and sector
size variables because the parameter structures are not fed into
number conversion functions that use them.

> Doing that will eliminate a whole class of "we forgot that we have to
> set sb_newfield in setup_superblock /and/ in max_trans_res and now mkfs
> creates broken filesystems" bugs.  Even now there are small
> discrepancies between (for example) tr_itruncate.tr_logres in the kernel
> and in mkfs, which make me nervous.  AFAICT the discrepancies result in
> mkfs using a minimum log size that is larger than what the kernel
> calculates, so there's no user-visible badness.

Getting rid of the max_trans_res problem will be good, but it won't
completely fix the problem up.  Other nasties in this area that need
further cleanup is the units that stripe configuration are passed
around in, when we store the log stripe unit into the superblock,
documenting what the values in the sb variable are supposed to be,
how sector size and blocksize affects LSU, etc. These were all
things I tripped over that led to similar "why doesn't this
filesystem mount/crash log recovery?" issues.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [GIT PULL] xfsprogs: mkfs refactor

[Darrick J. Wong]

On Wed, Oct 04, 2017 at 07:07:26AM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 10:16:04AM -0700, Darrick J. Wong wrote:
> > On Tue, Oct 03, 2017 at 07:06:07PM +1100, 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.
> > 
> > I had a look at mkfs-refactor.  It looks ok to me (I defer to Eric on
> > the question of pull req. vs. patchbomb) though I have one question:
> > 
> > calculate_log_size calls max_trans_res, and max_trans_res assembles a
> > fake struct xfs_mount in order to call libxfs_log_calc_minimum_size.
> > I've fixed a few mkfs bugs over the past couple of years that all stem
> > from us forgetting to propagate superblock settings from the
> > configuration we're building in main() into the fake xfs_mount->m_sb
> > that we use to calculate the minimum log size, which results in a
> > disagreement between the kernel and mkfs as to what is the minimum log
> > size for a given fs configuration.  This disagreement pops up in the
> > form of a freshly mkfs'd 500MB filesystem immediately failing to mount.
> > 
> > With this branch applied it looks like we've nearly finished filling out
> > the real xfs_mount->m_sb when we call calculate_log_size, so could we
> > refactor setup_superblock to set all the non-log superblock fields in
> > the real m_sb and then pass that directly into max_trans_res so that we
> > can memcpy the real superblock settings into the fake struct xfs_mount?
> 
> Yes, I plan on making further cleanups like that. There are a few
> others, like the remaining uses of the global block size and sector
> size variables because the parameter structures are not fed into
> number conversion functions that use them.
> 
> > Doing that will eliminate a whole class of "we forgot that we have to
> > set sb_newfield in setup_superblock /and/ in max_trans_res and now mkfs
> > creates broken filesystems" bugs.  Even now there are small
> > discrepancies between (for example) tr_itruncate.tr_logres in the kernel
> > and in mkfs, which make me nervous.  AFAICT the discrepancies result in
> > mkfs using a minimum log size that is larger than what the kernel
> > calculates, so there's no user-visible badness.
> 
> Getting rid of the max_trans_res problem will be good, but it won't
> completely fix the problem up.  Other nasties in this area that need
> further cleanup is the units that stripe configuration are passed
> around in, when we store the log stripe unit into the superblock,
> documenting what the values in the sb variable are supposed to be,
> how sector size and blocksize affects LSU, etc. These were all
> things I tripped over that led to similar "why doesn't this
> filesystem mount/crash log recovery?" issues.

Ok, just making sure it was on your radar. :)

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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

Re: [GIT PULL] xfsprogs: mkfs refactor

[Eric Sandeen]

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.

> 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.

> 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...

> 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.

> mkfs: Introduce mkfs configuration structure

would be nice to proofread commit log a bit ;)

> mkfs: Introduce mkfs configuration structure

s/teh/the/

<ok stopping here for now, will send more later>

Re: [GIT PULL] xfsprogs: mkfs refactor

[Eric Sandeen]

On 10/6/17 1:01 PM, Eric Sandeen wrote:
> On 10/3/17 3:06 AM, Dave Chinner wrote:


>> 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.
> 

Whoops meant to check it sooner than later.  Looks like respec is handled
fine and nvflag was indeed extraneous.

Thanks,
-Eric

Re: [GIT PULL] xfsprogs: mkfs refactor

[Dave Chinner]

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. :/

> 
> > 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.

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...

> > 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....


> > 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. :/

Fixed.

> > mkfs: Introduce mkfs configuration structure
> 
> s/teh/the/
> 
> <ok stopping here for now, will send more later>

Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [GIT PULL] xfsprogs: mkfs refactor

[Eric Sandeen]

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