On Wed, Aug 2, 2023 at 10:49 AM Linus Torvalds torvalds@linux-foundation.org wrote:
On Tue, 1 Aug 2023 at 15:07, Suren Baghdasaryan surenb@google.com wrote:
To make locking more visible, change thesefunctions to assert that the vma write lock is taken and explicitly lock the vma beforehand.
So I obviously think this is a good change, but the fact that it touched driver files makes me go "we're still doing something wrong".
I'm not super-happy with hfi1_file_mmap() doing something like vma_start_write(), in that I *really* don't think drivers should ever have to think about issues like this.
And I think it's unnecessary. This is the mmap op in the hfi1_file_ops, and I think that any actual mmap() code had _better_ had locked the new vma before asking any driver to set things up (and the assert would catch it if somebody didn't).
I realize that it doesn't hurt in a technical sense, but I think having drivers call these VM-internal subtle locking functions does hurt in a maintenance sense, so we should make sure to not have it.
Ok, IOW the vma would be already locked before mmap() is called... Just to confirm, you are suggesting to remove vma_start_write() call from hfi1_file_mmap() and let vm_flags_reset() generate an assertion if it's ever called with an unlocked vma, correct?
Linus