Re: git: 077f757d72e5 - main - newfs_msdos: align data area to VM page boundary by default

Am 05.06.24 um 00:25 schrieb Konstantin Belousov:
> On Tue, Jun 04, 2024 at 08:39:16AM +0200, Stefan Eßer wrote:
>> Am 03.06.24 um 19:25 schrieb Jessica Clarke:
>>> On 2 Jun 2024, at 13:27, Stefan Eßer <se@FreeBSD.org> wrote:
>>>>
>>>> The branch main has been updated by se:
>>>>
>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=077f757d72e561eb84193d8e58f63e96e69b8096
>>>>
>>>> commit 077f757d72e561eb84193d8e58f63e96e69b8096
>>>> Author:     Stefan Eßer <se@FreeBSD.org>
>>>> AuthorDate: 2024-06-02 12:07:52 +0000
>>>> Commit:     Stefan Eßer <se@FreeBSD.org>
>>>> CommitDate: 2024-06-02 12:07:52 +0000
>>>>
>>>>      newfs_msdos: align data area to VM page boundary by default
>> [...]
>>>
>>> Hi,
>>> This has completely broken[1] all of the Linux and macOS cross-build CI
>>> jobs (which pass -DWITH_DISK_IMAGE_TOOLS_BOOTSTRAP). Please either fix
>>> this promptly or back it out until you can do so.
>>>
>>> Jess
>>>
>>> [1] See https://github.com/freebsd/freebsd-src/actions/runs/9338256762
>>
>> Thank you for the report and sorry for the breakage.
>>
>> This should be fixed in commit 41ee91c64f47faaa by skipping the
>> alignment of the data area, if PAGE_SIZE is not defined.
>>
>> An alternative approach would have been to define PAGE_SIZE to a
>> sane default value of 4096 (as used on most current architectures
>> by default), if not set in the build environment.
>>
>> If there is consensus that this would be more appropriate (since
>> it matches what the man page says), I'd revert the latest commit
>> and instead of it commit the following change:
> I believe this is a better approach.
> It does not unexpectedly hide some code.

There is an even simpler solution, which restores identical build
results on FreeBSD, MacOS, and Linux:

Always adjusting to a multiple of the cluster size works for all
page sizes. Adjusting to a granularity of > 4 KB is not required
to fix the buffer cache misalignment, but large clusters are only
useful on very large volumes (i.e., with more than 2000 clusters,
else a smaller cluster would be more efficient).

I have updated the previous review:

	https://reviews.freebsd.org/D45436

index 1bca560a59e1..dcc2bb982efc 100644
--- a/sbin/newfs_msdos/mkfs_msdos.c
+++ b/sbin/newfs_msdos/mkfs_msdos.c
@@ -568,14 +568,7 @@
             x1 += (bpb.bpbBigFATsecs - 1) * bpb.bpbFATs;
         }
         if (set_res) {
-           if (o.align)
-               alignto = bpb.bpbSecPerClust;
-           else
-#ifdef PAGE_SIZE
-               alignto = PAGE_SIZE / bpb.bpbBytesPerSec;
-#else
-               alignto = 1;
-#endif
+           alignto = bpb.bpbSecPerClust;
             if (alignto > 1) {
                 /* align data clusters */

Best regards, STefan