Jan Kara <jack@xxxxxxx> wrote:
> Well, it seems like you are trying to get rid of the dependency
> sb_writers->mmap_sem. But there are other places where this dependency is
> created, in particular write(2) path is a place where it would be very
> difficult to get rid of it (you take sb_writers, then do all the work
> preparing the write and then you copy user data into page cache which
> may require mmap_sem).
>
> ...
>
> This is the problematic step - from quite deep in the locking chain holding
> invalidate_lock and having PG_Writeback set you suddently jump to very outer
> locking context grabbing sb_writers. Now AFAICT this is not a real deadlock
> problem because the locks are actually on different filesystems, just
> lockdep isn't able to see this. So I don't think you will get rid of these
> lockdep splats unless you somehow manage to convey to lockdep that there's
> the "upper" fs (AFS in this case) and the "lower" fs (the one behind
> cachefiles) and their locks are different.
I'm not sure you're correct about that. If you look at the lockdep splat:
> -> #2 (sb_writers#14){.+.+}-{0:0}:
The sb_writers lock is "personalised" to the filesystem type (the "#14"
annotation) which is set here:
for (i = 0; i < SB_FREEZE_LEVELS; i++) {
if (__percpu_init_rwsem(&s->s_writers.rw_sem[i],
sb_writers_name[i],
&type->s_writers_key[i])) <----
goto fail;
}
in fs/super.c.
I think the problem is (1) that on one side, you've got, say, sys_setxattr()
taking an sb_writers lock and then accessing a userspace buffer, which (a) may
take mm->mmap_lock and vma->vm_lock and (b) may cause reading or writeback
from the netfs-based filesystem via an mmapped xattr name buffer].
Then (2) on the other side, you have a read or a write to the network
filesystem through netfslib which may invoke the cache, which may require
cachefiles to check the xattr on the cache file and maybe set/remove it -
which requires the sb_writers lock on the cache filesystem.
So if ->read_folio(), ->readahead() or ->writepages() can ever be called with
mm->mmap_lock or vma->vm_lock held, netfslib may call down to cachefiles and
ultimately, it should[*] then take the sb_writers lock on the backing
filesystem to perform xattr manipulation.
[*] I say "should" because at the moment cachefiles calls vfs_set/removexattr
functions which *don't* take this lock (which is a bug). Is this an error
on the part of vfs_set/removexattr()? Should they take this lock
analogously with vfs_truncate() and vfs_iocb_iter_write()?
However, as it doesn't it manages to construct a locking chain via the
mapping.invalidate_lock, the afs vnode->validate_lock and something in execve
that I don't exactly follow.
I wonder if this is might be deadlockable by a multithreaded process (ie. so
they share the mm locks) where one thread is writing to a cached file whilst
another thread is trying to set/remove the xattr on that file.
David
