vfs: stop using SAVESTART for rename
ClosedPublic
Actions

Authored by mjg on Mar 7 2022, 2:45 PM.

Details

Reviewers

mckusick
markj
kib

Commits

rG56da4aa5545f: vfs: stop using SAVESTART for rename
rG8f874e92ebc9: vfs: make relookup take an additional argument

Summary

As far as I can say ni_startdir is of no significance for rename. The target buf is used, but that one is covered with SAVENAME.

ufs rename asserts that SAVESTART is passed, but the assert dates back to the original BSD source import and I don't see it to be of any use here. msdosfs had the same assert copy-pasted.

The goal is to eradicate the SAVESTART flag.

This is 2 commits squashed into one review:

commit f32b40a60e76332951e6dabc9977bd7a76e06d48 (HEAD -> kekeacz)
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Mon Mar 7 13:50:45 2022 +0100

    vfs: stop using SAVESTART for rename
    
    ni_startdir has never reached rename routines anyway. Do pass SAVENAME
    instead as the name is used for relookup (if needed).

commit 4b965c66044dfb3a3849db5e5fd5397408c9e244
Author: Mateusz Guzik <mjg@FreeBSD.org>
Date:   Mon Mar 7 16:34:52 2022 +0100

    vfs: make relookup take an additional argument
    
    instead of looking at SAVESTART
    
    This is a step towards removing the flag.

Test Plan

will ask pho

ran several rename tests from stress2, no issues

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mjg requested review of this revision.Mar 7 2022, 2:45 PM

mjg created this revision.

hm, ext2 is using relookup which looks at SAVESTART. I may need to extend the patch, but the ufs and msdosfs remarks remain.

added relookup changes

mjg added inline comments.Mar 7 2022, 4:20 PM

sys/kern/vfs_lookup.c
1407	relookup would preferably drop this feature altogether -- interested parties can just vref on their own, but that's for later

This change looks reasonable to me.

sys/kern/vfs_lookup.c
1455	This panic should be more descriptive and should have the correct function name.

This revision is now accepted and ready to land.Mar 13 2022, 12:52 AM

markj added inline comments.Mar 14 2022, 6:12 PM

sys/kern/vfs_syscalls.c
3680	Looks like zfs_freebsd_rename() asserts SAVESTART is set, for some reason. Doesn't it need to be patched as well?

mjg added inline comments.Mar 14 2022, 6:52 PM

sys/kern/vfs_syscalls.c
3680	it asserts SAVENAME and/or SAVESTART, so it wont fire. I'm going to patch it when removing the flag to begin with.

mjg added inline comments.Mar 14 2022, 7:01 PM

sys/kern/vfs_syscalls.c
3680	maybe i should note that SAVESTART implies SAVENAME

markj added inline comments.Mar 14 2022, 8:28 PM

sys/fs/ext2fs/ext2_vnops.c
922	I'm having trouble understanding this. Without the change, kern_renameat() passes SAVESTART, so tdvp gets ref'ed in relookup(). Where does that ref get released?
sys/kern/vfs_lookup.c
1453	Does this comment still make sense?
sys/kern/vfs_syscalls.c
3680	Oops, I see.