On Wed, Aug 2, 2023 at 10:49 AM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Tue, 1 Aug 2023 at 15:07, Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: > > > > To make locking more visible, change these > > functions 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
