On Tue, Aug 05, 2025 at 08:50:03PM +0100, Al Viro wrote:
> On Tue, Aug 05, 2025 at 04:34:57PM +0100, Al Viro wrote:
>
> > They do no allow to express things like "foo() consumes lock X".
> > >From time to time, we *do* need that, and when that happens guards
> > become a menace.
> >
> > Another case is
> > lock
> > if (lock-dependent condition)
> > some work
> > unlock
> > work that can't be under that lock
> > else
> > some other work
> > unlock
> > more work that can't be under that lock
> >
> > Fairly common, especially when that's a spinlock and "can't be under that
> > lock" includes blocking operations. Can't be expressed with guards, not
> > without a massage that often ends up with bloody awful results.
>
> FWIW, I'm looking through the raw data I've got during ->d_lock audit.
> Except for a few functions (all static in fs/dcache.c), all scopes
> terminate in the same function where they begin.
... and for ->file_lock we have the following:
expand_fdtable(): drops and regains
expand_files(): either nothing or drops and regains
do_dup2(): drops
everything else is neutral.
20 functions touching that stuff total. Convertible to guard() or
scoped_guard(): put_unused_fd(), fd_install(), close_fd() (scoped_guard
only), __range_cloexec(), file_close_fd(), set_close_on_exec(),
iterate_fd(), fcntl_setlk() and fcntl_setlk64() (scoped_guard only),
seq_show() - 10 out of 20.
Anything that uses expand_fdtable() is in the best case an abuse of
guard/scoped_guard; in the worst, it's actively confusing, since
there's *not* one continuous scope there. expand_files() is in
the same boat. That covers alloc_fd(), replace_fd() and ksys_dup3().
That's 5 out of remaining 10. BTW, trying to eliminate gotos from
alloc_fd() is not fun either.
dup_fd():
spin_lock(&oldf->file_lock);
...
while (unlikely(open_files > new_fdt->max_fds)) {
spin_unlock(&oldf->file_lock);
... (blocking, possibly return on failure here)
spin_lock(&oldf->file_lock);
...
}
...
spin_unlock(&oldf->file_lock);
...
No way to do that with guard() - not unless you mix it with explicit
unlock/lock in the middle of scope, and even that will be bitch to
deal with due to failure exit after allocation failure. We'd need
to do this:
spin_unlock(&oldf->file_lock);
if (new_fdt != &newf->fdtab)
__free_fdtable(new_fdt);
new_fdt = alloc_fdtable(open_files);
spin_lock(&oldf->file_lock);
if (IS_ERR(new_fdt)) {
kmem_cache_free(files_cachep, newf);
return ERR_CAST(new_fdt);
}
all of that under guard(spinlock)(&oldf->file_lock). IMO that would
be too confusing and brittle.
__range_close():
spin_lock(&files->file_lock);
...
for (; fd <= max_fd; fd++) {
file = file_close_fd_locked(files, fd);
if (file) {
spin_unlock(&files->file_lock);
filp_close(file, files);
cond_resched();
spin_lock(&files->file_lock);
} else if (need_resched()) {
spin_unlock(&files->file_lock);
cond_resched();
spin_lock(&files->file_lock);
}
}
spin_unlock(&files->file_lock);
Not a good fit for guard(), for the same reasons.
do_close_on_exec():
...
spin_lock(&files->file_lock);
for (i = 0; ; i++) {
....
for ( ; set ; fd++, set >>= 1) {
....
spin_unlock(&files->file_lock);
filp_close(file, files);
cond_resched();
spin_lock(&files->file_lock);
}
}
spin_unlock(&files->file_lock);
Same story.
io_close():
might be convertible to scoped_guard; won't be pretty,
AFAICS - that -EAGAIN case in the middle makes it very clumsy.
do_dup2(): well... we could lift filp_close() into the callers,
but that ends up with fairly unpleasant boilerplate in the
callers, and one of those callers is a fairly hot syscall.
And that's the remaining 5. For some locks scoped_guard() is
a decent fit; for some it really isn't ;-/
