Now looks good. Sorry for the slow response.

The use of brelse in makefs/ffs/ffs_balloc.c is inconsistent. Assuming you are consistent in having bwrite brelse in both error and non-error cases there are two places in ffs_balloc.c that need to have brelse removed.

Looks ready to commit.

Have you verified that these changes resolve the issues reported in PR #270587?

Looks good.

This looks ready to go to me. I concur with putting off your other issues for another change later.

In D39246#894573, @chs wrote:

The concern I have with this change is about user interface. With this patch, if someone renames a directory that is the root of a subtree containing other directories and then unmounts the file system cleanly, fsck will report that the file system is now corrupted because all of the dirdepth values for directories in that subtree are wrong. In fact, every existing file system will immediately be reported as corrupted the first time that the new fsck is run. There are millions of people in the world who know what the fsck output for a clean and uncorrupted UFS file system looks like, and that clean fsck output is very easy to recognize because it's very short and always the same (modulo the line with the last-mounted-on path and the line at the end with the block/inode usage and fragmentation summary). After this patch, fsck can produce millions of lines of output for file systems that aren't really corrupted but rather are merely not optimal, and it will no longer be so simple to tell if a file system is corrupted or not by looking at the fsck output.

I think that fsck should not report incorrect dirdepth values at all, because an incorrect dirdepth value is not considered corruption. It would still be desirable for fsck to set the dirdepth fields to their optimal values, but producing potentially a huge number of lines of output while doing so would obscure the fact that the file system may otherwise be completely consistent, so it would be best if these updates to dirdepth values were done silently. I realize that this changes a different long-standing tradition that fsck reports (and asks about) every individual change that it makes, but prior to this patch, everything that fsck would change represented either a kernel bug or a hardware failure of some kind (ie. a serious problem), whereas a dirdepth value being wrong is a normal situation that the kernel creates intentionally and is thus nothing to worry about.

Responding to comment.

These changes look good. I recommend adding the changes to the df(1) and msdos(5) man pages here so it all gets checked in together.

IMO not adding extra deductible data to the on-disk format is good, or if we do add such data, then we should check it dynamically. I think you would go with the dinode field re-use, but then please check that the condition that I formulated, and do the usual action when the broken metadata detected.

I do not mean to check it all way back to the root. I mean, if ufs_lookup(dvp) found vp which is directory, then vp.dirdepth must be dvp.dirdepth + 1 or both must be zero.

In D39246#893800, @kib wrote:

If I mount a filesystem without running fsck on it at all, then the things are undefined? For instance, I often boot my crash boxes single-user and directly mount some partition r/w

Thanks for cleanup.

Looks ready to go. Happy the change ended up so simple. I have the equivalent changes ready for UFS/FFS:

In UFS/FFS we handle the mangled '..' entry by silently ignoring the error and proceeding with the removal of the old directory entry. That leaves the only cleanup for fsck is to fix the broken '..' entry. I think that is what you should do too.

Since I'll be traveling for one week without good Internet access, I'll probably not be able upload the updated patch before 2023-03-20, though.

No rush. I hope some of your travel will be for fun.

Improvements so far look good.
Several places already noted still need new or updated comments.
Should add updates to msdos(5) and df(1) manual pages.

In email to me Stefan Esser writes:

I'm not happy with the choice of function names for the increment/decrement macros, and I think that the exact location of their invocation should be reviewed with regard to error cases.

In D38987#887669, @se wrote:

In D38987#887609, @mckusick wrote:

This looks like a useful change. How difficult would it be to make it also work for FAT32 filesystems?

FAT12/16 have a fixed number of directory blocks between the FAT tables and the data area.
FAT32 does away with a fixed root directory and uses kind of a 1st level sub-directory for root directory entries instead (i.e. there is a single block pointer in the boot block which points at the start of a directory "file" of variable length).

Since there is no size limit for the root directory on FAT32, there is no limit on the number of "inodes" as reported by statfs().
Both the number of inodes and of free inodes are reported as 0 and the df command deals with this special case by not reporting a percentage of free inodes.

This looks like a useful change. How difficult would it be to make it also work for FAT32 filesystems?

I found a disk image to reproduce this panic: https://people.freebsd.org/~pho/fsck11.sh.diskimage.20230228T064402.gz

I concur with the revisions. Glad to see this being done.

These changes look good to me. Only change is to eliminate the blank lines.

Fix for Peter Holm's latest evil filesystem. When trying to check just the minimal set of fields to make a superblock usable I failed to verify fs_ncg which then made the attempt to read the summary information take approximately forever.

In D38789#882931, @fbsd_paepcke.de wrote:

But the flags are all reversible via common base userland tools (runlevel/offline) similar to a simple r/w re-mount. So its no a bug, the on-disk written flag tracks only the last known r/w state, but does not enforces a state?

As far as I can see we currently have in ffs 21 x int32_t to burn.

/* reserved for future constants */
int32_t fs_sparecon32[21];

Is there a small chance to burn one of the 21 spares
into a generic future proof mount-restrictions bit-matrix
and use the first bit as a (backwards compatible) future
real fs_read-only lock bool?

As you note, the existing value of the ronly flag is ignored when a filesystem is mounted. Rather it is set to reflect how the filesystem has been mounted. So presetting it to a value when creating the filesystem is pointless. If you want a filesystem in which the files and directories cannot be changed then you should set the SF_IMMUTABLE and SF_NOUNLINK flags (see chflags(2) for details) in every inode in the filesystem when you create it.

I will make the corresponding changes in UFS once this is committed.

Fix problems found by Peter Holm. Fine tune and consolodate reported errors when fixing broken filesystems.

Note that the three FS's that exhibit these problems are available at:
https://www.funkthat.com/~jmg/FreeBSD/ffs.afl/

Per my comment, both ext2 and ufs should no longer panic in ufs_baddir.

In D37907#862802, @kib wrote:

If you do not want to move it to libutil, then a private library might be due. I do not like idea of shipping one more library for such specialized purpose.

Private libraries are not distributed, they are only built during buildword stage and are statically linked into consumers. To build it, it seems to be enough to put INTERNALLIB= line into Makefile.

This change looks correct.

This seems like a reasonable optimization to me.

In D37907#861317, @kib wrote:

Why not add the functions to libutil?

Looks good to go.

This approach will certainly fix the problem.