On Sat, 20 Sept 2025 at 00:42, Al Viro <v...@zeniv.linux.org.uk> wrote:
>
> The branch is -rc5-based; it lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.persistency
I reacted to the "d_make_persistent() + dput()" pattern, and wondered
if it should just use the refcount that the caller has, but it does
look like that alternate approach would just result in a
"d_make_persistent(dget()))" pattern instead.
And I guess you already get the lock for d_make_persistent(), so it's
better to do the dget while having it - but arguably that is also true
for the dput().
I think you did pick the right model, with d_make_persistent() taking
a ref, and d_make_discardable() releasing it, but this series did make
me think that the refcounting on the caller side is a bit odd.
Because some places would clearly want a "d_make_persistent_and_put()"
function. But probably not worth the extra interface.
Anyway, apart from that I only had one reaction: I think
d_make_discardable() should have a
WARN_ON(!(dentry->d_flags & DCACHE_PERSISTENT))
because without that I think it can mistakenly be used as some kind of
"dput that always takes the dentry lock", which seems bad.
Or was that intentional for some reason?
Talking about WARN_ON() - please don't add new BUG_ON() cases. I
realize that those will never trigger, but *if* they were to trigger,
some of them would do so during early boot and be a pain for people to
ever even report to us.
BUG_ON() really should be shunned. I think it makes sense to you and
for automated testing, but it really is absolutely *horrendously* bad
for the case where the code hits actual users.
Linus
