Re: [Cluster-devel] [PATCH v2 00/23] fs-verity support for XFS
On Tue, Apr 11, 2023 at 07:33:19PM -0700, Eric Biggers wrote: > On Mon, Apr 10, 2023 at 10:19:46PM -0700, Christoph Hellwig wrote: > > Dave is going to hate me for this, but.. > > > > I've been looking over some of the interfaces here, and I'm starting > > to very seriously questioning the design decisions of storing the > > fsverity hashes in xattrs. > > > > Yes, storing them beyond i_size in the file is a bit of a hack, but > > it allows to reuse a lot of the existing infrastructure, and much > > of fsverity is based around it. So storing them in an xattrs causes > > a lot of churn in the interface. And the XFS side with special > > casing xattr indices also seems not exactly nice. > > It seems it's really just the Merkle tree caching interface that is causing > problems, as it's currently too closely tied to the page cache? That is just > an > implementation detail that could be reworked along the lines of what is being > discussed. > > But anyway, it is up to the XFS folks. Keep in mind there is also the option > of > doing what btrfs is doing, where it stores the Merkle tree separately from the > file data stream, but caches it past i_size in the page cache at runtime. Right. It's not entirely simple to store metadata on disk beyond EOF in XFS because of all the assumptions throughout the IO path and allocator interfaces that it can allocate space beyond EOF at will and something else will clean it up later if it is not needed. This impacts on truncate, delayed allocation, writeback, IO completion, EOF block removal on file close, background garbage collection, ENOSPC/EDQUOT driven space freeing, etc. Some of these things cross over into iomap infrastructure, too. AFAIC, it's far more intricate, complex and risky to try to store merkle tree data beyond EOF than it is to put it in an xattr namespace because IO path EOF handling bugs result in user data corruption. This happens over and over again, no matter how careful we are about these aspects of user data handling. OTOH, putting the merkle tree data in a different namespace avoids these issues completely. Yes, we now have to solve an API mismatch, but we aren't risking the addition of IO path data corruption bugs to every non-fsverity filesystem in production... Hence I think copying the btrfs approach (i.e. only caching the merkle tree data in the page cache beyond EOF) would be as far as I think we'd want to go. Realistically, there would be little practical difference between btrfs storing the merkle tree blocks in a separate internal btree and XFS storing them in an internal private xattr btree namespace. I would, however, prefer not to have to do this at all if we could simply map the blocks directly out of the xattr buffers as we already do internally for all the XFS code... > I guess there is also the issue of encryption, which hasn't come up yet since > we're talking about fsverity support only. The Merkle tree (including the > fsverity_descriptor) is supposed to be encrypted, just like the file contents > are. Having it be stored after the file contents accomplishes that easily... > Of course, it doesn't have to be that way; a separate key could be derived, or > the Merkle tree blocks could be encrypted with the file contents key using > indices past i_size, without them physically being stored in the data stream. I'm expecting that fscrypt for XFS will include encryption of the xattr names and values (just like we will need to do for directory names) except for the xattrs that hold the encryption keys themselves. That means the merkle tree blocks should get encrypted without any extra work needing to be done anywhere. This will simply require the fscrypt keys to be held in a private internal xattr namespace that isn't encrypted, but that's realtively trivial to do... Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: [Cluster-devel] [PATCH v2 00/23] fs-verity support for XFS
On Mon, Apr 10, 2023 at 10:19:46PM -0700, Christoph Hellwig wrote: > Dave is going to hate me for this, but.. > > I've been looking over some of the interfaces here, and I'm starting > to very seriously questioning the design decisions of storing the > fsverity hashes in xattrs. > > Yes, storing them beyond i_size in the file is a bit of a hack, but > it allows to reuse a lot of the existing infrastructure, and much > of fsverity is based around it. So storing them in an xattrs causes > a lot of churn in the interface. And the XFS side with special > casing xattr indices also seems not exactly nice. It seems it's really just the Merkle tree caching interface that is causing problems, as it's currently too closely tied to the page cache? That is just an implementation detail that could be reworked along the lines of what is being discussed. But anyway, it is up to the XFS folks. Keep in mind there is also the option of doing what btrfs is doing, where it stores the Merkle tree separately from the file data stream, but caches it past i_size in the page cache at runtime. I guess there is also the issue of encryption, which hasn't come up yet since we're talking about fsverity support only. The Merkle tree (including the fsverity_descriptor) is supposed to be encrypted, just like the file contents are. Having it be stored after the file contents accomplishes that easily... Of course, it doesn't have to be that way; a separate key could be derived, or the Merkle tree blocks could be encrypted with the file contents key using indices past i_size, without them physically being stored in the data stream. - Eric
[Cluster-devel] gfs2-utils 3.5.1 released
gfs2-utils contains the tools needed to create, check, modify and inspect gfs2 filesystems along with support scripts needed on every gfs2 cluster node. This minor release includes fixes for a small number of issues discovered in 3.5.0, including test failures that occur on 32-bit architectures and when compiling with specific gcc versions with link-time optimization enabled. The full git shortlog is below. Release tarballs and signed checksums can be found here: https://releases.pagure.org/gfs2-utils/ Direct link for convenience: https://releases.pagure.org/gfs2-utils/gfs2-utils-3.5.1.tar.gz Please report bugs to the cluster-devel@redhat.com mailing list or at: https://bugzilla.kernel.org/enter_bug.cgi?product=File%20System=gfs2 Patches or pull requests can be sent to the same list or submitted on Pagure: https://pagure.io/gfs2-utils Thanks, Andy gfs2-utils changes since 3.5.0: Andrew Price (9): Update version for development gfs2_edit: Fix savemeta test failures in 32-bit environments gfs2_jadd: Fix format string warnings on 32-bit libgfs2: Fix strict-aliasing warning in lgfs2_rgrp_bitbuf_alloc gfs2_convert: Clean up strict-aliasing warnings Re-enable -Wstrict-aliasing libgfs2: Separate gfs and gfs2 code in lgfs2_sb_out() Don't use char arrays as temporary buffers Prepare for version 3.5.1
[Cluster-devel] [PATCH dlm-tool 1/4] fence: make pkg-config binary as passable make var
This patch defines PKG_CONFIG make var which could be overwrite by the user like it's the case for dlm_controld Makefile. --- fence/Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fence/Makefile b/fence/Makefile index ee4dfb88..894f6396 100644 --- a/fence/Makefile +++ b/fence/Makefile @@ -19,7 +19,10 @@ CFLAGS += -D_GNU_SOURCE -O2 -ggdb \ CFLAGS += -fPIE -DPIE CFLAGS += -I../include -CFLAGS += $(shell pkg-config --cflags pacemaker-fencing) + +PKG_CONFIG ?= pkg-config + +CFLAGS += $(shell $(PKG_CONFIG) --cflags pacemaker-fencing) LDFLAGS += -Wl,-z,relro -Wl,-z,now -pie LDFLAGS += -ldl -- 2.31.1
[Cluster-devel] [PATCH dlm-tool 2/4] fence: move pkg-config fail to bin target
Currently make (GNU Make 4.2.1) will fail if pkg-config fails even if it's not necessary, e.g. make clean target. We move the make bailout if pkg-config failed to the binary target when pkg-config result is necessary. Note: this is using $(.SHELLSTATUS) which is only available on GNU Make version 4.2 or above. --- fence/Makefile | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/fence/Makefile b/fence/Makefile index 894f6396..263657ec 100644 --- a/fence/Makefile +++ b/fence/Makefile @@ -21,8 +21,10 @@ CFLAGS += -fPIE -DPIE CFLAGS += -I../include PKG_CONFIG ?= pkg-config +PKG_CONFIG_FLAGS := --errors-to-stdout -CFLAGS += $(shell $(PKG_CONFIG) --cflags pacemaker-fencing) +PACEMAKER_CFLAGS := $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --cflags pacemaker-fencing) +PACEMAKER_CFLAGS_STATUS := $(.SHELLSTATUS) LDFLAGS += -Wl,-z,relro -Wl,-z,now -pie LDFLAGS += -ldl @@ -30,7 +32,10 @@ LDFLAGS += -ldl all: $(BIN_TARGET) $(BIN_TARGET): $(BIN_SOURCE) - $(CC) $(BIN_SOURCE) $(CFLAGS) $(LDFLAGS) -o $@ -L. +ifneq ($(PACEMAKER_CFLAGS_STATUS),0) + $(error "Failed to call pkg-config for pacemaker cflags: $(PACEMAKER_CFLAGS)") +endif + $(CC) $(BIN_SOURCE) $(CFLAGS) $(PACEMAKER_CFLAGS) $(LDFLAGS) -o $@ -L. clean: rm -f *.o *.so *.so.* $(BIN_TARGET) -- 2.31.1
[Cluster-devel] [PATCH dlm-tool 3/4] dlm_controld: move pkg-config fail to bin target
Currently make (GNU Make 4.2.1) will fail if pkg-config fails even if it's not necessary, e.g. make clean target. We move the make bailout if pkg-config failed to the binary target when pkg-config result is necessary. Note: this is using $(.SHELLSTATUS) which is only available on GNU Makefile version 4.2 or above. --- dlm_controld/Makefile | 27 +-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/dlm_controld/Makefile b/dlm_controld/Makefile index 9cf7152f..44715982 100644 --- a/dlm_controld/Makefile +++ b/dlm_controld/Makefile @@ -54,20 +54,35 @@ BIN_LDFLAGS += -lpthread -lrt -lcpg -lcmap -lcfg -lquorum -luuid LIB_LDFLAGS += $(LDFLAGS) -Wl,-z,relro -Wl,-z,now -pie PKG_CONFIG ?= pkg-config +PKG_CONFIG_FLAGS := --errors-to-stdout + ifeq ($(USE_SD_NOTIFY),yes) - BIN_CFLAGS += $(shell $(PKG_CONFIG) --cflags libsystemd) \ + SYSTEMD_CFLAGS := $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --cflags libsystemd) \ -DUSE_SD_NOTIFY - BIN_LDFLAGS += $(shell $(PKG_CONFIG) --libs libsystemd) + SYSTEMD_CFLAGS_STATUS := $(.SHELLSTATUS) + SYSTEMD_LDFLAGS := $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --libs libsystemd) + SYSTEMD_LDFLAGS_STATUS := $(.SHELLSTATUS) +else + SYSTEMD_CFLAGS_STATUS := 0 + SYSTEMD_LDFLAGS_STATUS := 0 endif -ifeq (, $(shell $(PKG_CONFIG) --libs "libquorum >= 3.1.0")) -$(error "Requires libquorum at least version 3.1.0") -endif +CPG_LDFLAGS := $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --libs "libquorum >= 3.1.0") +CPG_LDFLAGS_STATUS := $(.SHELLSTATUS) all: $(LIB_TARGET) $(BIN_TARGET) $(LIB_PC) $(BIN_TARGET): $(BIN_SOURCE) - $(CC) $(BIN_SOURCE) $(BIN_CFLAGS) $(BIN_LDFLAGS) -o $@ -L. +ifneq ($(CPG_LDFLAGS_STATUS),0) + $(error "Failed to call pkg-config for corosync ldflags: $(CPG_LDFLAGS)") +endif +ifneq ($(SYSTEMD_CFLAGS_STATUS),0) + $(error "Failed to call pkg-config for systemd cflags: $(SYSTEMD_CFLAGS)") +endif +ifneq ($(SYSTEMD_LDFLAGS_STATUS),0) + $(error "Failed to call pkg-config for systemd ldflags: $(SYSTEMD_LDFLAGS)") +endif + $(CC) $(BIN_SOURCE) $(BIN_CFLAGS) $(SYSTEMD_CFLAGS) $(BIN_LDFLAGS) $(SYSTEMD_LDFLAGS) -o $@ -L. $(LIB_TARGET): $(LIB_SOURCE) $(CC) $^ $(LIB_CFLAGS) $(LIB_LDFLAGS) -shared -o $@ -Wl,-soname=$(LIB_SMAJOR) -- 2.31.1
[Cluster-devel] [PATCH dlm-tool 4/4] dlm_controld: use pkg-config to find corosync libs
Currently all corosync dependencies are hardcorded inside the dlm_controld Makefile. We will determine those dependencies by pkg-config. This allows us that dlm_controld can detect when the corosync libaries are installed in a different place determined by changing the PKG_CONFIG_PATH env variable. --- dlm_controld/Makefile | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/dlm_controld/Makefile b/dlm_controld/Makefile index 44715982..2d97453a 100644 --- a/dlm_controld/Makefile +++ b/dlm_controld/Makefile @@ -50,7 +50,7 @@ BIN_CFLAGS += -I../include -I../libdlm LIB_CFLAGS += $(CFLAGS) -fPIC -fplugin=annobin BIN_LDFLAGS += $(LDFLAGS) -Wl,-z,relro -Wl,-z,now -pie -BIN_LDFLAGS += -lpthread -lrt -lcpg -lcmap -lcfg -lquorum -luuid +BIN_LDFLAGS += -lpthread -lrt -luuid LIB_LDFLAGS += $(LDFLAGS) -Wl,-z,relro -Wl,-z,now -pie PKG_CONFIG ?= pkg-config @@ -67,12 +67,18 @@ else SYSTEMD_LDFLAGS_STATUS := 0 endif -CPG_LDFLAGS := $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --libs "libquorum >= 3.1.0") +CPG_CFLAGS := $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --cflags libcpg libcmap libcfg "libquorum >= 3.1.0") +CPG_CFLAGS_STATUS := $(.SHELLSTATUS) + +CPG_LDFLAGS += $(shell $(PKG_CONFIG) $(PKG_CONFIG_FLAGS) --libs libcpg libcmap libcfg "libquorum >= 3.1.0") CPG_LDFLAGS_STATUS := $(.SHELLSTATUS) all: $(LIB_TARGET) $(BIN_TARGET) $(LIB_PC) $(BIN_TARGET): $(BIN_SOURCE) +ifneq ($(CPG_CFLAGS_STATUS),0) + $(error "Failed to call pkg-config for corosync cflags: $(CPG_CFLAGS)") +endif ifneq ($(CPG_LDFLAGS_STATUS),0) $(error "Failed to call pkg-config for corosync ldflags: $(CPG_LDFLAGS)") endif @@ -82,7 +88,7 @@ endif ifneq ($(SYSTEMD_LDFLAGS_STATUS),0) $(error "Failed to call pkg-config for systemd ldflags: $(SYSTEMD_LDFLAGS)") endif - $(CC) $(BIN_SOURCE) $(BIN_CFLAGS) $(SYSTEMD_CFLAGS) $(BIN_LDFLAGS) $(SYSTEMD_LDFLAGS) -o $@ -L. + $(CC) $(BIN_SOURCE) $(BIN_CFLAGS) $(CPG_CFLAGS) $(SYSTEMD_CFLAGS) $(BIN_LDFLAGS) $(CPG_LDFLAGS) $(SYSTEMD_LDFLAGS) -o $@ -L. $(LIB_TARGET): $(LIB_SOURCE) $(CC) $^ $(LIB_CFLAGS) $(LIB_LDFLAGS) -shared -o $@ -Wl,-soname=$(LIB_SMAJOR) -- 2.31.1
Re: [Cluster-devel] [PATCH v2 00/23] fs-verity support for XFS
Dave is going to hate me for this, but.. I've been looking over some of the interfaces here, and I'm starting to very seriously questioning the design decisions of storing the fsverity hashes in xattrs. Yes, storing them beyond i_size in the file is a bit of a hack, but it allows to reuse a lot of the existing infrastructure, and much of fsverity is based around it. So storing them in an xattrs causes a lot of churn in the interface. And the XFS side with special casing xattr indices also seems not exactly nice.
