Sponsored by: Juniper Networks, Inc.
Sponsored by: Klara, Inc.
Details
Details
- Reviewers
imp pauamma_gundo.com - Group Reviewers
Klara manpages - Commits
- rG69d94f4c7608: Add tarfs, a filesystem backed by tarballs.
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| sys/fs/tarfs/tarfs_io.c | ||
|---|---|---|
| 630 | There is now a LOR here which will have to be addressed... | |
| sys/fs/tarfs/tarfs_vfsops.c | ||
|---|---|---|
| 1031 | It's something that is done by most (all?) the file systems in the tree. See the various unmount functions in the sources under sys/fs/ I don't recall what the reasoning was for it. | |
| sys/fs/tarfs/tarfs_io.c | ||
|---|---|---|
| 420 | Yes, you need to rangelock the whole file. Or at least, from the point where you start reading, to the MAX_OFF_T. | |
| 632 | These two lines can be written as vput() | |
| sys/fs/tarfs/tarfs_vfsops.c | ||
| 1031 | ||
| sys/fs/tarfs/tarfs_vnops.c | ||
| 210 | The only invariant that prevents filesystem unmount in lookup is the locked vnode. After the vnode is unlocked, other thread might unmount fs, and then VFS_VGET() would operate on the free memory, specifically *mp and *(mp->mnt_data). vn_vget_ino() busies mp in a way that avoids LoR between vnode lock and busy state. Busy filesystem cannot be unmounted. While busy, vn_vget_ino() does VFS_VGET() ensuring that we do not operate on the freed memory. | |
| share/man/man5/tarfs.5 | ||
|---|---|---|
| 58 | Why is this tunable, and not a mount option? | |
| sys/fs/tarfs/tarfs.h | ||
| 132 | I was not able to find the use or initialization to non-NULL values, for both cp and dev members. What for are they? | |
| sys/fs/tarfs/tarfs_io.c | ||
| 2 | New files should come with SPDX-License-Identifier there. No idea where to look for the guidance, might be the committers guide has some info. | |
| 29 | I believe the convention now is to not add $FreeBSD$ tags (and sys/cdefs.h) for new files. | |
| 117 | LK_SHARED is enough for VOP_READ | |
| 119 | This is wrong lock order, range lock is before vnode lock. | |
| 253 | LK_SHARED is enough for VOP_ACCESS | |
| 277 | LK_SHARED is enough for VOP_GETATTR | |
| 280 | It is better to unlock the vnode right after VOP_GETATTR(), there is no point of copying data from local va under the vnode lock. Don't you leak the vnode lock on VOP_GETATTR() error? | |
| 323 | uiomove() under the vnode lock is potential deadlock, when e.g. user sets up the buffer as backed by the vnode itself. Can zio node be mapped in the userspace? Is it writeable? It seems that it is accessible by userspace? | |
| 416 | malloc() under the vnode lock is potential deadlock in pagedaemon, which might need to write out or free pages owned by the locked vnode, to satisfy the allocation. BTW, why do you need the exclusive lock there? | |
| 425 | This is LoR, rangelock should be taken before the vnode lock. | |
| sys/fs/tarfs/tarfs_vfsops.c | ||
| 1109 | vn_lock/vref is better written as vget() | |
| share/man/man5/tarfs.5 | ||
|---|---|---|
| 5–24 | Maybe he means section 1 of https://docs.freebsd.org/en/articles/license-guide/ ? I need to update that article (there's currently like 5 places we recommend stuff). Same answer below. | |
| share/man/man5/tarfs.5 | ||
|---|---|---|
| 5–24 | Yes, that's what I meant. And this isn't the only time I confused those. | |
| share/man/man5/tarfs.5 | ||
|---|---|---|
| 5–24 | I'm sorry but this does not answer my question. What, specifically, do you want me to change? | |
| sys/fs/tarfs/tarfs.h | ||
| 132 | Leftovers from an earlier version of the driver. | |
| sys/fs/tarfs/tarfs_io.c | ||
| 323 | It's read-only, accessible by userspace for debugging purposes. I can remove that if it causes trouble. | |
| share/man/man5/tarfs.5 | ||
|---|---|---|
| 5–24 | Decide (based on imp's link) whether the license should just be a copyright notice followed by the SPDX-License-Identifier line, without the explicit language, since that's the preferred license for new files. Then adjust the license comment block if/as needed. (This may also be applicable to any new source files; my focus as a rule is on documentation only.) | |
Comment Actions
Incorporate further review feedback.
Stop playing games with the buffer cache. We now perform decompression in
tarfs_zread(), and tarfs_zstrategy() calls that, instead of the other way
around.
Comment Actions
Manual page changes LGTM, but did some unrelated changes leak into this?
| sys/conf/files | ||
|---|---|---|
| 175 | I was paging past the manual page changes and this caught my eye, probably because the connection with this change isn't obvious to me. Did an unrelated change leak into this? | |
| 194 | Same here. | |
| 1741 | And here, and maybe in other places. | |
| sys/conf/files | ||
|---|---|---|
| 175 | There is no change here. | |
| sys/conf/files | ||
|---|---|---|
| 175 | Uh. Either I was hallucinating, or Phabricator claimed ${NO_WINFINITE_RECURSION} was added. My money is on the latter. But either way, no change shows here now. | |
| share/man/man5/tarfs.5 | ||
|---|---|---|
| 58 | I think it's something you want to tune system-wide, not per-mount. But I suppose I can add a mount option that overrides the tunable. Or remove it entirely, as I'm not sure how useful it really is. | |