Cc: OGAWA Hirofumi <hirofumi@...>, Thomas Gleixner <tglx@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Linux Kernel Mailing List <linux-kernel@...>, <corbet@...>
Thanks, that was something I didn't test. I just tested various simple
file create/delete/read/write.
Yup.
And as far as I can tell there is absolutely nothing in fat_setattr() that
we actually want to protect - it calls things like fat_cont_expand(), but
that just calls down to the generic VFS layer that uses the pagecache
functions, and those need to handle the locking correctly for other
reasons (totally unrelated to setattr - they get called without locking
for regular writes, after all).
So it looks like the correct fix is to just remove the lock_super() in
fat_setattr() entirely (along with the "sb" variable that is then no
longer used).
It *also* turns out that we should remove the lock_super() from
fat_truncate: all the truncate paths already hold the inode mutex from the
VFS layer, so the inode data structures themselves would be serialized for
other reasons. And it only protects the call to "fat_free()", which in
turn calls the FAT cluster routines ("fatent") that already are protected
by the fatent spinlock.
More importantly, if it's a filesystem marked DIRSYNC, it will call
"fat_sync_inode()", which takes the superblock lock (and needs it, because
it's called frm the VFS layer too) and would deadlock there.
So the end result of that is all the "lock_kernel()" calls in
fs/fat/file.c should actually just go away - not be replaced by
lock_super() at alL!
Jonathan, do you want an updated replacement patch, or an incremental one,
or will you just do that trivial fix yourself?
Linus
--
