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 > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
