This repository has been archived by the owner on Feb 16, 2020. It is now read-only.
Untested ashift #3
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit tries to implement the ashift pool property as used in ZFSonLinux and the ZEVO beta releases. It is based on the ZFSonLinux implementation, and follows this comment: openzfs/zfs#195 (comment) and the resulting final patch for ZFSonLinux found here: openzfs/zfs@df30f56 The original inventors are - Darik Horn <[email protected]> - Richard Laager <[email protected]> - Christian Kohlschütter <[email protected]> Due to the large distance between macZFS and ZFSonLinux (pool version 8 vs. 28) it was necessary to redo and adapt their approach instead of just copying the changes. Note that this solution is technical not fully correct: (a) it introduces a pool property that should not exist in pool version 8 (b) it introduces a new property type, PROP_ONTIME that should not exist in pool version 8 (c) the ashift value is really a property of individual top-level vdevs, and not a pool-wide value. A pool can (and should under certain conditions) have top-level vdevs of different block size. However, using a pool-wide property is what other implementations do, so we do the same for the sake of compatibility.
Conflicts: usr/src/cmd/zpool/zpool_main.c usr/src/common/zfs/zfs_prop.c usr/src/uts/common/fs/zfs/dsl_prop.c usr/src/uts/common/sys/fs/zfs.h Integrated the ashift12 implementation for maczfs_74 into untested maczfs_78. This version compiles, but panics at disk attach. The backtrace dies in the file system mount path, and a test with the unmodified untested branch and a fresh disk image paniced also. So the panic is most likely not related to the ashift implementation.
|
I think the other one has superseded this one, hasn't it? |
|
No, this one is for the onnv / maczfs 78 (seven-eight) "untested". It is substantial different from the version in pull request #4 (and the closed #2) which is for maczfs 74. Thats why I suggested to make the maczfs_74-2 release from a side branch off master's maczfs_74-1 point. That way master stays clean for later merge with untested |
|
On 27 Aug 2012, at 22:58, BjoKaSH wrote:
OK. I need to look into more details why we're needing to do the chown/chmod at install time for the ZFS kernel extension though - that doesn't seem right to me. The installer package should be able to revert the permissions at install time, but a chown might cause issues (particularly if the kernel extension is already loaded, then replaced with a non-executable one). I've merged it in (and created a separate branch for it) but think it would be a good idea to cut a release from 41be385 for the initial 10.8 support, and then see why we need to do the recursive chown at install time in 93b251f I also have to dig out my 10.5 SDK which I don't have on this machine to do the build :( I'll get back to you with that when I've found it. I may even have to fire up my G5 to do the build for that part ... Alex |
|
The chmod might be an artifact of my machine. Everytime I run support/release.sh, the generated package had the wrong permissions set for zfs.kext and everything inside. This resulted on every try in a "The system extension zfs.kex is damaged and can not be used" message poping up (text translated back from German to English). I suggest you try to remove the chmod / post-install.sh and try to generate a package. If it works, then fine. I can try the package on my box if you like. I have a 10.5 binary (ppc + i386) ready, in case you need one. |
…_init() and txg_fini().
In zio_ready(), under some condition a blockpointer is copied into itself, i.e. bptr_t bp1, *bp2; bp2 = &bp; bp1 = *bp2; This produces a memcpy overlap error in valgrind. The real question of course is, if trying to copy the blockpointer into itself is a legitimate operation here. Not sure. Call tree for the offending copy: ==00:00:00:02.705 27867== Source and destination overlap in memcpy(0x1004b94f8, 0x1004b94f8, 128) ==00:00:00:02.705 27867== at 0x10016AD73: memcpy (mc_replace_strmem.c:523) ==00:00:00:02.705 27867== by 0x1000B954C: zio_ready (zio.c:919) ==00:00:00:02.705 27867== by 0x1000BE103: zio_next_stage (zio.c:2024) ==00:00:00:02.705 27867== by 0x1000B9192: zio_wait_for_children (zio.c:846) ==00:00:00:02.705 27867== by 0x1000B93BA: zio_wait_children_ready (zio.c:874) ==00:00:00:02.705 27867== by 0x1000BE38A: zio_next_stage_async (zio.c:2073) ==00:00:00:02.705 27867== by 0x1000B9111: zio_nowait (zio.c:833) ==00:00:00:02.705 27867== by 0x10009B7D8: vdev_label_read (vdev_label.c:168) ==00:00:00:02.705 27867== by 0x10009C69D: vdev_label_read_config (vdev_label.c:336) ==00:00:00:02.705 27867== by 0x100096FD3: vdev_validate (vdev.c:1009) ==00:00:00:02.705 27867== by 0x100096F80: vdev_validate (vdev.c:999) ==00:00:00:02.705 27867== by 0x100096F80: vdev_validate (vdev.c:999) ==00:00:00:02.705 27867==
In zap_lookup_norm(objset_t *os, uint64_t zapobj, const char *name,
uint64_t integer_size, uint64_t num_integers, void *buf,
matchtype_t mt, char *realname, int rn_len,
boolean_t *ncp)
strlcpy() is called with a size of 0, which results in undefined behavior.
Calltree (example):
==00:00:00:07.711 27867== Invalid write of size 1
==00:00:00:07.711 27867== at 0x10016AFFA: strlcpy (mc_replace_strmem.c:373)
==00:00:00:07.711 27867== by 0x1000AF568: zap_lookup_norm (zap_micro.c:683)
==00:00:00:07.711 27867== by 0x1000AF428: zap_lookup (zap_micro.c:645)
==00:00:00:07.711 27867== by 0x10007A78B: dsl_pool_open (dsl_pool.c:92)
==00:00:00:07.711 27867== by 0x100083AB0: spa_load (spa.c:1124)
==00:00:00:07.711 27867== by 0x100084538: spa_open_common (spa.c:1456)
==00:00:00:07.711 27867== by 0x100084783: spa_open (spa.c:1528)
==00:00:00:07.711 27867== by 0x100008404: main (zdb.c:2399)
==00:00:00:07.711 27867== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==00:00:00:07.711 27867==
==00:00:00:07.711 27867==
==00:00:00:07.711 27867== Process terminating with default action of signal 11 (SIGSEGV)
==00:00:00:07.711 27867== Access not within mapped region at address 0x0
Note that zap_lookup() *always* calls zap_lookup_norm() with a zero
size argument, which is correct. So zap_lookup_norm() must not depend
on implementation-defined behavior regarding strlcpy with zero size.
spa_add() does not fully initialize the spa structure, resulting in undefined behavior due to uninitialized memory later on. Calltree at time of crash ==00:00:02:46.240 31470== Process terminating with default action of signal 4 (SIGILL) ==00:00:02:46.240 31470== Illegal opcode at address 0x10023291D ==00:00:02:46.240 31470== at 0x10023291D: __abort (in /usr/lib/libSystem.B.dylib) ==00:00:02:46.240 31470== by 0x100232980: abort (in /usr/lib/libSystem.B.dylib) ==00:00:02:46.240 31470== by 0x1000A21E8: cv_wait (kernel.c:348) ==00:00:02:46.240 31470== by 0x10000D65B: ztest_suspend_monitor (ztest.c:3174) ==00:00:02:46.240 31470== by 0x10017D455: _pthread_start (in /usr/lib/libSystem.B.dylib) ==00:00:02:46.240 31470== by 0x10017D308: thread_start (in /usr/lib/libSystem.B.dylib) here the conditional variable spa->spa_zio_cv is used uninitialized, causing cv_wait to fail and the program to raising SIGABORT.
…_dirent_lock().
The call to zfs_dirent_lock() in zfs_create() was missing an
replacing the "char *name" parameter with Apples"componentname_t *cnp".
This triggered a kerel panic on file create.
panic trace:
0x21b455 <panic+445>: mov 0x8011d0,%eax
0x2a8ab2 <kernel_trap+1530>: jmp 0x2a8ace <kernel_trap+1558>
0x29e9a8 <lo_alltraps+712>: mov %edi,%esp
0x4d614177 <zfs_dirent_lock+95>: movzbl (%eax),%eax
0x4d6240ca <zfs_create+542>: mov %eax,-0x28(%ebp)
0x4d624820 <zfs_vnop_create+259>: leave
0x4d614177 is in zfs_dirent_lock (/Users/bj/new-mac-zfs-bjoka/usr/src/uts/common/fs/zfs/zfs_dir.c:197).
192 ASSERT(name[cnp->cn_namelen] == '\0');
193 #endif
194 /*
195 * Verify that we are not trying to lock '.', '..', or '.zfs'
196 */
197 if (name[0] == '.' &&
198 (name[1] == '\0' || (name[1] == '.' && name[2] == '\0')) ||
199 zfs_has_ctldir(dzp) && strcmp(name, ZFS_CTLDIR_NAME) == 0)
200 return (EEXIST);
201
0x4d6240ca is in zfs_create (/Users/bj/new-mac-zfs-bjoka/usr/src/uts/common/fs/zfs/zfs_vnops.c:1674).
1669 #ifndef __APPLE__
1670 if (flag & FIGNORECASE)
1671 zflg |= ZCILOOK;
1672 #endif
1673
1674 error = zfs_dirent_lock(&dl, dzp, name, &zp, zflg,
1675 NULL, NULL);
1676 if (error) {
1677 if (strcmp(name, "..") == 0)
1678 error = EISDIR;
Due to a typo, build/ZFS105 was cleaned twice and build/ZFS106 no at all.
…redits. Reworded most texts in the installer based on Daniel Bethe's suggestions. File CREDITS not added, need further work.
This imports the latests fixes from untested: d3df144 Fixed missing cv_init() / cv_destroy() calls in space_map_load(), txg_init() and txg_fini(). e85d2cc Fixed type mismatch between lib/libzpool/common/kernel.c and zfs_context.h 339b734 Fixed unneeded memcpy on it self in zio_ready(zio_t *zio). 9b24f0f Fixed potential NULL pointer dereference in zap_lookup_norm() 34cbc19 Fix a missing cv_init() cv_destroy() pair in spa_add() / spa_remove(). e35f948 Fixed missing #ifdef __APPLE__ around change name type in call to zfs_dirent_lock(). cb6a432 Fixed missing clean of ZFS106. 448671a Updated installer texts based on Daniel Bethe's suggestions, except credits. 6e5b7e5 Added option to specify module identifier on command line.
In usr/src/uts/common/fs/zfs/zfs_vnops.c was the vnops structure only half-defined, causing a NULL pointer dereference. All fifo specific functions had been #if ... #endif'ed out, leaving the function vector completely scrambled with NULL entries and other entries in the wrong place. This was probably due to a partial merge.
…_desc registration. The vnode operations structure zfs_fifoop_opv_desc was referenced in the setup of a fifo vnode, but was never been registered with the kernel. This adds the missing registration of zfs_fifoop_opv_desc.
* untested: Fixed crash on stat() call on a fifo, part II. Missing zfs_fifoop_opv_desc registration. Fixed crash on stat() call on a fifo, part I.
|
This has been merged into maczfs_78 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi Alex,
this implements an ashift property as in ZFonLinux. It is based on the implementation from ZFSonLinux, see also issue openzfs/zfs#280 in the ZFSonLinux repository openzfs/zfs@df30f56
I am quite certain that this feature is sound and solid. So I think it should go into the next stable release.