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.

In general b_lblkno is for the use of the client of the buffer cache to use for whatever purpose they want. The b_blkno is what the underlying hardware is going to use to locate the requested data. The filesystem generally uses its BMAP routine to convert the b_lblkno to the appropriate b_blkno.

In D38789#883134, @fbsd_paepcke.de wrote:

[...] It would be possible to add another flag to the existing um_flags word with the semantic that the filesystem can be mounted only as read-only (UM_READONLY). Of course it would still be possible for the superuser to clear this flag but it takes slightly more effort.

Yes, that is a great approach!
I will look into it / make a proposal.

In a second step, we could guard the UM_READONLY flag via the
existing FFS superblock checksum logic in way that fsck will detect
and refuse to auto-repair a manual flag clean attempt.

Thank you!

Relevant parts of these changes were incorporated in commit 772430dd6795585 as discussed in PR 267654 and review https://reviews.freebsd.org/D41724.

This does fix the problem as described in the summary.

If I understand your change, you are coming up with a different synthetic inode value to ensure that it will be unique. Presumably this value is externally visible as the "st_ino" in the stat structure and the "d_fileno" in the dirent structure. If so, this value will be different after this change so any program that has saved it somewhere could get confused. Not sure if this is really an issue though.

Non-obvious bug, but once figured out the fix is simple and clearly correct.

I would be happy to review a subsequent patch to make i_nlink a 16-bit unsigned int and move/adapt the KASSERT() checking for underflow.

Change in-memory i_nlink to a signed 32-bit value so that can check that it has not gone negative before assigning it to the on-disk unsigned 16-bit di_nlink field.

In D42767#976082, @mckusick wrote:

In D42767#975487, @kib wrote:

ffs_softdep.c has several lines of kind

sys/ufs/ffs/ffs_softdep.c:    KASSERT(ip->i_nlink >= 0, ("handle_workitem_remove: file ino "

which no longer make sense.

There are less critical comparisons like ip->i_nlink <= 0 at least in ufs_inode.c and ufs_vnops.c which could be cleaned up.

Actually i_nlink is a signed 32-bit value where the nlink count is maintained while the inode is in memory. So it can in fact be checked for negative values which of course should never be copied to the 16-bit unsigned value that is maintained on the disk.

In D42767#975487, @kib wrote:

ffs_softdep.c has several lines of kind

sys/ufs/ffs/ffs_softdep.c:    KASSERT(ip->i_nlink >= 0, ("handle_workitem_remove: file ino "

which no longer make sense.

There are less critical comparisons like ip->i_nlink <= 0 at least in ufs_inode.c and ufs_vnops.c which could be cleaned up.

Per kib suggestions:

Comments from kib.