Re: [PATCH] Btrfs: wait for ordered extents before removing extent maps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2013/12/14 Filipe David Manana <fdmanana@xxxxxxxxx>:
> On Sat, Dec 14, 2013 at 2:56 PM, Shilong Wang <wangshilong1991@xxxxxxxxx> wrote:
>> Hello Filipe,
>>
>> 2013/12/14 Filipe David Borba Manana <fdmanana@xxxxxxxxx>:
>>> Wang Shilong got into a case where during inode eviction we were
>>> removing an extent map while it was pinned. This triggered a warning
>>> in remove_extent_mapping() because the extent map had the pinned
>>> flag set:
>>>
>>> [ 1209.102076]  [<ffffffffa04721b9>] remove_extent_mapping+0x69/0x70 [btrfs]
>>> [ 1209.102084]  [<ffffffffa0466b06>] btrfs_evict_inode+0x96/0x4d0 [btrfs]
>>> [ 1209.102089]  [<ffffffff81073010>] ? wake_atomic_t_function+0x40/0x40
>>> [ 1209.102092]  [<ffffffff8118ab2e>] evict+0x9e/0x190
>>> [ 1209.102094]  [<ffffffff8118b313>] iput+0xf3/0x180
>>> [ 1209.102101]  [<ffffffffa0461fd1>] btrfs_run_delayed_iputs+0xb1/0xd0 [btrfs]
>>> [ 1209.102107]  [<ffffffffa045d358>] __btrfs_end_transaction+0x268/0x350 [btrfs]
>>>
>>> Therefore wait for any pending ordered extents, if any, which will
>>> trigger calls to unpin_extent_cache(), before removing the extent maps.
>>>
>>> Wang's solution of simply clearing the pinned bit wasn't enough, as after
>>> unpin_extent_cache() will be called and trigger another WARN_ON() because
>>> the lookup for the extent map returned NULL.
>>
>> Why not in evict_inode_truncate_pages() move remove_extent_mapping() after
>> clear_extent_bit()?
>
> So, if the pinned bit is set, it means some task will clear it later,
> via unpin_extent_cache(). And if you look at that function, it has
> this:
>
> write_lock(&tree->lock);
> em = lookup_extent_mapping(tree, start, len);
>
> WARN_ON(!em || em->start != start);
>
> And remove_extent_mapping() will remove the em from the rbtree,
> regardless of its reference count value, therefore triggering that
> warning above.

Here i mean,  in evict_inode_truncate_pages()
We change it to:

Step1:  unpin_extent_cache()
Step2:  remove it from extent_mapping

Dose this cause any problems? i am a little confused, correct me if i
am wrong some places^_^.


>
> Does it makes sense?
>
> thanks
>
>>
>> Thanks,
>> Wang
>>>
>>> Thanks Wang for finding out this.
>>>
>>> Signed-off-by: Filipe David Borba Manana <fdmanana@xxxxxxxxx>
>>> ---
>>>  fs/btrfs/inode.c |    5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index e889779..c2933fb 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -4509,6 +4509,9 @@ static void evict_inode_truncate_pages(struct inode *inode)
>>>         ASSERT(inode->i_state & I_FREEING);
>>>         truncate_inode_pages(&inode->i_data, 0);
>>>
>>> +       /* do we really want it for ->i_nlink > 0 and zero btrfs_root_refs? */
>>> +       btrfs_wait_ordered_range(inode, 0, (u64)-1);
>>> +
>>>         write_lock(&map_tree->lock);
>>>         while (!RB_EMPTY_ROOT(&map_tree->map)) {
>>>                 struct extent_map *em;
>>> @@ -4566,8 +4569,6 @@ void btrfs_evict_inode(struct inode *inode)
>>>                 btrfs_orphan_del(NULL, inode);
>>>                 goto no_delete;
>>>         }
>>> -       /* do we really want it for ->i_nlink > 0 and zero btrfs_root_refs? */
>>> -       btrfs_wait_ordered_range(inode, 0, (u64)-1);
>>>
>>>         if (root->fs_info->log_root_recovering) {
>>>                 BUG_ON(test_bit(BTRFS_INODE_HAS_ORPHAN_ITEM,
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Filipe David Manana,
>
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystem Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux