* [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order @ 2022-06-18 8:38 Miaohe Lin 2022-06-18 10:34 ` Matthew Wilcox 0 siblings, 1 reply; 7+ messages in thread From: Miaohe Lin @ 2022-06-18 8:38 UTC (permalink / raw) To: akpm, willy; +Cc: linux-fsdevel, linux-mm, linux-kernel, linmiaohe The invalidate_locks of two mappings should be unlocked in reverse order relative to the locking order in filemap_invalidate_lock_two(). Modifying the code to obey it. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/filemap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm/filemap.c b/mm/filemap.c index 8ef861297ffb..9948b26e6400 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1009,6 +1009,8 @@ EXPORT_SYMBOL(filemap_invalidate_lock_two); void filemap_invalidate_unlock_two(struct address_space *mapping1, struct address_space *mapping2) { + if (mapping1 < mapping2) + swap(mapping1, mapping2); if (mapping1) up_write(&mapping1->invalidate_lock); if (mapping2 && mapping1 != mapping2) -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order 2022-06-18 8:38 [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order Miaohe Lin @ 2022-06-18 10:34 ` Matthew Wilcox 2022-06-20 1:56 ` Miaohe Lin 0 siblings, 1 reply; 7+ messages in thread From: Matthew Wilcox @ 2022-06-18 10:34 UTC (permalink / raw) To: Miaohe Lin; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote: > The invalidate_locks of two mappings should be unlocked in reverse order > relative to the locking order in filemap_invalidate_lock_two(). Modifying Why? It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B). If it weren't we'd have lockdep check it and complain. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order 2022-06-18 10:34 ` Matthew Wilcox @ 2022-06-20 1:56 ` Miaohe Lin 2022-06-20 4:47 ` Matthew Wilcox 0 siblings, 1 reply; 7+ messages in thread From: Miaohe Lin @ 2022-06-20 1:56 UTC (permalink / raw) To: Matthew Wilcox; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel On 2022/6/18 18:34, Matthew Wilcox wrote: > On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote: >> The invalidate_locks of two mappings should be unlocked in reverse order >> relative to the locking order in filemap_invalidate_lock_two(). Modifying > > Why? It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B). > If it weren't we'd have lockdep check it and complain. For spin_lock, they are lock(A) lock(B) unlock(B) unlock(A) e.g. in copy_huge_pud, copy_huge_pmd, move_huge_pmd and so on: dst_ptl = pmd_lock(dst_mm, dst_pmd); src_ptl = pmd_lockptr(src_mm, src_pmd); spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); ... spin_unlock(src_ptl); spin_unlock(dst_ptl); For rw_semaphore, they are also lock(A) lock(B) unlock(B) unlock(A) e.g. in dup_mmap(): mmap_write_lock_killable(oldmm) mmap_write_lock_nested(mm, SINGLE_DEPTH_NESTING); ... mmap_write_unlock(mm); mmap_write_unlock(oldmm); and ntfs_extend_mft(): down_write(&ni->file.run_lock); down_write_nested(&sbi->used.bitmap.rw_lock, BITMAP_MUTEX_CLUSTERS); ... up_write(&sbi->used.bitmap.rw_lock); up_write(&ni->file.run_lock); But I see some lock(A) lock(B) unlock(A) unlock(B) examples in some fs codes. Could you please tell me the right lock/unlock order? I'm somewhat confused now... BTW: If lock(A) lock(B) unlock(A) unlock(B) is requested, filemap_invalidate_lock_two might still need to be changed to respect that order? Thanks! > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order 2022-06-20 1:56 ` Miaohe Lin @ 2022-06-20 4:47 ` Matthew Wilcox 2022-06-20 6:35 ` Miaohe Lin 0 siblings, 1 reply; 7+ messages in thread From: Matthew Wilcox @ 2022-06-20 4:47 UTC (permalink / raw) To: Miaohe Lin; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel On Mon, Jun 20, 2022 at 09:56:06AM +0800, Miaohe Lin wrote: > On 2022/6/18 18:34, Matthew Wilcox wrote: > > On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote: > >> The invalidate_locks of two mappings should be unlocked in reverse order > >> relative to the locking order in filemap_invalidate_lock_two(). Modifying > > > > Why? It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B). > > If it weren't we'd have lockdep check it and complain. > > For spin_lock, they are lock(A) lock(B) unlock(B) unlock(A) e.g. in copy_huge_pud, I think you need to spend some time thinking about the semantics of locks and try to figure out why it would make any difference at all which order locks (of any type) are _unlocked_ in, > copy_huge_pmd, move_huge_pmd and so on: > dst_ptl = pmd_lock(dst_mm, dst_pmd); > src_ptl = pmd_lockptr(src_mm, src_pmd); > spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); > ... > spin_unlock(src_ptl); > spin_unlock(dst_ptl); > > For rw_semaphore, they are also lock(A) lock(B) unlock(B) unlock(A) e.g. in dup_mmap(): > mmap_write_lock_killable(oldmm) > mmap_write_lock_nested(mm, SINGLE_DEPTH_NESTING); > ... > mmap_write_unlock(mm); > mmap_write_unlock(oldmm); > > and ntfs_extend_mft(): > down_write(&ni->file.run_lock); > down_write_nested(&sbi->used.bitmap.rw_lock, BITMAP_MUTEX_CLUSTERS); > ... > up_write(&sbi->used.bitmap.rw_lock); > up_write(&ni->file.run_lock); > > But I see some lock(A) lock(B) unlock(A) unlock(B) examples in some fs codes. Could you > please tell me the right lock/unlock order? I'm somewhat confused now... > > BTW: If lock(A) lock(B) unlock(A) unlock(B) is requested, filemap_invalidate_lock_two might > still need to be changed to respect that order? > > Thanks! > > > > > . > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order 2022-06-20 4:47 ` Matthew Wilcox @ 2022-06-20 6:35 ` Miaohe Lin 2022-06-20 9:47 ` Muchun Song 0 siblings, 1 reply; 7+ messages in thread From: Miaohe Lin @ 2022-06-20 6:35 UTC (permalink / raw) To: Matthew Wilcox; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel On 2022/6/20 12:47, Matthew Wilcox wrote: > On Mon, Jun 20, 2022 at 09:56:06AM +0800, Miaohe Lin wrote: >> On 2022/6/18 18:34, Matthew Wilcox wrote: >>> On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote: >>>> The invalidate_locks of two mappings should be unlocked in reverse order >>>> relative to the locking order in filemap_invalidate_lock_two(). Modifying >>> >>> Why? It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B). >>> If it weren't we'd have lockdep check it and complain. It seems I misunderstand your word. I thought you said it must be at lock(A) lock(B) unlock(A) unlock(B) order... Sorry. >> >> For spin_lock, they are lock(A) lock(B) unlock(B) unlock(A) e.g. in copy_huge_pud, > > I think you need to spend some time thinking about the semantics of > locks and try to figure out why it would make any difference at all > which order locks (of any type) are _unlocked_ in, IIUC, the lock orders are important to prevent possible deadlock. But unlock orders should be relaxed because they won't result in problem indeed. And what I advocate here is that making it at lock(A) lock(B) unlock(B) unlock(A) order should be a better program practice. Or unlock order shouldn't be obligatory at practice? Thanks. > >> copy_huge_pmd, move_huge_pmd and so on: >> dst_ptl = pmd_lock(dst_mm, dst_pmd); >> src_ptl = pmd_lockptr(src_mm, src_pmd); >> spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); >> ... >> spin_unlock(src_ptl); >> spin_unlock(dst_ptl); >> >> For rw_semaphore, they are also lock(A) lock(B) unlock(B) unlock(A) e.g. in dup_mmap(): >> mmap_write_lock_killable(oldmm) >> mmap_write_lock_nested(mm, SINGLE_DEPTH_NESTING); >> ... >> mmap_write_unlock(mm); >> mmap_write_unlock(oldmm); >> >> and ntfs_extend_mft(): >> down_write(&ni->file.run_lock); >> down_write_nested(&sbi->used.bitmap.rw_lock, BITMAP_MUTEX_CLUSTERS); >> ... >> up_write(&sbi->used.bitmap.rw_lock); >> up_write(&ni->file.run_lock); >> >> But I see some lock(A) lock(B) unlock(A) unlock(B) examples in some fs codes. Could you >> please tell me the right lock/unlock order? I'm somewhat confused now... >> >> BTW: If lock(A) lock(B) unlock(A) unlock(B) is requested, filemap_invalidate_lock_two might >> still need to be changed to respect that order? >> >> Thanks! >> >>> >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order 2022-06-20 6:35 ` Miaohe Lin @ 2022-06-20 9:47 ` Muchun Song 2022-06-20 12:14 ` Miaohe Lin 0 siblings, 1 reply; 7+ messages in thread From: Muchun Song @ 2022-06-20 9:47 UTC (permalink / raw) To: Miaohe Lin; +Cc: Matthew Wilcox, akpm, linux-fsdevel, linux-mm, linux-kernel On Mon, Jun 20, 2022 at 02:35:30PM +0800, Miaohe Lin wrote: > On 2022/6/20 12:47, Matthew Wilcox wrote: > > On Mon, Jun 20, 2022 at 09:56:06AM +0800, Miaohe Lin wrote: > >> On 2022/6/18 18:34, Matthew Wilcox wrote: > >>> On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote: > >>>> The invalidate_locks of two mappings should be unlocked in reverse order > >>>> relative to the locking order in filemap_invalidate_lock_two(). Modifying > >>> > >>> Why? It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B). > >>> If it weren't we'd have lockdep check it and complain. > > It seems I misunderstand your word. I thought you said it must be at lock(A) lock(B) unlock(A) unlock(B) > order... Sorry. > > >> > >> For spin_lock, they are lock(A) lock(B) unlock(B) unlock(A) e.g. in copy_huge_pud, > > > > I think you need to spend some time thinking about the semantics of > > locks and try to figure out why it would make any difference at all > > which order locks (of any type) are _unlocked_ in, > > IIUC, the lock orders are important to prevent possible deadlock. But unlock orders should be relaxed > because they won't result in problem indeed. And what I advocate here is that making it at lock(A) lock(B) > unlock(B) unlock(A) order should be a better program practice. Or unlock order shouldn't be obligatory > at practice? > lock(A) lock(B) unlock(A) unlock(B) is fine. So it is better not to complicate the code. > Thanks. > > > > >> copy_huge_pmd, move_huge_pmd and so on: > >> dst_ptl = pmd_lock(dst_mm, dst_pmd); > >> src_ptl = pmd_lockptr(src_mm, src_pmd); > >> spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); > >> ... > >> spin_unlock(src_ptl); > >> spin_unlock(dst_ptl); > >> > >> For rw_semaphore, they are also lock(A) lock(B) unlock(B) unlock(A) e.g. in dup_mmap(): > >> mmap_write_lock_killable(oldmm) > >> mmap_write_lock_nested(mm, SINGLE_DEPTH_NESTING); > >> ... > >> mmap_write_unlock(mm); > >> mmap_write_unlock(oldmm); > >> > >> and ntfs_extend_mft(): > >> down_write(&ni->file.run_lock); > >> down_write_nested(&sbi->used.bitmap.rw_lock, BITMAP_MUTEX_CLUSTERS); > >> ... > >> up_write(&sbi->used.bitmap.rw_lock); > >> up_write(&ni->file.run_lock); > >> > >> But I see some lock(A) lock(B) unlock(A) unlock(B) examples in some fs codes. Could you > >> please tell me the right lock/unlock order? I'm somewhat confused now... > >> > >> BTW: If lock(A) lock(B) unlock(A) unlock(B) is requested, filemap_invalidate_lock_two might > >> still need to be changed to respect that order? > >> > >> Thanks! > >> > >>> > >>> . > >>> > >> > > > > . > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order 2022-06-20 9:47 ` Muchun Song @ 2022-06-20 12:14 ` Miaohe Lin 0 siblings, 0 replies; 7+ messages in thread From: Miaohe Lin @ 2022-06-20 12:14 UTC (permalink / raw) To: Muchun Song; +Cc: Matthew Wilcox, akpm, linux-fsdevel, linux-mm, linux-kernel On 2022/6/20 17:47, Muchun Song wrote: > On Mon, Jun 20, 2022 at 02:35:30PM +0800, Miaohe Lin wrote: >> On 2022/6/20 12:47, Matthew Wilcox wrote: >>> On Mon, Jun 20, 2022 at 09:56:06AM +0800, Miaohe Lin wrote: >>>> On 2022/6/18 18:34, Matthew Wilcox wrote: >>>>> On Sat, Jun 18, 2022 at 04:38:20PM +0800, Miaohe Lin wrote: >>>>>> The invalidate_locks of two mappings should be unlocked in reverse order >>>>>> relative to the locking order in filemap_invalidate_lock_two(). Modifying >>>>> >>>>> Why? It's perfectly valid to lock(A) lock(B) unlock(A) unlock(B). >>>>> If it weren't we'd have lockdep check it and complain. >> >> It seems I misunderstand your word. I thought you said it must be at lock(A) lock(B) unlock(A) unlock(B) >> order... Sorry. >> >>>> >>>> For spin_lock, they are lock(A) lock(B) unlock(B) unlock(A) e.g. in copy_huge_pud, >>> >>> I think you need to spend some time thinking about the semantics of >>> locks and try to figure out why it would make any difference at all >>> which order locks (of any type) are _unlocked_ in, >> >> IIUC, the lock orders are important to prevent possible deadlock. But unlock orders should be relaxed >> because they won't result in problem indeed. And what I advocate here is that making it at lock(A) lock(B) >> unlock(B) unlock(A) order should be a better program practice. Or unlock order shouldn't be obligatory >> at practice? >> > > lock(A) lock(B) unlock(A) unlock(B) is fine. So it is better not to complicate the code. Yes, it seems the gain is not worth complicating the code. So I will drop the patch. Thanks. > >> Thanks. >> >>> >>>> copy_huge_pmd, move_huge_pmd and so on: >>>> dst_ptl = pmd_lock(dst_mm, dst_pmd); >>>> src_ptl = pmd_lockptr(src_mm, src_pmd); >>>> spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); >>>> ... >>>> spin_unlock(src_ptl); >>>> spin_unlock(dst_ptl); >>>> >>>> For rw_semaphore, they are also lock(A) lock(B) unlock(B) unlock(A) e.g. in dup_mmap(): >>>> mmap_write_lock_killable(oldmm) >>>> mmap_write_lock_nested(mm, SINGLE_DEPTH_NESTING); >>>> ... >>>> mmap_write_unlock(mm); >>>> mmap_write_unlock(oldmm); >>>> >>>> and ntfs_extend_mft(): >>>> down_write(&ni->file.run_lock); >>>> down_write_nested(&sbi->used.bitmap.rw_lock, BITMAP_MUTEX_CLUSTERS); >>>> ... >>>> up_write(&sbi->used.bitmap.rw_lock); >>>> up_write(&ni->file.run_lock); >>>> >>>> But I see some lock(A) lock(B) unlock(A) unlock(B) examples in some fs codes. Could you >>>> please tell me the right lock/unlock order? I'm somewhat confused now... >>>> >>>> BTW: If lock(A) lock(B) unlock(A) unlock(B) is requested, filemap_invalidate_lock_two might >>>> still need to be changed to respect that order? >>>> >>>> Thanks! >>>> >>>>> >>>>> . >>>>> >>>> >>> >>> . >>> >> >> > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-20 12:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-18 8:38 [PATCH] filemap: obey mapping->invalidate_lock lock/unlock order Miaohe Lin 2022-06-18 10:34 ` Matthew Wilcox 2022-06-20 1:56 ` Miaohe Lin 2022-06-20 4:47 ` Matthew Wilcox 2022-06-20 6:35 ` Miaohe Lin 2022-06-20 9:47 ` Muchun Song 2022-06-20 12:14 ` Miaohe Lin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.