On Fri, Nov 15, 2024 at 05:31:38PM -0800, Hugh Dickins wrote:
On Fri, 15 Nov 2024, Chuck Lever wrote:
As I said before, I've failed to find any file system getattr method that explicitly takes the inode's semaphore around a generic_fillattr() call. My understanding is that these methods assume that their caller handles appropriate serialization. Therefore, taking the inode semaphore at all in shmem_getattr() seems to me to be the wrong approach entirely.
The point of reverting immediately is that any fix can't possibly get the review and testing it deserves three days before v6.12 becomes final. Also, it's not clear what the rush to fix the KCSAN splat is; according to the Fixes: tag, this issue has been present for years without causing overt problems. It doesn't feel like this change belongs in an -rc in the first place.
Please revert d949d1d14fa2, then let's discuss a proper fix in a separate thread. Thanks!
Thanks so much for reporting this issue, Chuck: just in time.
I agree abso-lutely with you: I was just preparing a revert, when I saw that akpm is already on it: great, thanks Andrew.
Thanks to you both for jumping on this!
I was not very keen to see that locking added, just to silence a syzbot sanitizer splat: added where there has never been any practical problem (and the result of any stat immediately stale anyway). I was hoping we might get a performance regression reported, but a hang serves better!
If there's a "data_race"-like annotation that can be added to silence the sanitizer, okay. But more locking? I don't think so.
IMHO the KCSAN splat suggests there is a missing inode_lock/unlock pair /somewhere/. Just not in shmem_getattr().
Earlier in this thread, Jeongjun reported:
... I found that this data-race mainly occurs when vfs_statx_path does not acquire the inode_lock ...
That sounds to me like a better place to address this; or at least that is a good place to start looking for the problem.