Bug 274513 - bsdinstall(8): Can't disable forcing of 4k sectors with ZFS pool using scripted install
Summary: bsdinstall(8): Can't disable forcing of 4k sectors with ZFS pool using script...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: John Baldwin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-16 13:36 UTC by Albin "a12l" Otterhäll
Modified: 2024-01-09 19:56 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Albin "a12l" Otterhäll 2023-10-16 13:36:32 UTC
Context
=======

I'm trying to write my own `installconfig` script to make it easier to install FreeBSD 14 on my desktop machines. From what I understand, modern SSD drives have 8k physical sectors. Therefore I want to use 8k sectors for my ZFS pool.

I've added `-o ashift=13` to `$ZFSBOOT_POOL_CREATE_OPTIONS`. I now want to disable the forcing of 4k sectors to avoid problems with contradicting settings.

Expected Behavior
=================

I only need to set `$ZFSBOOT_FORCE_4K_SECTORS` to some specific value, so that the condition `[ "$ZFSBOOT_FORCE_4K_SECTORS" ]` evaluates to false.

Actual Behavior
===============

On line 833 in `//usr.sbin/bsdinstall/scripts/zfsboot` we have the if-statement

	if [ "$ZFSBOOT_FORCE_4K_SECTORS" ];

From what I understand, it's only with the empty string argument that the `test` builtin evaluates to false. I.e., for this condition to be evaluated to `false` requires `$ZFSBOOT_FORCE_4K_SECTORS` to be equal to the empty string.

But on line 74 in the above mentioned file we have

	: ${ZFSBOOT_FORCE_4K_SECTORS:=1}

From what I've understood from what I've read and tested, I conclude that this line will bind `ZFSBOOT_FORCE_4K_SECTORS=1` if `ZFSBOOT_FORCE_4K_SECTORS` is unset or equal to the empty string.

Therefore, I conclude that it's currently impossible to disable this option. Have I missed something, or is this a bug?
Comment 1 John Baldwin freebsd_committer freebsd_triage 2023-10-20 20:36:18 UTC
I have posted a possible fix for review at https://reviews.freebsd.org/D42319.  Would you be able to test it?
Comment 2 Albin "a12l" Otterhäll 2023-10-21 14:23:43 UTC
(In reply to John Baldwin from comment #1)

> I have posted a possible fix for review at https://reviews.freebsd.org/D42319.  Would you be able to test it?

Thanks! I'm unable to test this fix because I'm not yet running FreeBSD (scripting the installer was part of the process for migrating from NixOS to FreeBSD). And while I'm planning to learn how to build FreeBSD on my own with customizations, I don't think I'll be able to do that in a timely manner to give you feedback.

What I was able to do was to test this using Bash's sh compatible mode

```shell
#!/usr/bin/env sh

ZFSBOOT_FORCE_4K_SECTORS=''

: ${ZFSBOOT_FORCE_4K_SECTORS=1}

if [ "$ZFSBOOT_FORCE_4K_SECTORS" ];
  then printf 'True\n'
  else printf 'False\n'
fi
```

this outputs `False`, so I guess it would work.

I think if would be good if we added a sentence to the `ZFSBOOT_FORCE_4K_SECTORS` in bsdinstall(8) about setting the `ZFSBOOT_FORCE_4K_SECTORS=''` to disable it, because anything else will result in 4K sectors being used.

A little bit off-topic, but still relevant

	Indicates  either the pool will use 4K or 512 sec-
	tors.  If this variable is not empty,  4K  sectors
	will be used.  Default: "1"

I'm not sure, but if you disable this option you don't actually choose between 4K or 512B sectors? From what I see, you choose between 4K sectors or what the disk report as its physical sector size (there 512B is still common)?

Maybe change it to something similar to

	Indicates to either the pool will force using 4K sectors,
	or will trust the drives reported sector size. You can set
	a specific size by using the zpool property ashift.   If
	this variable is anything else than the empty string '' 4K
	sectors will be used.   Default: "1"

But I would honestly prefer a variable `${ZFSBOOT_SECTOR_SIZE:=4K}`, but I guess that is a bit off-topic for this PR.
Comment 3 commit-hook freebsd_committer freebsd_triage 2023-11-18 19:21:52 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=de82aed1192470574a08d3e479d81c4c1280487a

commit de82aed1192470574a08d3e479d81c4c1280487a
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-11-18 19:18:29 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-11-18 19:18:29 +0000

    bsdinstall zfsboot: Don't override ZFSBOOT_FORCE_4K_SECTORS if it is null.

    Only set a default value of 1 if the shell variable is unset.  This allows
    installer scripts to disable the variable.

    PR:             274513
    Reported by:    Albin "a12l" Otterhäll <bugs.freebsd.org@a12l.xyz>
    Differential Revision:  https://reviews.freebsd.org/D42319

 usr.sbin/bsdinstall/scripts/zfsboot | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-11-18 19:46:57 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=84f773037ee4fc78585501a2ce2f5398f0bae9f4

commit 84f773037ee4fc78585501a2ce2f5398f0bae9f4
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-11-18 19:31:07 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-11-18 19:45:42 +0000

    bsdinstall.8: Clarify the description of ZFSBOOT_FORCE_4K_SECTORS

    This variable does not set the exact sector size of the pool, but
    controls the minimum sector size.  The sector size of the underlying
    disks can always be larger than the minium controlled by this knob.

    PR:             274513
    Reported by:    Albin "a12l" Otterhäll <bugs.freebsd.org@a12l.xyz>

 usr.sbin/bsdinstall/bsdinstall.8 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Comment 5 John Baldwin freebsd_committer freebsd_triage 2023-11-18 19:47:54 UTC
Thanks, I've taken a stab at updating the description.  In particular, the knob only controls the minimum size as you noted.  In practice this knob controls several other settings such as the alignment of GPT partitions, etc. as well.
Comment 6 commit-hook freebsd_committer freebsd_triage 2024-01-03 20:33:44 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=bbfd69760203a7ee0532e96cbb7e69ec6ee7118b

commit bbfd69760203a7ee0532e96cbb7e69ec6ee7118b
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-11-18 19:31:07 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-01-03 20:18:56 +0000

    bsdinstall.8: Clarify the description of ZFSBOOT_FORCE_4K_SECTORS

    This variable does not set the exact sector size of the pool, but
    controls the minimum sector size.  The sector size of the underlying
    disks can always be larger than the minium controlled by this knob.

    PR:             274513
    Reported by:    Albin "a12l" Otterhäll <bugs.freebsd.org@a12l.xyz>

    (cherry picked from commit 84f773037ee4fc78585501a2ce2f5398f0bae9f4)

 usr.sbin/bsdinstall/bsdinstall.8 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2024-01-03 20:33:47 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=904ebc903cbe4f528a7e65f694d11cbdebcc353a

commit 904ebc903cbe4f528a7e65f694d11cbdebcc353a
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-11-18 19:18:29 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-01-03 20:18:50 +0000

    bsdinstall zfsboot: Don't override ZFSBOOT_FORCE_4K_SECTORS if it is null.

    Only set a default value of 1 if the shell variable is unset.  This allows
    installer scripts to disable the variable.

    PR:             274513
    Reported by:    Albin "a12l" Otterhäll <bugs.freebsd.org@a12l.xyz>
    Differential Revision:  https://reviews.freebsd.org/D42319

    (cherry picked from commit de82aed1192470574a08d3e479d81c4c1280487a)

 usr.sbin/bsdinstall/scripts/zfsboot | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 8 Albin "a12l" Otterhäll 2024-01-03 22:57:03 UTC
(In reply to John Baldwin from comment #5)
Sorry for late reply! Forgot to write you a reply.

This looks much better!
Comment 9 commit-hook freebsd_committer freebsd_triage 2024-01-05 00:23:31 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=a463db46a11ce84da39206c493ab0c4c02c29eb1

commit a463db46a11ce84da39206c493ab0c4c02c29eb1
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-11-18 19:18:29 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-01-05 00:21:59 +0000

    bsdinstall zfsboot: Don't override ZFSBOOT_FORCE_4K_SECTORS if it is null.

    Only set a default value of 1 if the shell variable is unset.  This allows
    installer scripts to disable the variable.

    PR:             274513
    Reported by:    Albin "a12l" Otterhäll <bugs.freebsd.org@a12l.xyz>
    Differential Revision:  https://reviews.freebsd.org/D42319

    (cherry picked from commit de82aed1192470574a08d3e479d81c4c1280487a)

 usr.sbin/bsdinstall/scripts/zfsboot | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-01-05 00:23:34 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=57238e6d03710a040ad420f0632ba07076372ec5

commit 57238e6d03710a040ad420f0632ba07076372ec5
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-11-18 19:31:07 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-01-05 00:22:09 +0000

    bsdinstall.8: Clarify the description of ZFSBOOT_FORCE_4K_SECTORS

    This variable does not set the exact sector size of the pool, but
    controls the minimum sector size.  The sector size of the underlying
    disks can always be larger than the minium controlled by this knob.

    PR:             274513
    Reported by:    Albin "a12l" Otterhäll <bugs.freebsd.org@a12l.xyz>

    (cherry picked from commit 84f773037ee4fc78585501a2ce2f5398f0bae9f4)

 usr.sbin/bsdinstall/bsdinstall.8 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)