Re: [PATCH 1/5] Btrfs: fix repeated delalloc work allocation

[Miao Xie <miaox@cn.fujitsu.com>]

On Wed, 23 Jan 2013 17:52:14 +0800, Liu Bo wrote:
> On Wed, Jan 23, 2013 at 04:58:27PM +0800, Miao Xie wrote:
>> On wed, 23 Jan 2013 16:17:53 +0800, Liu Bo wrote:
>>> On Wed, Jan 23, 2013 at 02:33:27PM +0800, Miao Xie wrote:
>>>> On wed, 23 Jan 2013 14:06:21 +0800, Liu Bo wrote:
>>>>> On Wed, Jan 23, 2013 at 12:44:49PM +0800, Miao Xie wrote:
>>>>>> 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().
>>>>>
>>>>> OK, then please merge patch 1 and 4 so that we can backport 1 less patch
>>>>> at least.
>>>>
>>>> I don't think we should merge these two patch because they do two different things - one
>>>> is bug fix, and the other is just a improvement, and this improvement changes the logic
>>>> of the code and might be argumentative for some developers. So 2 patches is  better than one,
>>>> I think.
>>>
>>> Sorry, this is right only when patch 1 really fixes the problem Alex reported.
>>>
>>> But the fact is
>>>
>>> 1)  patch 1 is not enough to fix the bug, it just fixes the OOM of
>>> allocating 'struct btrfs_delalloc_work' while the original OOM of allocating
>>> requests remains.  We can still get the same inode over and over again
>>> and then stuck in btrfs_start_delalloc_inodes() because we 'goto again' to
>>> make sure we flush all inodes listed in fs_info->delalloc_inodes.
>>>
>>> 2)  patch 4 fixes 1)'s problems by removing 'goto again'.
>>>
>>> Am I missing something?
>>
>> In fact, there are two different problems.
>> One is OOM bug. it is a serious problem and also is an regression, so we should fix it
>> as soon as possible.
>> The other one is that we may fetch the same inode again and again if someone write data
>> into it endlessly. This problem is not so urgent to deal with. and perhaps some developers
>> think it is not a problem, we should flush that inode since there are dirty pages in it.
>> So we need split it from the patch of the 1st problem.
> 
> All right, I'm ok with this.
> 
> But the TWO different problems are both due to fetching the same inode more
> than once, and the solutions are indeed same, "fetch every inode on
> the list just once", and they are in the same code.

I forgot to say that the first problem can happen even though no task writes data into
the file after we start to flush the delalloc inodes.

Thanks
Miao

> 
> It is definitely a bug/problem if any users complains about their box
> getting stuck.  It is KERNEL that should be blamed.
> 
> thanks,
> liubo
>