Page MenuHomeFreeBSD

Add tarfs, a filesystem backed by tarballs.
ClosedPublic

Authored by des on Dec 20 2022, 1:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, May 28, 5:59 PM
Unknown Object (File)
May 23 2024, 5:16 PM
Unknown Object (File)
May 7 2024, 11:57 AM
Unknown Object (File)
May 3 2024, 7:05 AM
Unknown Object (File)
May 2 2024, 8:30 AM
Unknown Object (File)
Apr 26 2024, 11:27 PM
Unknown Object (File)
Apr 26 2024, 8:51 AM
Unknown Object (File)
Apr 26 2024, 8:51 AM

Details

Summary

Sponsored by: Juniper Networks, Inc.
Sponsored by: Klara, Inc.

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
des marked 20 inline comments as done.

First round of review.

sys/fs/tarfs/tarfs_vnops.c
210

Like most of VFS, these are completely undocumented. Can you elaborate?

633

Most definitely not.

Address locking issues.

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.

des marked 14 inline comments as done.Jan 13 2023, 11:57 AM

Catch up with VFS changes.

Fix further locking issues and add a test.

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

Can you please be more specific?

sys/fs/tarfs/tarfs_io.c
2

Can you please be more specific?

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.

des marked 10 inline comments as done.

Incorporate further review feedback.

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.

sys/fs/tarfs/tarfs.h
132

Why did you kept them?

134

What is this vnode for? If it is used, please add a comment.

186
sys/fs/tarfs/tarfs_io.c
253

This is not handled despite marked as such.

277

Same.

280

Again not handled.

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.)

des marked an inline comment as done.Jan 24 2023, 8:41 AM
des added inline comments.
sys/fs/tarfs/tarfs.h
132

I've removed them from my branch, but I haven't updated the review yet because the current version panics.

134

It's the tarball.

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.

des marked 5 inline comments as done.Jan 30 2023, 4:47 PM

Add SPDX tags and consistently use the same license text.

des marked 7 inline comments as done.Jan 30 2023, 5:02 PM

Fix unused var in !TARFS_DEBUG case.

approved for license stuff.

share/man/man5/tarfs.5
4

these look good.

This revision is now accepted and ready to land.Jan 30 2023, 10:05 PM

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.

des marked 4 inline comments as done.Jan 31 2023, 1:22 PM
des added inline comments.
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.

des marked 2 inline comments as done.Feb 2 2023, 4:28 PM
des marked an inline comment as done.Feb 2 2023, 4:32 PM
des added inline comments.
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.

This revision was automatically updated to reflect the committed changes.