The logic is easier for me to follow.

I am really happy to see these changes as it will be especially helpful for vnodes that waste many Megabytes of space.
I concur with Jeff that the new code is difficult to follow, though do not have any suggestions on how to simplify it.

After consulting with kib@ I have concluded that it is acceptable to MFC -r343111 to stable/12. You should MFC your change separately (i.e., in a second checkin).

This change seems like an improvement over what we have now.

I have never been a fan of short-circuiting the layers, so am fine with keeping them intact.

Looks good to go.

OK, looks good to go.

I agree with mac@ that it looks like EFRAGS is only in an unused code path. But it does not hurt to map it to a FreeBSD error in the event that the code path gets brought back to life.

I noted I don't know what guaratees are needed for quota files, thus I for safety I regressed the quota-induced scan to go over all vnodes in the mount point. Getting rid of the extra loop is a laudable goal of course, but I don't know how it should look like on a kernel with my patch. If the current places which put the vnode on the dirty list are sufficient that's great, but chances are excellent quota code needs extra calls

My confusion is what "clusters" represent. Apparently a cluster is a 32KB block. Based on your clarification, I now agree with your calculation and printing of free space, but suggest that you clarify that clusters are 32KB blocks (either by replacing the word "clusters" with "32KB blocks", or if clusters is meaningful to administrators change it to "32KB clusters").

Overall I like these changes. I have one specific comment here and also a suggested change to qsync/qsyncvp to mitigate the increased cost of qsync().

What is the reason for adding this macro to set i_flags?

The heavy use of qsync() is in ffs_sync_lazy() and ffs_sync(). These should be using qsyncvp() in their loops rather than separately calling qsync() as show in my attached diff.

The current output is number of free blocks and number of bad blocks. You are changing it to number of free bytes and number of bad bytes.
Since fsck for all other filesystems reports blocks this is a non-standard semantic change, so I recommend that you restore the / 1024 to output block counts.
If you think that it is useful to have byte counts then should make it clear you are reporting bytes and not blocks (e.g., %sB free bytes).

In D23049#505425, @emaste wrote:

Might we remove it in the future? I.e., should we add a note that users should not rely on it remaining?

This looks good.

Adding a new error number requires changes to a lot more files than just lib/libc/gen/errlst.c. See for example -r343111 when I added EINTEGRITY which touched 16 different files in the system. And that is just to add the base error. It then has to be added to every system call manual page for which it can be returned.

A couple of minor nits noted inline.

This request is entirely reasonable.

I concur that VFS_STATFS should be done here.

With the minor changes requested, looks good to me.

What scenario would ever have v_writecount be less than zero?

Removing the function is right approach. All looks good.

My inline comment seems to have been lost. I requested that ufs_prepare_reclaim() be made static.

This change looks good to me modulo minor inline comment.

Any progress on getting this done?

This is long overdue. I would love to have these known LORs ignored.

I concur that taking evasive action is preferable to looping forever. However, we can do better than just bailing out. The kernel code checks for a bad d_reclen and if found skips up to the beginning of the next block within the directory using this code:

Looks good.

Looks good to go.

Some of the checks that I am contemplating are expensive in that they may require extra I/O operations. Another possible thing is that I may spawn a process to run a full filesystem check before proceeding with the mount. These are not things that should or need to be done in the case of a trusted filesystem. I believe that the name "untrusted" well and succinctly describes this option.

Doing asynchronous writes prevents clustering. The correct fix is to say:

Simply deleting the comments about splbio is fine. It might be helpful to change the comment to say `Called with the soft update lock held.' In many places the lock ownership is asserted which may be sufficient, or perhaps the assertion should be added where it is not already there.

Changing ufs_bamparray() to return errors rather than panicing is a helpful move forward.

Looks good to me.

Has this been resolved / committed?

Has this been resolved / committed?