In D46876#1068975, @emaste wrote:

phabricator tagged it with "does not need to be reviewed"

Phabricator recognizes it as a generated file and so doesn't need to be reviewed. The note is intended more for the assigned reviewers - the change can be included, Phab just notes that reviewers don't need to look at it.

Address review comments.

In D46876#1068925, @des wrote:

You also need to remove tools/build/options/WITH_GH_BC, move the relevant section of tools/build/mk/OptionalObsoleteFiles.inc to ObsoleteFiles.inc, and regenerate share/man/man5/src.conf.5.

In D46876#1068928, @des wrote:

In D46876#1068845, @emaste wrote:

Is there value in keeping the tests that were added to the old version?

Perhaps. More importantly, Gavin Howard's bc has its own test suite which we could adopt. Take a look at how Apple did it: https://github.com/apple-oss-distributions/bc

Update diff to include removal of dependency on MK_GH_BC from usr.bin/Makefile and from tools/build/mk/OptionalObsoleteFiles.inc.

In D45436#1036630, @kib wrote:

I still think that changing msdosfs to use directory blocks owned by dir vnodes (instead of devvp) is the best fix for kernel.

Modify the patch to align the data area to a cluster boundary by default.

As stated before, the lack of support of distinct physical and logical sector numbers in mmap() would require to drop mmap support from msdosfs, if this approach was to be committed.

Document latest version of this attempt to align FAT sectors with VM Pages.
This version passes all MSDOSFS stress tests, except for one (stress2/misc/msdos10.sh) that uses mmap() calls when running "fsx".

Please commit with a relative short MFC delay (e.g. 1 week).
I had planned to MFC the (now fixed) code on March 26 (1 month after the commit to -CURRENT).
Since this fix is required to be merged with the initial commit, I plan to squash the cherry-picked commits (including your fix).

In D44472#1014219, @tuexen wrote:

In D44472#1014214, @se wrote:

Thank you for finding the logic error.

The commented out extra condition was meant to be enabled in a future update to make little-endian systems assign a constant to is_le and to let constant expression evaluation effectively remove the conditional byte swap on them.

My understanding is that is_le is true, iff the byte order of the hints file is little endian, not if the host byte order
is little endian. Is that correct?
Using le32toh(1) == 1 or better htole32(1) == 1 checks the host byte order, not the byte order of the file.

Using your condition would result in is_le being true if the host byte order is little endian or the file byte order
is little endian. So if you run this on a host with host byte order being little endian using a file with file byte order
being big endian, is_le would be true. Therefore you would use le32toh for the conversion, which wouldn't do
anything, and it would break.

Thank you for finding the logic error.

In D44319#1012663, @pho wrote:

D44319.id135654.diff doesn't seem to work for me:

root@mercat1:/usr/src/tools/test/stress2/misc # ./all.sh -onc msdos11.sh
20240318 13:16:17 all: msdos11.sh
** /dev/md10a
** Phase 1 - Read FAT and checking connectivity
** Phase 2 - Checking Directories
** Phase 3 - Checking for Lost Files
Lost cluster chain at cluster 4
1 Cluster(s) lost
Reconnect? yes
No LOST.DIR directory
Clear? yes
Lost cluster chain at cluster 5
1 Cluster(s) lost
Reconnect? yes
No LOST.DIR directory
Clear? yes

In D44319#1011511, @kib wrote:

This looks somewhat strange to me. Most of the weirdness is in that we have lblkno != blkno for devvp buffers, which I believe we did not have anywhere before.

Update diff to include the new macros breadb() and breadbn() added to sys/buf.h to allow passing of separate physical and buffer cache block number parameters.

A version based on these patches (but with slightly different command line options) has been discussed and accepted in review D44093.