Respond to latest comments.

Get rid of unneeded else statement.

Add comment on why it is not necessary to check for NULL return from prependdevtopath().

Responding to Dag-Erling's comments.

Respond to Dag-Erling's comments.

This looks good to me, but I will await a review from Johan before proceeding.

I will put in this change to fix the tests.
Pleas start a new review to address the additional suggestions.

Indeed this functionality is now embodied in ffs_search.

Fix is correct. Clearly a cut & paste blunder when I wrote it.

In D37615#1212734, @guest-jsollvander wrote:

In D37615#1212613, @guest-svmhdvn wrote:

This patch seems to break several testcases in the sys/geom/class/multipath/misc test suite with this output:

===> sys/geom/class/multipath/misc:add
Result:     failed: ACTIVE !=  (ACTIVE != )

I'll take a look.

I note that the sshd daemon is restarted when it is reinstalled. The sshd daemon is restarted since its update may be security related and thus the security fix should take effect as soon as the updated version is available.

First, sorry for letting this languish. Though I have not dealt with libxo, I assume that you have ensured that this generates the output that you need. Structurally it is properly added to the geom utility. If there are no issues raised by the other reviewers by Monday October 13th, I will commit it.

Mentor approval.

Mentor approval.

Looks ready to go.

Interesting history of this bug.

This has been committed to head and MFC'ed to 14.

Mentor approval.

Mentor approval.

Mentor approval.

Mentor approval for commit.

Mentor approval.

Mentor approval - looks good to go.

Mentor approval for commit.

Mentor approval for commit.

I am always in favor of improvement to code clarity.

Mentor approval for commit.

I concur with @imp comment that I like the notion of abstracting them so we don't churn that too much...

This is (still) a sensible thing to do.

With Mark's review and approval this looks good to go.

This change is correct.

Review process looks good and complete.
Adding my accept as your mentor.

Review process looks good and complete.
Adding my accept as your mentor.

Review process looks good and complete.
Adding my accept as your mentor.

Review process is complete with all the right folks signing off.

I suggest that you add 'Sponsored by: The FreeBSD Foundation' to your commit as it helps show that they are supporting your work.

I too suggest that you add 'Sponsored by: The FreeBSD Foundation' to your commit as it helps show that they are supporting your work.

Looks good.

This seems to be a reasonable change.

Agree with change.

One grammer nit.

Sorry for being out of the loop here, I just got back from a trip to the arctic where unsurprisingly there was little to no Internet.

In D48374#1145383, @emaste wrote:

There's another set of questions:

• Is SU+J detrimental under certain conditions?
• Should we be asking the user this question in the installer?

It's the second question that prompted this review. I don't care too much about whether SU+J is always on by default, always off by default, or on by default iff the fs is on spinning rust (or some equivalent). I do want to remove cognitive load from installer questions that aren't really required.

This has been fixed in newfs for some time. Along with numerous other changes like checkhash validations, superblock integrity checks, and other fixes and improvements. In my opinion makefs/ffs should just use use newfs/mkfs.c rather than (poorly) trying to parody it.

Clarify when the superblock is a copy and fix yet another Peter Holm test failure.

Update with kib comments.
Add fixes for bugs found by Peter Holm.

This has been committed to head and MFC'ed to 14 and 13. It was committed to 13 in time to be part of the 13.5 distribution.

I have accepted gleb's changes, so this issue is resolved.

I have gone with Gleb's fix. It is simple and as Warner has pointed out `It's the least bad outcome. We are doing ffs things, even if they are minimal.' As I have pointed out, your change breaks libufs and would require many additional changes after making the needed fixes in libufs to the clients of libufs.

Actually, there is an even easier fix. Just move the declaration of vfs_ffs from ffs_alloc.c to ffs_subr.c. Then everything just works.

This will cause libufs to fail to build. Also, ffs_subr.c is supposed to contain all the kernel functionality needed by filesystem utilities. And ffs_oldfscompat_inode_read() is one of those functions.

Updates to respond to reviewer feedback,

I will follow up these comments with a new set of diffs reflecting suggested changes.

Committed as 661ca921e8cd56b17fc6615bc7e596e56e0e7c31

These changes look correct to me. I am surprised that it has taken this long to trip over this error case.

I agree that -h changes the format of all numbers, So it should just be "x MB".

I largely agree with phk's analysis. SU+J does more writing than just SU (the extra writes being the journal data). Fsck runs very quickly on SSDs since random seeks do not slow it down as they do spinning rust. It would be worthwhile to run a timing test of fsck -p on a large SSD to see if it takes more than say 30 seconds but my guess is that it will not.

Sorry, late to comment.