Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1706362 > unrolled thread

Re: [RFC v5 05/11] mm: fix lock dependency against mapping->i_mmap_rwsem

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC v5 05/11] mm: fix lock dependency against  mapping->i_mmap_rwsem Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-08-08 13:20 +0200
    Re: [RFC v5 05/11] mm: fix lock dependency against  mapping->i_mmap_rwsem Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-08-08 14:30 +0200
      Re: [RFC v5 05/11] mm: fix lock dependency against  mapping->i_mmap_rwsem Jan Kara <jack@suse.cz> - 2017-08-08 14:50 +0200
        Re: [RFC v5 05/11] mm: fix lock dependency against  mapping->i_mmap_rwsem Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-08-08 15:10 +0200
      Re: [RFC v5 05/11] mm: fix lock dependency against  mapping->i_mmap_rwsem Peter Zijlstra <peterz@infradead.org> - 2017-08-08 15:20 +0200
        Re: [RFC v5 05/11] mm: fix lock dependency against  mapping->i_mmap_rwsem Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-08-08 15:40 +0200

#1706362 — Re: [RFC v5 05/11] mm: fix lock dependency against mapping->i_mmap_rwsem

On 06/16/2017 11:22 PM, Laurent Dufour wrote:
> kworker/32:1/819 is trying to acquire lock:
>  (&vma->vm_sequence){+.+...}, at: [<c0000000002f20e0>]
> zap_page_range_single+0xd0/0x1a0
> 
> but task is already holding lock:
>  (&mapping->i_mmap_rwsem){++++..}, at: [<c0000000002f229c>]
> unmap_mapping_range+0x7c/0x160
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #2 (&mapping->i_mmap_rwsem){++++..}:
>        down_write+0x84/0x130
>        __vma_adjust+0x1f4/0xa80
>        __split_vma.isra.2+0x174/0x290
>        do_munmap+0x13c/0x4e0
>        vm_munmap+0x64/0xb0
>        elf_map+0x11c/0x130
>        load_elf_binary+0x6f0/0x15f0
>        search_binary_handler+0xe0/0x2a0
>        do_execveat_common.isra.14+0x7fc/0xbe0
>        call_usermodehelper_exec_async+0x14c/0x1d0
>        ret_from_kernel_thread+0x5c/0x68
> 
> -> #1 (&vma->vm_sequence/1){+.+...}:
>        __vma_adjust+0x124/0xa80
>        __split_vma.isra.2+0x174/0x290
>        do_munmap+0x13c/0x4e0
>        vm_munmap+0x64/0xb0
>        elf_map+0x11c/0x130
>        load_elf_binary+0x6f0/0x15f0
>        search_binary_handler+0xe0/0x2a0
>        do_execveat_common.isra.14+0x7fc/0xbe0
>        call_usermodehelper_exec_async+0x14c/0x1d0
>        ret_from_kernel_thread+0x5c/0x68
> 
> -> #0 (&vma->vm_sequence){+.+...}:
>        lock_acquire+0xf4/0x310
>        unmap_page_range+0xcc/0xfa0
>        zap_page_range_single+0xd0/0x1a0
>        unmap_mapping_range+0x138/0x160
>        truncate_pagecache+0x50/0xa0
>        put_aio_ring_file+0x48/0xb0
>        aio_free_ring+0x40/0x1b0
>        free_ioctx+0x38/0xc0
>        process_one_work+0x2cc/0x8a0
>        worker_thread+0xac/0x580
>        kthread+0x164/0x1b0
>        ret_from_kernel_thread+0x5c/0x68
> 
> other info that might help us debug this:
> 
> Chain exists of:
>   &vma->vm_sequence --> &vma->vm_sequence/1 --> &mapping->i_mmap_rwsem
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(&mapping->i_mmap_rwsem);
>                                lock(&vma->vm_sequence/1);
>                                lock(&mapping->i_mmap_rwsem);
>   lock(&vma->vm_sequence);
> 
>  *** DEADLOCK ***
> 
> To fix that we must grab the vm_sequence lock after any mapping one in
> __vma_adjust().
> 
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>

Should not this be folded back into the previous patch ? It fixes an
issue introduced by the previous one.

[toc] | [next] | [standalone]

#1706480

On 08/08/2017 13:17, Anshuman Khandual wrote:
> On 06/16/2017 11:22 PM, Laurent Dufour wrote:
>> kworker/32:1/819 is trying to acquire lock:
>>  (&vma->vm_sequence){+.+...}, at: [<c0000000002f20e0>]
>> zap_page_range_single+0xd0/0x1a0
>>
>> but task is already holding lock:
>>  (&mapping->i_mmap_rwsem){++++..}, at: [<c0000000002f229c>]
>> unmap_mapping_range+0x7c/0x160
>>
>> which lock already depends on the new lock.
>>
>> the existing dependency chain (in reverse order) is:
>>
>> -> #2 (&mapping->i_mmap_rwsem){++++..}:
>>        down_write+0x84/0x130
>>        __vma_adjust+0x1f4/0xa80
>>        __split_vma.isra.2+0x174/0x290
>>        do_munmap+0x13c/0x4e0
>>        vm_munmap+0x64/0xb0
>>        elf_map+0x11c/0x130
>>        load_elf_binary+0x6f0/0x15f0
>>        search_binary_handler+0xe0/0x2a0
>>        do_execveat_common.isra.14+0x7fc/0xbe0
>>        call_usermodehelper_exec_async+0x14c/0x1d0
>>        ret_from_kernel_thread+0x5c/0x68
>>
>> -> #1 (&vma->vm_sequence/1){+.+...}:
>>        __vma_adjust+0x124/0xa80
>>        __split_vma.isra.2+0x174/0x290
>>        do_munmap+0x13c/0x4e0
>>        vm_munmap+0x64/0xb0
>>        elf_map+0x11c/0x130
>>        load_elf_binary+0x6f0/0x15f0
>>        search_binary_handler+0xe0/0x2a0
>>        do_execveat_common.isra.14+0x7fc/0xbe0
>>        call_usermodehelper_exec_async+0x14c/0x1d0
>>        ret_from_kernel_thread+0x5c/0x68
>>
>> -> #0 (&vma->vm_sequence){+.+...}:
>>        lock_acquire+0xf4/0x310
>>        unmap_page_range+0xcc/0xfa0
>>        zap_page_range_single+0xd0/0x1a0
>>        unmap_mapping_range+0x138/0x160
>>        truncate_pagecache+0x50/0xa0
>>        put_aio_ring_file+0x48/0xb0
>>        aio_free_ring+0x40/0x1b0
>>        free_ioctx+0x38/0xc0
>>        process_one_work+0x2cc/0x8a0
>>        worker_thread+0xac/0x580
>>        kthread+0x164/0x1b0
>>        ret_from_kernel_thread+0x5c/0x68
>>
>> other info that might help us debug this:
>>
>> Chain exists of:
>>   &vma->vm_sequence --> &vma->vm_sequence/1 --> &mapping->i_mmap_rwsem
>>
>>  Possible unsafe locking scenario:
>>
>>        CPU0                    CPU1
>>        ----                    ----
>>   lock(&mapping->i_mmap_rwsem);
>>                                lock(&vma->vm_sequence/1);
>>                                lock(&mapping->i_mmap_rwsem);
>>   lock(&vma->vm_sequence);
>>
>>  *** DEADLOCK ***
>>
>> To fix that we must grab the vm_sequence lock after any mapping one in
>> __vma_adjust().
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> 
> Should not this be folded back into the previous patch ? It fixes an
> issue introduced by the previous one.

This is an option, but the previous one was signed by Peter, and I'd prefer
to keep his unchanged and add this new one to fix that.
Again this is to ease the review.

[toc] | [prev] | [next] | [standalone]

#1706500

On Tue 08-08-17 14:20:23, Laurent Dufour wrote:
> On 08/08/2017 13:17, Anshuman Khandual wrote:
> > On 06/16/2017 11:22 PM, Laurent Dufour wrote:
> >> kworker/32:1/819 is trying to acquire lock:
> >>  (&vma->vm_sequence){+.+...}, at: [<c0000000002f20e0>]
> >> zap_page_range_single+0xd0/0x1a0
> >>
> >> but task is already holding lock:
> >>  (&mapping->i_mmap_rwsem){++++..}, at: [<c0000000002f229c>]
> >> unmap_mapping_range+0x7c/0x160
> >>
> >> which lock already depends on the new lock.
> >>
> >> the existing dependency chain (in reverse order) is:
> >>
> >> -> #2 (&mapping->i_mmap_rwsem){++++..}:
> >>        down_write+0x84/0x130
> >>        __vma_adjust+0x1f4/0xa80
> >>        __split_vma.isra.2+0x174/0x290
> >>        do_munmap+0x13c/0x4e0
> >>        vm_munmap+0x64/0xb0
> >>        elf_map+0x11c/0x130
> >>        load_elf_binary+0x6f0/0x15f0
> >>        search_binary_handler+0xe0/0x2a0
> >>        do_execveat_common.isra.14+0x7fc/0xbe0
> >>        call_usermodehelper_exec_async+0x14c/0x1d0
> >>        ret_from_kernel_thread+0x5c/0x68
> >>
> >> -> #1 (&vma->vm_sequence/1){+.+...}:
> >>        __vma_adjust+0x124/0xa80
> >>        __split_vma.isra.2+0x174/0x290
> >>        do_munmap+0x13c/0x4e0
> >>        vm_munmap+0x64/0xb0
> >>        elf_map+0x11c/0x130
> >>        load_elf_binary+0x6f0/0x15f0
> >>        search_binary_handler+0xe0/0x2a0
> >>        do_execveat_common.isra.14+0x7fc/0xbe0
> >>        call_usermodehelper_exec_async+0x14c/0x1d0
> >>        ret_from_kernel_thread+0x5c/0x68
> >>
> >> -> #0 (&vma->vm_sequence){+.+...}:
> >>        lock_acquire+0xf4/0x310
> >>        unmap_page_range+0xcc/0xfa0
> >>        zap_page_range_single+0xd0/0x1a0
> >>        unmap_mapping_range+0x138/0x160
> >>        truncate_pagecache+0x50/0xa0
> >>        put_aio_ring_file+0x48/0xb0
> >>        aio_free_ring+0x40/0x1b0
> >>        free_ioctx+0x38/0xc0
> >>        process_one_work+0x2cc/0x8a0
> >>        worker_thread+0xac/0x580
> >>        kthread+0x164/0x1b0
> >>        ret_from_kernel_thread+0x5c/0x68
> >>
> >> other info that might help us debug this:
> >>
> >> Chain exists of:
> >>   &vma->vm_sequence --> &vma->vm_sequence/1 --> &mapping->i_mmap_rwsem
> >>
> >>  Possible unsafe locking scenario:
> >>
> >>        CPU0                    CPU1
> >>        ----                    ----
> >>   lock(&mapping->i_mmap_rwsem);
> >>                                lock(&vma->vm_sequence/1);
> >>                                lock(&mapping->i_mmap_rwsem);
> >>   lock(&vma->vm_sequence);
> >>
> >>  *** DEADLOCK ***
> >>
> >> To fix that we must grab the vm_sequence lock after any mapping one in
> >> __vma_adjust().
> >>
> >> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > 
> > Should not this be folded back into the previous patch ? It fixes an
> > issue introduced by the previous one.
> 
> This is an option, but the previous one was signed by Peter, and I'd prefer
> to keep his unchanged and add this new one to fix that.
> Again this is to ease the review.

In this particular case I disagree. We should not have buggy patches in the
series. It breaks bisectability and the ease of review is IMO very
questionable because the previous patch is simply buggy and thus is hard to
validate on its own. If the resulting combo would be too complex, you could
think of a different way how to split it up so that intermediate steps are
not buggy...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1706527

On 08/08/2017 14:49, Jan Kara wrote:
> On Tue 08-08-17 14:20:23, Laurent Dufour wrote:
>> On 08/08/2017 13:17, Anshuman Khandual wrote:
>>> On 06/16/2017 11:22 PM, Laurent Dufour wrote:
>>>> kworker/32:1/819 is trying to acquire lock:
>>>>  (&vma->vm_sequence){+.+...}, at: [<c0000000002f20e0>]
>>>> zap_page_range_single+0xd0/0x1a0
>>>>
>>>> but task is already holding lock:
>>>>  (&mapping->i_mmap_rwsem){++++..}, at: [<c0000000002f229c>]
>>>> unmap_mapping_range+0x7c/0x160
>>>>
>>>> which lock already depends on the new lock.
>>>>
>>>> the existing dependency chain (in reverse order) is:
>>>>
>>>> -> #2 (&mapping->i_mmap_rwsem){++++..}:
>>>>        down_write+0x84/0x130
>>>>        __vma_adjust+0x1f4/0xa80
>>>>        __split_vma.isra.2+0x174/0x290
>>>>        do_munmap+0x13c/0x4e0
>>>>        vm_munmap+0x64/0xb0
>>>>        elf_map+0x11c/0x130
>>>>        load_elf_binary+0x6f0/0x15f0
>>>>        search_binary_handler+0xe0/0x2a0
>>>>        do_execveat_common.isra.14+0x7fc/0xbe0
>>>>        call_usermodehelper_exec_async+0x14c/0x1d0
>>>>        ret_from_kernel_thread+0x5c/0x68
>>>>
>>>> -> #1 (&vma->vm_sequence/1){+.+...}:
>>>>        __vma_adjust+0x124/0xa80
>>>>        __split_vma.isra.2+0x174/0x290
>>>>        do_munmap+0x13c/0x4e0
>>>>        vm_munmap+0x64/0xb0
>>>>        elf_map+0x11c/0x130
>>>>        load_elf_binary+0x6f0/0x15f0
>>>>        search_binary_handler+0xe0/0x2a0
>>>>        do_execveat_common.isra.14+0x7fc/0xbe0
>>>>        call_usermodehelper_exec_async+0x14c/0x1d0
>>>>        ret_from_kernel_thread+0x5c/0x68
>>>>
>>>> -> #0 (&vma->vm_sequence){+.+...}:
>>>>        lock_acquire+0xf4/0x310
>>>>        unmap_page_range+0xcc/0xfa0
>>>>        zap_page_range_single+0xd0/0x1a0
>>>>        unmap_mapping_range+0x138/0x160
>>>>        truncate_pagecache+0x50/0xa0
>>>>        put_aio_ring_file+0x48/0xb0
>>>>        aio_free_ring+0x40/0x1b0
>>>>        free_ioctx+0x38/0xc0
>>>>        process_one_work+0x2cc/0x8a0
>>>>        worker_thread+0xac/0x580
>>>>        kthread+0x164/0x1b0
>>>>        ret_from_kernel_thread+0x5c/0x68
>>>>
>>>> other info that might help us debug this:
>>>>
>>>> Chain exists of:
>>>>   &vma->vm_sequence --> &vma->vm_sequence/1 --> &mapping->i_mmap_rwsem
>>>>
>>>>  Possible unsafe locking scenario:
>>>>
>>>>        CPU0                    CPU1
>>>>        ----                    ----
>>>>   lock(&mapping->i_mmap_rwsem);
>>>>                                lock(&vma->vm_sequence/1);
>>>>                                lock(&mapping->i_mmap_rwsem);
>>>>   lock(&vma->vm_sequence);
>>>>
>>>>  *** DEADLOCK ***
>>>>
>>>> To fix that we must grab the vm_sequence lock after any mapping one in
>>>> __vma_adjust().
>>>>
>>>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>>>
>>> Should not this be folded back into the previous patch ? It fixes an
>>> issue introduced by the previous one.
>>
>> This is an option, but the previous one was signed by Peter, and I'd prefer
>> to keep his unchanged and add this new one to fix that.
>> Again this is to ease the review.
> 
> In this particular case I disagree. We should not have buggy patches in the
> series. It breaks bisectability and the ease of review is IMO very
> questionable because the previous patch is simply buggy and thus is hard to
> validate on its own. If the resulting combo would be too complex, you could
> think of a different way how to split it up so that intermediate steps are
> not buggy...

I don't think the combo will become too large, it's just moving some calls
around. So as bisectability seems to be more important than readability,
I'll merge it into the original Peter's patch.

[toc] | [prev] | [next] | [standalone]

#1706538

On Tue, Aug 08, 2017 at 02:20:23PM +0200, Laurent Dufour wrote:
> This is an option, but the previous one was signed by Peter, and I'd prefer
> to keep his unchanged and add this new one to fix that.
> Again this is to ease the review.

You can always add something like:

[ldufour: fixed lockdep complaint]

Before your SoB.

[toc] | [prev] | [next] | [standalone]

#1706555

On 08/08/2017 15:15, Peter Zijlstra wrote:
> On Tue, Aug 08, 2017 at 02:20:23PM +0200, Laurent Dufour wrote:
>> This is an option, but the previous one was signed by Peter, and I'd prefer
>> to keep his unchanged and add this new one to fix that.
>> Again this is to ease the review.
> 
> You can always add something like:
> 
> [ldufour: fixed lockdep complaint]
> 
> Before your SoB.

Yes, that's what I'm doing right now, and I'll push a new series based on
4.13-rc4 asap.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web