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.

Sorry to be slow to comment. Like D45624, this change looks functionally correct. Again, the main issue for me is to understand what sort of workload it helps. Do you have an example benchmark whose performance is improved with this change?

Sorry to be slow to comment. This change looks functionally correct. The main issue for me is to understand what sort of workload it helps. Do you have an example benchmark whose performance is improved with this change?

In D45987#1051603, @olce wrote:

In D45987#1051237, @mckusick wrote:

I do not remember why the change to allow the removing of directories with whiteouts became allowed. The changes in commit 996c772f581f56248 are quite extensive. If you can let me know which file(s) in that commit have the relevant changes, I can go back through the changes made in those files for the Lite/2 release to see if any explanation shows up.

Sure, I'm referring to:

--- a/sys/ufs/ufs/ufs_lookup.c
+++ b/sys/ufs/ufs/ufs_lookup.c
@@ -906,7 +961,7 @@ ufs_dirempty(ip, parentino, cred)
                if (dp->d_reclen == 0)
                        return (0);
                /* skip empty entries */
-               if (dp->d_ino == 0)
+               if (dp->d_ino == 0 || dp->d_ino == WINO)
                        continue;
                /* accept only "." and ".." */
 #              if (BYTE_ORDER == LITTLE_ENDIAN)

part of function ufs_dirempty(), which is (now, but if I'm not mistaken also was then) called only by ufs_rename() and ufs_rmdir().

Barring other reasons I can't see now, I think we'd better revert that behavior, both for logical reasons (the directory references no file but still has whiteout entries that we are going to lose silently) and practical ones (e.g., on Linux, whiteouts are implemented as device files, and thus rmdir() on directories holding some will fail with ENOTEMPTY; this is also to ease consistency in ZFS' code).

I suspect that removing a directory with whiteouts was allowed for UFS only because not doing so would confuse most users not aware of whiteouts, as these are not printed by a regular ls. I doubt there was any other reason than that. It appears the test on WINO wasn't in 4.4BSD and was introduced in the Lite/2 commit (996c772f581f56248). Adding @mckusick as he might have some recollection of what lead to this decision.

I concur that this seems like a reasonable optimization.

Useful abstraction of internal state.

Better to avoid use of internal lock state.

This looks like the correct fix. One other unrelated nit.