mirror of
https://git.proxmox.com/git/mirror_zfs.git
synced 2024-11-17 10:01:01 +03:00
2e7f664f04
I recently gained the ability to run Clang's static analyzer on the linux kernel modules via a few hacks. This extended coverage to code that was previously missed since Clang's static analyzer only looked at code that we built in userspace. Running it against the Linux kernel modules built from my local branch produced a total of 72 reports against my local branch. Of those, 50 were reports of logic errors and 22 were reports of dead code. Since we already had cleaned up all of the previous dead code reports, I felt it would be a good next step to clean up these dead code reports. Clang did a further breakdown of the dead code reports into: Dead assignment 15 Dead increment 2 Dead nested assignment 5 The benefit of cleaning these up, especially in the case of dead nested assignment, is that they can expose places where our error handling is incorrect. A number of them were fairly straight forward. However several were not: In vdev_disk_physio_completion(), not only were we not using the return value from the static function vdev_disk_dio_put(), but nothing used it, so I changed it to return void and removed the existing (void) cast in the other area where we call it in addition to no longer storing it to a stack value. In FSE_createDTable(), the function is dead code. Its helper function FSE_freeDTable() is also dead code, as are the CPP definitions in `module/zstd/include/zstd_compat_wrapper.h`. We just delete it all. In zfs_zevent_wait(), we have an optimization opportunity. cv_wait_sig() returns 0 if there are waiting signals and 1 if there are none. The Linux SPL version literally returns `signal_pending(current) ? 0 : 1)` and FreeBSD implements the same semantics, we can just do `!cv_wait_sig()` in place of `signal_pending(current)` to avoid unnecessarily calling it again. zfs_setattr() on FreeBSD version did not have error handling issue because the code was removed entirely from FreeBSD version. The error is from updating the attribute directory's files. After some thought, I decided to propapage errors on it to userspace. In zfs_secpolicy_tmp_snapshot(), we ignore a lack of permission from the first check in favor of checking three other permissions. I assume this is intentional. In zfs_create_fs(), the return value of zap_update() was not checked despite setting an important version number. I see no backward compatibility reason to permit failures, so we add an assertion to catch failures. Interestingly, Linux is still using ASSERT(error == 0) from OpenSolaris while FreeBSD has switched to the improved ASSERT0(error) from illumos, although illumos has yet to adopt it here. ASSERT(error == 0) was used on Linux while ASSERT0(error) was used on FreeBSD since the entire file needs conversion and that should be the subject of another patch. dnode_move()'s issue was caused by us not having implemented POINTER_IS_VALID() on Linux. We have a stub in `include/os/linux/spl/sys/kmem_cache.h` for it, when it really should be in `include/os/linux/spl/sys/kmem.h` to be consistent with Illumos/OpenSolaris. FreeBSD put both `POINTER_IS_VALID()` and `POINTER_INVALIDATE()` in `include/os/freebsd/spl/sys/kmem.h`, so we copy what it did. Whenever a report was in platform-specific code, I checked the FreeBSD version to see if it also applied to FreeBSD, but it was only relevant a few times. Lastly, the patch that enabled Clang's static analyzer to be run on the Linux kernel modules needs more work before it can be put into a PR. I plan to do that in the future as part of the on-going static analysis work that I am doing. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes #14380 |
||
|---|---|---|
| .. | ||
| include | ||
| lib | ||
| README.md | ||
| zfs_zstd.c | ||
| zstd_sparc.c | ||
| zstd-in.c | ||
ZSTD-On-ZFS Library Manual
Introduction
This subtree contains the ZSTD library used in ZFS. It is heavily cut-down by dropping any unneeded files, and combined into a single file, but otherwise is intentionally unmodified. Please do not alter the file containing the zstd library, besides upgrading to a newer ZSTD release.
Tree structure:
zfs_zstd.care the actualzfskernel module hooks.lib/contains the unmodified version of theZstandardlibraryzstd-in.cis our template file for generating the single-file libraryinclude/: This directory contains supplemental includes for platform compatibility, which are not expected to be used by ZFS elsewhere in the future. Thus we keep them private to ZSTD.
Updating ZSTD
To update ZSTD the following steps need to be taken:
- Grab the latest release of ZSTD.
- Copy the files output by the following script to
module/zstd/lib/:grep include [path to zstd]/contrib/single_file_libs/zstd-in.c | awk '{ print $2 }' - Remove debug.c, threading.c, and zstdmt_compress.c.
- Update Makefiles with resulting file lists.
- Follow symbol renaming notes in
include/zstd_compat_wrapper.h
Altering ZSTD and breaking changes
If ZSTD made changes that break compatibility or you need to make breaking changes to the way we handle ZSTD, it is required to maintain backwards compatibility.
We already save the ZSTD version number within the block header to be used to add future compatibility checks and/or fixes. However, currently it is not actually used in such a way.