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. Thanks Miao -- 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
