Skip to content

Conversation

@amotin
Copy link
Member

@amotin amotin commented Nov 24, 2025

Trying to investigate CI raidz_001_neg failures on FreeBSD 16 I ended up with few patches.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

The output is not so big here, so lets collect something useful.

Signed-off-by: Alexander Motin <[email protected]>
 - io_offset of 1 makes no sense.  Set default to 0.
 - Initialize io_offset in all cases.

Signed-off-by: Alexander Motin <[email protected]>
 - When filling ABDs of several segments, consider offset.
 - "Corrupt" ABDs with actually different data to fail something.

Signed-off-by: Alexander Motin <[email protected]>
It feels dirty to modify protection of a memory allocated via libc,
but at least we should try to restore it before freeing.

Signed-off-by: Alexander Motin <[email protected]>
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Nov 25, 2025
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is all pretty sensible. What a funny little program.

err = run_test(NULL);
}

mprotect(rand_data, SPA_MAXBLOCKSIZE, PROT_READ | PROT_WRITE);
Copy link
Member

@robn robn Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right about using mprotect() on memory obtained from libc (technically, wherever umem_alloc() got it from, which is kinda worse maybe?). POSIX explicitly says calling it on a pointer you didn't get from mmap() is unspecified, so.

I don't care for this PR, since we were already doing and your change is an improvement, but if you like we could just use an anonymous mapping:

diff --git a/cmd/raidz_test/raidz_test.c b/cmd/raidz_test/raidz_test.c
index 4839e909e4..eb1d6a562a 100644
--- a/cmd/raidz_test/raidz_test.c
+++ b/cmd/raidz_test/raidz_test.c
@@ -820,7 +820,8 @@ main(int argc, char **argv)
 	kernel_init(SPA_MODE_READ);
 
 	/* setup random data because rand() is not reentrant */
-	rand_data = (int *)umem_alloc(SPA_MAXBLOCKSIZE, UMEM_NOFAIL);
+	rand_data = (int *)mmap(NULL, SPA_MAXBLOCKSIZE, PROT_READ|PROT_WRITE,
+	    MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
 	srand((unsigned)time(NULL) * getpid());
 	for (i = 0; i < SPA_MAXBLOCKSIZE / sizeof (int); i++)
 		rand_data[i] = rand();
@@ -835,7 +836,7 @@ main(int argc, char **argv)
 		err = run_test(NULL);
 	}
 
-	umem_free(rand_data, SPA_MAXBLOCKSIZE);
+	munmap(rand_data, SPA_MAXBLOCKSIZE);
 	kernel_fini();
 
 	return (err);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, assuming using an anonymous mapping works as hoped this does seems preferable. Still I don't think we need to hold up this PR on that. We can always follow up with this.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 1, 2025
@behlendorf behlendorf closed this in 3647fa3 Dec 1, 2025
behlendorf pushed a commit that referenced this pull request Dec 1, 2025
 - io_offset of 1 makes no sense.  Set default to 0.
 - Initialize io_offset in all cases.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes #17977
behlendorf pushed a commit that referenced this pull request Dec 1, 2025
 - When filling ABDs of several segments, consider offset.
 - "Corrupt" ABDs with actually different data to fail something.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes #17977
behlendorf pushed a commit that referenced this pull request Dec 1, 2025
It feels dirty to modify protection of a memory allocated via libc,
but at least we should try to restore it before freeing.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Closes #17977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Accepted Ready to integrate (reviewed, tested)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants