On wed, 23 Jan 2013 11:56:55 +0800, Liu Bo wrote:
> On Wed, Jan 23, 2013 at 10:54:39AM +0800, Miao Xie wrote:
>> On Tue, 22 Jan 2013 22:24:15 +0800, Liu Bo wrote:
>>> On Tue, Jan 22, 2013 at 06:49:00PM +0800, Miao Xie wrote:
>>>> btrfs_start_delalloc_inodes() locks the delalloc_inodes list, fetches the
>>>> first inode, unlocks the list, triggers btrfs_alloc_delalloc_work/
>>>> btrfs_queue_worker for this inode, and then it locks the list, checks the
>>>> head of the list again. But because we don't delete the first inode that it
>>>> deals with before, it will fetch the same inode. As a result, this function
>>>> allocates a huge amount of btrfs_delalloc_work structures, and OOM happens.
>>>>
>>>> Fix this problem by splice this delalloc list.
>>>>
>>>> Reported-by: Alex Lyakas <[email protected]>
>>>> Signed-off-by: Miao Xie <[email protected]>
>>>> ---
>>>> fs/btrfs/inode.c | 55
>>>> ++++++++++++++++++++++++++++++++++++++++-------------
>>>> 1 files changed, 41 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>>> index 67ed24a..86f1d25 100644
>>>> --- a/fs/btrfs/inode.c
>>>> +++ b/fs/btrfs/inode.c
>>>> @@ -7545,41 +7545,61 @@ void btrfs_wait_and_free_delalloc_work(struct
>>>> btrfs_delalloc_work *work)
>>>> */
>>>> int btrfs_start_delalloc_inodes(struct btrfs_root *root, int delay_iput)
>>>> {
>>>> - struct list_head *head = &root->fs_info->delalloc_inodes;
>>>> struct btrfs_inode *binode;
>>>> struct inode *inode;
>>>> struct btrfs_delalloc_work *work, *next;
>>>> struct list_head works;
>>>> + struct list_head splice;
>>>> int ret = 0;
>>>>
>>>> if (root->fs_info->sb->s_flags & MS_RDONLY)
>>>> return -EROFS;
>>>>
>>>> INIT_LIST_HEAD(&works);
>>>> -
>>>> + INIT_LIST_HEAD(&splice);
>>>> +again:
>>>> spin_lock(&root->fs_info->delalloc_lock);
>>>> - while (!list_empty(head)) {
>>>> - binode = list_entry(head->next, struct btrfs_inode,
>>>> + list_splice_init(&root->fs_info->delalloc_inodes, &splice);
>>>> + while (!list_empty(&splice)) {
>>>> + binode = list_entry(splice.next, struct btrfs_inode,
>>>> delalloc_inodes);
>>>> +
>>>> + list_del_init(&binode->delalloc_inodes);
>>>> +
>>>
>>> I believe this patch can work well, but it's a little complex.
>>>
>>> How about adding a flag in runtime_flags set?
>>
>> I have tried to adding a flag in runtime_flags, but I found it is not a good
>> way, because
>> - it can not avoid traversing the delalloc list repeatedly when someone write
>> data into the file endlessly. In fact, it is unnecessary because we can
>> just
>> see that data as the one which is written after the flush is done.
>> - bit operation need lock the bus, but we have a spin lock to protect all
>> the relative variants, so it is unnecessary.
>>
>> besides that, there is something wrong with the following patch.
>
> Okay, I see the problem.
>
> But with [PATCH 4/5], I think maybe we can merge these two patches and
> simplify things as following?
>
> Just flush them once,
>
> spin_lock(&root->fs_info->delalloc_lock);
> list_splice_init(&root->fs_info->delalloc_inodes, &splice);
> spin_unlock(&root->fs_info->delalloc_lock);
>
> while (!list_empty(&splice)) {
> ...
> }
No, we can't. The other tasks which flush the delalloc data may remove the inode
from the delalloc list/splice list. If we release the lock, we will meet the
race
between list traversing and list_del().
Thanks
Miao
>
> thanks,
> liubo
>
>>
>>> We can use the flag instead of 'delalloc_inodes' list to tell if we
>>> have clear the delalloc bytes, and the most important thing is it
>>> won't touch the original code logic too much.
>>>
>>> thanks,
>>> liubo
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 67ed24a..692ed0e 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -1555,8 +1555,8 @@ static void btrfs_clear_bit_hook(struct inode *inode,
>>> BTRFS_I(inode)->delalloc_bytes -= len;
>>>
>>> if (do_list && BTRFS_I(inode)->delalloc_bytes == 0 &&
>>> - !list_empty(&BTRFS_I(inode)->delalloc_inodes)) {
>>> - list_del_init(&BTRFS_I(inode)->delalloc_inodes);
>>> + test_bit(BTRFS_INODE_FLUSH,
>>> &BTRFS_I(inode)->runtime_flags)) {
>>> + clear_bit(BTRFS_INODE_FLUSH,
>>> &BTRFS_I(inode)->runtime_flags);
>>> }
>>
>> We can not remove list_del_init(), because the delalloc file can be flushed
>> not only
>> by btrfs_start_delalloc_inodes(), but also by flusher or the task who invoke
>> btrfs_sync_file().
>>
>>> spin_unlock(&root->fs_info->delalloc_lock);
>>> }
>>> @@ -7562,8 +7562,9 @@ int btrfs_start_delalloc_inodes(struct btrfs_root
>>> *root, int delay_iput)
>>> binode = list_entry(head->next, struct btrfs_inode,
>>> delalloc_inodes);
>>> inode = igrab(&binode->vfs_inode);
>>> - if (!inode)
>>> - list_del_init(&binode->delalloc_inodes);
>>> +
>>> + list_del_init(&binode->delalloc_inodes);
>>> +
>>> spin_unlock(&root->fs_info->delalloc_lock);
>>> if (inode) {
>>> work = btrfs_alloc_delalloc_work(inode, 0, delay_iput);
>>> @@ -7572,6 +7573,7 @@ int btrfs_start_delalloc_inodes(struct btrfs_root
>>> *root, int delay_iput)
>>> goto out;
>>> }
>>> list_add_tail(&work->list, &works);
>>> + set_bit(BTRFS_INODE_FLUSH, &binode->runtime_flags);
>>
>> if someone flush the file before set_bit(), no one will clear bit.
>>
>>> btrfs_queue_worker(&root->fs_info->flush_workers,
>>> &work->work);
>>> }
>>> @@ -7580,6 +7582,18 @@ int btrfs_start_delalloc_inodes(struct btrfs_root
>>> *root, int delay_iput)
>>> }
>>> spin_unlock(&root->fs_info->delalloc_lock);
>>>
>>> + /* make sure we clear all delalloc bytes we have scheduled */
>>> + while (!list_empty(&works)) {
>>> + work = list_entry(works.next, struct btrfs_delalloc_work,
>>> + list);
>>> + binode = btrfs_ino(work->inode);
>>
>> ^^^^^^BTRFS_I(), not btrfs_ino()
>>
>>> + if (!test_bit(BTRFS_INODE_FLUSH, &binode->runtime_flags)) {
>>> + list_del_init(&work->list);
>>> + btrfs_wait_and_free_delalloc_work(work);
>>
>> We must wait and free all the delalloc work here, or memory leak will happen.
>>
>> Thanks
>> Miao
>>
>>> + }
>>> + cond_resched();
>>> + }
>>> +
>>> /* the filemap_flush will queue IO into the worker threads, but
>>> * we have to make sure the IO is actually started and that
>>> * ordered extents get created before we return
>>>
>>>
>>>
>>>> inode = igrab(&binode->vfs_inode);
>>>> if (!inode)
>>>> - list_del_init(&binode->delalloc_inodes);
>>>> + continue;
>>>> +
>>>> + list_add_tail(&binode->delalloc_inodes,
>>>> + &root->fs_info->delalloc_inodes);
>>>> spin_unlock(&root->fs_info->delalloc_lock);
>>>> - if (inode) {
>>>> - work = btrfs_alloc_delalloc_work(inode, 0, delay_iput);
>>>> - if (!work) {
>>>> - ret = -ENOMEM;
>>>> - goto out;
>>>> - }
>>>> - list_add_tail(&work->list, &works);
>>>> - btrfs_queue_worker(&root->fs_info->flush_workers,
>>>> - &work->work);
>>>> +
>>>> + work = btrfs_alloc_delalloc_work(inode, 0, delay_iput);
>>>> + if (unlikely(!work)) {
>>>> + ret = -ENOMEM;
>>>> + goto out;
>>>> }
>>>> + list_add_tail(&work->list, &works);
>>>> + btrfs_queue_worker(&root->fs_info->flush_workers,
>>>> + &work->work);
>>>> +
>>>> cond_resched();
>>>> spin_lock(&root->fs_info->delalloc_lock);
>>>> }
>>>> spin_unlock(&root->fs_info->delalloc_lock);
>>>>
>>>> + list_for_each_entry_safe(work, next, &works, list) {
>>>> + list_del_init(&work->list);
>>>> + btrfs_wait_and_free_delalloc_work(work);
>>>> + }
>>>> +
>>>> + spin_lock(&root->fs_info->delalloc_lock);
>>>> + if (!list_empty(&root->fs_info->delalloc_inodes)) {
>>>> + spin_unlock(&root->fs_info->delalloc_lock);
>>>> + goto again;
>>>> + }
>>>> + spin_unlock(&root->fs_info->delalloc_lock);
>>>> +
>>>> /* the filemap_flush will queue IO into the worker threads, but
>>>> * we have to make sure the IO is actually started and that
>>>> * ordered extents get created before we return
>>>> @@ -7592,11 +7612,18 @@ int btrfs_start_delalloc_inodes(struct btrfs_root
>>>> *root, int delay_iput)
>>>> atomic_read(&root->fs_info->async_delalloc_pages) == 0));
>>>> }
>>>> atomic_dec(&root->fs_info->async_submit_draining);
>>>> + return 0;
>>>> out:
>>>> list_for_each_entry_safe(work, next, &works, list) {
>>>> list_del_init(&work->list);
>>>> btrfs_wait_and_free_delalloc_work(work);
>>>> }
>>>> +
>>>> + if (!list_empty_careful(&splice)) {
>>>> + spin_lock(&root->fs_info->delalloc_lock);
>>>> + list_splice_tail(&splice, &root->fs_info->delalloc_inodes);
>>>> + spin_unlock(&root->fs_info->delalloc_lock);
>>>> + }
>>>> return ret;
>>>> }
>>>>
>>>> --
>>>> 1.6.5.2
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
