[Buildroot] [PATCH 1/2] Adding btrfs-progs host package.

[Buildroot] [PATCH 1/2] Adding btrfs-progs host package.

[Robert J. Heywood]

Adding a new option to build the btrfs toolset for the host.

Signed-off-by: Robert J. Heywood <robert.heywood@codethink.co.uk>
---
 package/Config.in.host             | 1 +
 package/btrfs-progs/Config.in.host | 6 ++++++
 package/btrfs-progs/btrfs-progs.mk | 4 ++++
 3 files changed, 11 insertions(+)
 create mode 100644 package/btrfs-progs/Config.in.host

diff --git a/package/Config.in.host b/package/Config.in.host
index 7838ffc219..432597ed31 100644
--- a/package/Config.in.host
+++ b/package/Config.in.host
@@ -2,6 +2,7 @@ menu "Host utilities"
 
 	source "package/aespipe/Config.in.host"
 	source "package/android-tools/Config.in.host"
+	source "package/btrfs-progs/Config.in.host"
 	source "package/cargo/Config.in.host"
 	source "package/cbootimage/Config.in.host"
 	source "package/checkpolicy/Config.in.host"
diff --git a/package/btrfs-progs/Config.in.host b/package/btrfs-progs/Config.in.host
new file mode 100644
index 0000000000..cdcd2e4ec4
--- /dev/null
+++ b/package/btrfs-progs/Config.in.host
@@ -0,0 +1,6 @@
+config BR2_PACKAGE_HOST_BTRFS_PROGS
+	bool "host btrfs-progs"
+	help
+	  Btrfs filesystem utilities
+
+	  https://btrfs.wiki.kernel.org/index.php/Main_Page
diff --git a/package/btrfs-progs/btrfs-progs.mk b/package/btrfs-progs/btrfs-progs.mk
index b4cc839352..3129ecc714 100644
--- a/package/btrfs-progs/btrfs-progs.mk
+++ b/package/btrfs-progs/btrfs-progs.mk
@@ -21,4 +21,8 @@ BTRFS_PROGS_INSTALL_TARGET_OPTS = DESTDIR=$(TARGET_DIR) install-static
 BTRFS_PROGS_INSTALL_STAGING_OPTS = DESTDIR=$(STAGING_DIR) install-static
 endif
 
+HOST_BTRFS_PROGS_DEPENDENCIES = host-e2fsprogs host-lzo host-zlib
+HOST_BTRFS_PROGS_CONF_OPTS = --disable-backtrace --disable-zstd --disable-python
+
 $(eval $(autotools-package))
+$(eval $(host-autotools-package))
-- 
2.11.0

[Buildroot] [PATCH 2/2] Adding btrfs rootfs option.

[Robert J. Heywood]

This patch makes it possible to format the rootfs using btrfs.
Introduces the option; BR2_TARGET_ROOTFS_BTRFS

When selected, the user is able to specify the filesystem size,
label, options, and node and sector sizes.
The new files are based on fs/ext2/{Config.in,ext2.mk}

Signed-off-by: Robert J. Heywood <robert.heywood@codethink.co.uk>
---
 fs/Config.in       |  1 +
 fs/btrfs/Config.in | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/btrfs.mk  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 fs/btrfs/Config.in
 create mode 100644 fs/btrfs/btrfs.mk

diff --git a/fs/Config.in b/fs/Config.in
index c25b01c3de..24f22fd7e3 100644
--- a/fs/Config.in
+++ b/fs/Config.in
@@ -1,6 +1,7 @@
 menu "Filesystem images"
 
 source "fs/axfs/Config.in"
+source "fs/btrfs/Config.in"
 source "fs/cloop/Config.in"
 source "fs/cpio/Config.in"
 source "fs/cramfs/Config.in"
diff --git a/fs/btrfs/Config.in b/fs/btrfs/Config.in
new file mode 100644
index 0000000000..cd563235ec
--- /dev/null
+++ b/fs/btrfs/Config.in
@@ -0,0 +1,48 @@
+config BR2_TARGET_ROOTFS_BTRFS
+	bool "btrfs root filesystem"
+	select BR2_PACKAGE_HOST_BTRFS_PROGS
+	help
+	  Build a btrfs root filesystem. If you enable this option, you
+	  probably want to enable the btrfs-progs package too.
+
+
+if BR2_TARGET_ROOTFS_BTRFS
+
+config BR2_TARGET_ROOTFS_BTRFS_LABEL
+	string "filesystem label"
+
+config BR2_TARGET_ROOTFS_BTRFS_SIZE
+	string "filesystem size"
+	default "100M"
+	help
+	  The size of the filesystem image. If it does not have a
+	  suffix, it is interpreted as power-of-two kilobytes. If it is
+	  suffixed by 'k', 'm', 'g', 't' (either upper-case or
+	  lower-case), then it is interpreted in power-of-two kilobytes,
+	  megabytes, gigabytes, terabytes, etc.
+
+config BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE
+	string "btree node size"
+	help
+	  The tree block size in which btrfs stores metadata. The default
+	  value is 16KiB (16384) or the page size, whichever is bigger.
+	  Must be a multiple of the sectorsize, but not larger than 64KiB
+	  (65536).
+
+config BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR
+	string "sector size"
+	help
+	  The default value is the page size and is autodetected. If the
+	  sectorsize differs from the page size, the created filesystem
+	  may not be mountable by the kernel. Therefore it is not
+	  recommended to use this option unless you are going to mount it
+	  on a system with the appropriate page size.
+
+config BR2_TARGET_ROOTFS_BTRFS_FEATURES
+	string "Filesystem Features"
+	help
+	  A comma separated string of features that can be enabled
+	  during creation time.
+	  For a list of available options, use; `mkfs.btrfs -O list-all`
+
+endif # BR2_TARGET_ROOTFS_BTRFS
diff --git a/fs/btrfs/btrfs.mk b/fs/btrfs/btrfs.mk
new file mode 100644
index 0000000000..9f474edac3
--- /dev/null
+++ b/fs/btrfs/btrfs.mk
@@ -0,0 +1,45 @@
+################################################################################
+#
+# Build the btrfs root filesystem image
+#
+################################################################################
+
+BTRFS_SIZE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE))
+ifeq ($(BR2_TARGET_ROOTFS_BTRFS)-$(BTRFS_SIZE),y-)
+$(error BR2_TARGET_ROOTFS_BTRFS_SIZE cannot be empty)
+endif
+
+BTRFS_SIZE_NODE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE))
+BTRFS_SIZE_NODE_FLAG = $(if $(BTRFS_SIZE_NODE),--nodesize "$(BTRFS_SIZE_NODE)",)
+
+BTRFS_SIZE_SECTOR = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR))
+BTRFS_SIZE_SECTOR_FLAG = $(if $(BTRFS_SIZE_SECTOR),--sectorsize "$(BTRFS_SIZE_SECTOR)",)
+
+BTRFS_FEATURES = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_FEATURES))
+BTRFS_FEATURES_FLAG = $(if $(BTRFS_FEATURES),--features "$(BTRFS_FEATURES)",)
+
+# qstrip results in stripping consecutive spaces into a single one. So the
+# variable is not qstrip-ed to preserve the integrity of the string value.
+BTRFS_LABEL := $(subst ",,$(BR2_TARGET_ROOTFS_BTRFS_LABEL))
+## ")
+
+BTRFS_OPTS = \
+	-r $(TARGET_DIR) \
+	-L "$(BTRFS_LABEL)" \
+	$(BTRFS_SIZE_NODE_FLAG) \
+	$(BTRFS_SIZE_SECTOR_FLAG) \
+	$(BTRFS_FEATURES_FLAG)
+
+ROOTFS_BTRFS_DEPENDENCIES = host-btrfs-progs
+
+define ROOTFS_BTRFS_CMD
+	rm -f $@
+	fallocate -l $(BTRFS_SIZE) $@
+	$(HOST_DIR)/bin/mkfs.btrfs $(BTRFS_OPTS) $@ \
+	|| { ret=$$?; \
+	     echo "*** Maybe you need to increase the filesystem size (BR2_TARGET_ROOTFS_BTRFS_SIZE)" 1>&2; \
+	     exit $$ret; \
+	}
+endef
+
+$(eval $(rootfs))
-- 
2.11.0

[Buildroot] [PATCH 2/2] Adding btrfs rootfs option.

[Yann E. MORIN]

Robert, All,

On 2018-08-21 17:04 +0100, Robert J. Heywood spake thusly:
> This patch makes it possible to format the rootfs using btrfs.
> Introduces the option; BR2_TARGET_ROOTFS_BTRFS
> 
> When selected, the user is able to specify the filesystem size,
> label, options, and node and sector sizes.
> The new files are based on fs/ext2/{Config.in,ext2.mk}

Nice addition! :-)

> Signed-off-by: Robert J. Heywood <robert.heywood@codethink.co.uk>
[--SNIP--]
> +config BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE
> +	string "btree node size"
> +	help
> +	  The tree block size in which btrfs stores metadata. The default
> +	  value is 16KiB (16384) or the page size, whichever is bigger.

How is the page size detected? I'd like to be sure this is detecting the
page size of the target, not that of the build machine.

Also, is it possible to indicate the size with a 'k' like for the fs
size, or must it be only bytes?

> +	  Must be a multiple of the sectorsize, but not larger than 64KiB
> +	  (65536).
> +
> +config BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR
> +	string "sector size"
> +	help
> +	  The default value is the page size and is autodetected. If the
> +	  sectorsize differs from the page size, the created filesystem
> +	  may not be mountable by the kernel. Therefore it is not
> +	  recommended to use this option unless you are going to mount it
> +	  on a system with the appropriate page size.

Given that the filsystem is specifically made for the target that will
mount it, does it make sense to ffer that option at all? I.e. if the
target uses a 4KiB page size, does it make sense to generatea rootfs
that has a different secotr size, and thus not mountable on the target
at all?

My suggestion: don't add that option.

> +config BR2_TARGET_ROOTFS_BTRFS_FEATURES
> +	string "Filesystem Features"
> +	help
> +	  A comma separated string of features that can be enabled
> +	  during creation time.
> +	  For a list of available options, use; `mkfs.btrfs -O list-all`

Nice. I guess that has to be done using the host-btrfs we built, as its
feature list may be different than the one from the build machine.

> +endif # BR2_TARGET_ROOTFS_BTRFS
> diff --git a/fs/btrfs/btrfs.mk b/fs/btrfs/btrfs.mk
> new file mode 100644
> index 0000000000..9f474edac3
> --- /dev/null
> +++ b/fs/btrfs/btrfs.mk
> @@ -0,0 +1,45 @@
> +################################################################################
> +#
> +# Build the btrfs root filesystem image
> +#
> +################################################################################
> +
> +BTRFS_SIZE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE))
> +ifeq ($(BR2_TARGET_ROOTFS_BTRFS)-$(BTRFS_SIZE),y-)
> +$(error BR2_TARGET_ROOTFS_BTRFS_SIZE cannot be empty)
> +endif
> +
> +BTRFS_SIZE_NODE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE))
> +BTRFS_SIZE_NODE_FLAG = $(if $(BTRFS_SIZE_NODE),--nodesize "$(BTRFS_SIZE_NODE)",)
> 
> +BTRFS_SIZE_SECTOR = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR))
> +BTRFS_SIZE_SECTOR_FLAG = $(if $(BTRFS_SIZE_SECTOR),--sectorsize "$(BTRFS_SIZE_SECTOR)",)
> +
> +BTRFS_FEATURES = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_FEATURES))
> +BTRFS_FEATURES_FLAG = $(if $(BTRFS_FEATURES),--features "$(BTRFS_FEATURES)",)
> +
> +# qstrip results in stripping consecutive spaces into a single one. So the
> +# variable is not qstrip-ed to preserve the integrity of the string value.
> +BTRFS_LABEL := $(subst ",,$(BR2_TARGET_ROOTFS_BTRFS_LABEL))
> +## ")

Why double comment here?

> +BTRFS_OPTS = \
> +	-r $(TARGET_DIR) \
> +	-L "$(BTRFS_LABEL)" \
> +	$(BTRFS_SIZE_NODE_FLAG) \
> +	$(BTRFS_SIZE_SECTOR_FLAG) \
> +	$(BTRFS_FEATURES_FLAG)

It is a bit annoying that you have to qstrip the variables, just to
re-add double quotes right after. However, I prefer the way you do it,
because then it is obvious the strings are quotted when passed to the
shell.

Furhtermore, I would quote with single quotes, to avoid shell expansion.

Also, no need for the trailing comma when there is no 'else' clause.

No need for intermediate variables for each option; just directly set
the BTRFS_OPTS variable.

Finally, the label case is a bit annyoing, but here is no better
solution, and is exactly what we do in ext2.mk.

    BTRFS_SIZE_NODE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE))
    BTRFS_SIZE_SECTOR = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR))
    BTRFS_FEATURES = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_FEATURES))
    # qstrip results in stripping consecutive spaces into a single one. So the
    # variable is not qstrip-ed to preserve the integrity of the string value.
    BTRFS_LABEL := $(subst ",,$(BR2_TARGET_ROOTFS_BTRFS_LABEL))
    # ")

    BTRFS_OPTS = \
        -r '$(TARGET_DIR)' \
        -L '$(BTRFS_LABEL)' \
        $(if $(BTRFS_SIZE_NODE),--nodesize '$(BTRFS_SIZE_NODE)') \
        $(if $(BTRFS_SIZE_SECTOR),--sectorsize '$(BTRFS_SIZE_SECTOR)') \
        $(if $(BTRFS_FEATURES),--features '$(BTRFS_FEATURES)')

> +
> +ROOTFS_BTRFS_DEPENDENCIES = host-btrfs-progs
> +
> +define ROOTFS_BTRFS_CMD
> +	rm -f $@
> +	fallocate -l $(BTRFS_SIZE) $@

Is fallocate really necessary here? If the host's filesystem is too
small to accomodate the image, then the mkfs.btrfs below fill fail; we
don't need fallocate to fail.

Besides, is fallocate readilly available on all distros?

Usually, we use truncate(1) to that effect; see for example:
    support/testing/tests/fs/test_ubi.py at 29

Regards,
Yann E. MORIN.

> +	$(HOST_DIR)/bin/mkfs.btrfs $(BTRFS_OPTS) $@ \
> +	|| { ret=$$?; \
> +	     echo "*** Maybe you need to increase the filesystem size (BR2_TARGET_ROOTFS_BTRFS_SIZE)" 1>&2; \
> +	     exit $$ret; \
> +	}
> +endef
> +
> +$(eval $(rootfs))
> -- 
> 2.11.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2] Adding btrfs-progs host package.

[Thomas Petazzoni]

Hello Robert,

Thanks for your contribution!

On Tue, 21 Aug 2018 17:04:11 +0100, Robert J. Heywood wrote:
> Adding a new option to build the btrfs toolset for the host.
> 
> Signed-off-by: Robert J. Heywood <robert.heywood@codethink.co.uk>
> ---
>  package/Config.in.host             | 1 +
>  package/btrfs-progs/Config.in.host | 6 ++++++
>  package/btrfs-progs/btrfs-progs.mk | 4 ++++
>  3 files changed, 11 insertions(+)
>  create mode 100644 package/btrfs-progs/Config.in.host

Applied to next after fixing the commit title to:

	btrfs-progs: add host package variant

Indeed, we want all our commit title to look like this:

	<package>: <description>

This should also be fixed for your PATCH 2/2, along with the other
comments made by Yann E. Morin.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/2] Adding btrfs rootfs option.

[Robert Heywood]

Hi Yann,
thanks for the through review :)
Comments inline.

On 21/08/18 21:27, Yann E. MORIN wrote:
> Robert, All,
> 
> On 2018-08-21 17:04 +0100, Robert J. Heywood spake thusly:
>> This patch makes it possible to format the rootfs using btrfs.
>> Introduces the option; BR2_TARGET_ROOTFS_BTRFS
>>
>> When selected, the user is able to specify the filesystem size,
>> label, options, and node and sector sizes.
>> The new files are based on fs/ext2/{Config.in,ext2.mk}
> 
> Nice addition! :-)
> 
>> Signed-off-by: Robert J. Heywood <robert.heywood@codethink.co.uk>
> [--SNIP--]
>> +config BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE
>> +	string "btree node size"
>> +	help
>> +	  The tree block size in which btrfs stores metadata. The default
>> +	  value is 16KiB (16384) or the page size, whichever is bigger.
> 
> How is the page size detected? I'd like to be sure this is detecting the
> page size of the target, not that of the build machine.
> 
> Also, is it possible to indicate the size with a 'k' like for the fs
> size, or must it be only bytes?
> 

Good question.
I guess because the makefile creates a sparse file with fallocate which 
mkfs.btrfs then formats, it won't be able to detect any page size (host or 
otherwise) and so will always be set to a default value of 16384.
I'll set that in the Config file explicitly.

It's possible to specify the size in kilobytes, ie 4k for 4096.
I can change the help text to reflect this.

>> +	  Must be a multiple of the sectorsize, but not larger than 64KiB
>> +	  (65536).
>> +
>> +config BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR
>> +	string "sector size"
>> +	help
>> +	  The default value is the page size and is autodetected. If the
>> +	  sectorsize differs from the page size, the created filesystem
>> +	  may not be mountable by the kernel. Therefore it is not
>> +	  recommended to use this option unless you are going to mount it
>> +	  on a system with the appropriate page size.
> 
> Given that the filsystem is specifically made for the target that will
> mount it, does it make sense to ffer that option at all? I.e. if the
> target uses a 4KiB page size, does it make sense to generatea rootfs
> that has a different secotr size, and thus not mountable on the target
> at all?
> 
> My suggestion: don't add that option.
> 

On my system (building for a raspi0) it autodetects the sector size to be 4k, 
and the resulting image is bootable. However I'm worried the host won't be able 
to detect the correct value for the target. In such a case I can see why it 
would be useful to set the sector size explicitly.

Is there another place where the sector size is specified? If so, it might be 
better to use the existing value instead.

>> +config BR2_TARGET_ROOTFS_BTRFS_FEATURES
>> +	string "Filesystem Features"
>> +	help
>> +	  A comma separated string of features that can be enabled
>> +	  during creation time.
>> +	  For a list of available options, use; `mkfs.btrfs -O list-all`
> 
> Nice. I guess that has to be done using the host-btrfs we built, as its
> feature list may be different than the one from the build machine.
> 

I can change the help text to include the full path to the binary.

>> +endif # BR2_TARGET_ROOTFS_BTRFS
>> diff --git a/fs/btrfs/btrfs.mk b/fs/btrfs/btrfs.mk
>> new file mode 100644
>> index 0000000000..9f474edac3
>> --- /dev/null
>> +++ b/fs/btrfs/btrfs.mk
>> @@ -0,0 +1,45 @@
>> +################################################################################
>> +#
>> +# Build the btrfs root filesystem image
>> +#
>> +################################################################################
>> +
>> +BTRFS_SIZE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE))
>> +ifeq ($(BR2_TARGET_ROOTFS_BTRFS)-$(BTRFS_SIZE),y-)
>> +$(error BR2_TARGET_ROOTFS_BTRFS_SIZE cannot be empty)
>> +endif
>> +
>> +BTRFS_SIZE_NODE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE))
>> +BTRFS_SIZE_NODE_FLAG = $(if $(BTRFS_SIZE_NODE),--nodesize "$(BTRFS_SIZE_NODE)",)
>>
>> +BTRFS_SIZE_SECTOR = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR))
>> +BTRFS_SIZE_SECTOR_FLAG = $(if $(BTRFS_SIZE_SECTOR),--sectorsize "$(BTRFS_SIZE_SECTOR)",)
>> +
>> +BTRFS_FEATURES = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_FEATURES))
>> +BTRFS_FEATURES_FLAG = $(if $(BTRFS_FEATURES),--features "$(BTRFS_FEATURES)",)
>> +
>> +# qstrip results in stripping consecutive spaces into a single one. So the
>> +# variable is not qstrip-ed to preserve the integrity of the string value.
>> +BTRFS_LABEL := $(subst ",,$(BR2_TARGET_ROOTFS_BTRFS_LABEL))
>> +## ")
> 
> Why double comment here?
> 

There is no reason for that. It was added when I first copied the ext2.mk file 
and commented out everything.
I'll remove one of them.

>> +BTRFS_OPTS = \
>> +	-r $(TARGET_DIR) \
>> +	-L "$(BTRFS_LABEL)" \
>> +	$(BTRFS_SIZE_NODE_FLAG) \
>> +	$(BTRFS_SIZE_SECTOR_FLAG) \
>> +	$(BTRFS_FEATURES_FLAG)
> 
> It is a bit annoying that you have to qstrip the variables, just to
> re-add double quotes right after. However, I prefer the way you do it,
> because then it is obvious the strings are quotted when passed to the
> shell.
> 
> Furhtermore, I would quote with single quotes, to avoid shell expansion.

Sure thing.

> 
> Also, no need for the trailing comma when there is no 'else' clause.

I didn't know that! :)

> 
> No need for intermediate variables for each option; just directly set
> the BTRFS_OPTS variable.
> 
> Finally, the label case is a bit annyoing, but here is no better
> solution, and is exactly what we do in ext2.mk.
> 
>      BTRFS_SIZE_NODE = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE))
>      BTRFS_SIZE_SECTOR = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR))
>      BTRFS_FEATURES = $(call qstrip,$(BR2_TARGET_ROOTFS_BTRFS_FEATURES))
>      # qstrip results in stripping consecutive spaces into a single one. So the
>      # variable is not qstrip-ed to preserve the integrity of the string value.
>      BTRFS_LABEL := $(subst ",,$(BR2_TARGET_ROOTFS_BTRFS_LABEL))
>      # ")
> 
>      BTRFS_OPTS = \
>          -r '$(TARGET_DIR)' \
>          -L '$(BTRFS_LABEL)' \
>          $(if $(BTRFS_SIZE_NODE),--nodesize '$(BTRFS_SIZE_NODE)') \
>          $(if $(BTRFS_SIZE_SECTOR),--sectorsize '$(BTRFS_SIZE_SECTOR)') \
>          $(if $(BTRFS_FEATURES),--features '$(BTRFS_FEATURES)')
> 

Good suggestions.

>> +
>> +ROOTFS_BTRFS_DEPENDENCIES = host-btrfs-progs
>> +
>> +define ROOTFS_BTRFS_CMD
>> +	rm -f $@
>> +	fallocate -l $(BTRFS_SIZE) $@
> 
> Is fallocate really necessary here? If the host's filesystem is too
> small to accomodate the image, then the mkfs.btrfs below fill fail; we
> don't need fallocate to fail.
> 
> Besides, is fallocate readilly available on all distros?
> 
> Usually, we use truncate(1) to that effect; see for example:
>      support/testing/tests/fs/test_ubi.py at 29
> 

fallocate is used here because (unlike other mkfs programs) you can't specify a 
size and have mkfs.btrfs create an image file for you. It needs a sparse file to 
work with.

I'll be happy to change this to truncate, as per your example.

> Regards,
> Yann E. MORIN.
> 
>> +	$(HOST_DIR)/bin/mkfs.btrfs $(BTRFS_OPTS) $@ \
>> +	|| { ret=$$?; \
>> +	     echo "*** Maybe you need to increase the filesystem size (BR2_TARGET_ROOTFS_BTRFS_SIZE)" 1>&2; \
>> +	     exit $$ret; \
>> +	}
>> +endef
>> +
>> +$(eval $(rootfs))
>> -- 
>> 2.11.0
>>

regards, rob

[Buildroot] [PATCH 2/2] Adding btrfs rootfs option.

[Yann E. MORIN]

Robert, All,

On 2018-08-22 14:10 +0100, Robert Heywood spake thusly:
> On 21/08/18 21:27, Yann E. MORIN wrote:
> >On 2018-08-21 17:04 +0100, Robert J. Heywood spake thusly:
> >>This patch makes it possible to format the rootfs using btrfs.
> >>Introduces the option; BR2_TARGET_ROOTFS_BTRFS
> >>
> >>When selected, the user is able to specify the filesystem size,
> >>label, options, and node and sector sizes.
> >>The new files are based on fs/ext2/{Config.in,ext2.mk}
> >
> >Nice addition! :-)
> >
> >>Signed-off-by: Robert J. Heywood <robert.heywood@codethink.co.uk>
> >[--SNIP--]
> >>+config BR2_TARGET_ROOTFS_BTRFS_SIZE_NODE
> >>+	string "btree node size"
> >>+	help
> >>+	  The tree block size in which btrfs stores metadata. The default
> >>+	  value is 16KiB (16384) or the page size, whichever is bigger.
> >
> >How is the page size detected? I'd like to be sure this is detecting the
> >page size of the target, not that of the build machine.
> Good question.
> I guess because the makefile creates a sparse file with fallocate which
> mkfs.btrfs then formats, it won't be able to detect any page size (host or
> otherwise) and so will always be set to a default value of 16384.
> I'll set that in the Config file explicitly.

So, the mkfs.btrfs is a host tool; it has no knowledge of the target at
all, so it can't possibly know the target page size. I believe the node
size should default to 16384, so that we get an explicit sane default.

Users who would change that value would know what they would be doing.

> >>+config BR2_TARGET_ROOTFS_BTRFS_SIZE_SECTOR
> >>+	string "sector size"
> >>+	help
> >>+	  The default value is the page size and is autodetected. If the
> >>+	  sectorsize differs from the page size, the created filesystem
> >>+	  may not be mountable by the kernel. Therefore it is not
> >>+	  recommended to use this option unless you are going to mount it
> >>+	  on a system with the appropriate page size.
> >
> >Given that the filsystem is specifically made for the target that will
> >mount it, does it make sense to ffer that option at all? I.e. if the
> >target uses a 4KiB page size, does it make sense to generatea rootfs
> >that has a different secotr size, and thus not mountable on the target
> >at all?
> >
> >My suggestion: don't add that option.
> >
> 
> On my system (building for a raspi0) it autodetects the sector size to be
> 4k, and the resulting image is bootable. However I'm worried the host won't
> be able to detect the correct value for the target. In such a case I can see
> why it would be useful to set the sector size explicitly.

Well, basically all architectures have a 4K page size, so does your
x86_64 and so does the rpi (0/1/2/3), so the default for your host is
valid for your target.

What architectures have a page-size that is not 4K?

> Is there another place where the sector size is specified? If so, it might
> be better to use the existing value instead.

I would also add a default here, to 4096. Users who want to change
that value would know what they would be doing.

> >>+config BR2_TARGET_ROOTFS_BTRFS_FEATURES
> >>+	string "Filesystem Features"
> >>+	help
> >>+	  A comma separated string of features that can be enabled
> >>+	  during creation time.
> >>+	  For a list of available options, use; `mkfs.btrfs -O list-all`
> >
> >Nice. I guess that has to be done using the host-btrfs we built, as its
> >feature list may be different than the one from the build machine.
> 
> I can change the help text to include the full path to the binary.

    For a list of available options, see:
        .../host/sbin/mkfs.btrfs -O list-all

[--SNIP--]
> >Is fallocate really necessary here? If the host's filesystem is too
> >small to accomodate the image, then the mkfs.btrfs below fill fail; we
> >don't need fallocate to fail.
> >
> >Besides, is fallocate readilly available on all distros?
> >
> >Usually, we use truncate(1) to that effect; see for example:
> >     support/testing/tests/fs/test_ubi.py at 29
> >
> 
> fallocate is used here because (unlike other mkfs programs) you can't
> specify a size and have mkfs.btrfs create an image file for you.

Yeah, I know. I was just questionning the use of fallocate for that,
rather than the usual truncate we use everywhere else.

> It needs a
> sparse file to work with.

Except fallocate will not create a sparse file at all; it will really
allocate all blocks the file needs.

> I'll be happy to change this to truncate, as per your example.

Yes, please, for consistency with the other parts of the code in
Buildroot.

Thanks for the feedback; eager to read your v2. ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2] Adding btrfs rootfs option.

[Yann E. MORIN]

Robert, All,

On 2018-08-22 18:57 +0200, Yann E. MORIN spake thusly:
> On 2018-08-22 14:10 +0100, Robert Heywood spake thusly:
> > On 21/08/18 21:27, Yann E. MORIN wrote:
[--SNIP--]
> > >Given that the filsystem is specifically made for the target that will
> > >mount it, does it make sense to ffer that option at all? I.e. if the
> > >target uses a 4KiB page size, does it make sense to generatea rootfs
> > >that has a different secotr size, and thus not mountable on the target
> > >at all?
> > >
> > >My suggestion: don't add that option.
> > 
> > On my system (building for a raspi0) it autodetects the sector size to be
> > 4k, and the resulting image is bootable. However I'm worried the host won't
> > be able to detect the correct value for the target. In such a case I can see
> > why it would be useful to set the sector size explicitly.
> 
> Well, basically all architectures have a 4K page size, so does your
> x86_64 and so does the rpi (0/1/2/3), so the default for your host is
> valid for your target.
> 
> What architectures have a page-size that is not 4K?

After discussing on IRC with Paul (Cc), it turns out that quite a few
architectures can have a page size different from 4K, like mips, arc and
a few others. Sometimes it is even configrable. Thanks Paul.

4K just happens to be the most common, but definitely not the only one.

> > Is there another place where the sector size is specified? If so, it might
> > be better to use the existing value instead.
> I would also add a default here, to 4096. Users who want to change
> that value would know what they would be doing.

So yeah, just keep that option, and let set the default to 4K. People
who have a non 4K arch or kernel option will know to change that value.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'