On Sun 29-01-17 00:27:27, Tetsuo Handa wrote: > Michal Hocko wrote: > > Tetsuo, > > before we settle on the proper fix for this issue, could you give the > > patch a try and try to reproduce the too_many_isolated() issue or > > just see whether patch [1] has any negative effect on your oom stress > > testing? > > > > [1] http://lkml.kernel.org/r/20170119112336.GN30786@xxxxxxxxxxxxxx > > I tested with both [1] and below patch applied on linux-next-20170125 and > the result is at http://I-love.SAKURA.ne.jp/tmp/serial-20170128.txt.xz . > > Regarding below patch, it helped avoiding complete memory depletion with > large write() request. I don't know whether below patch helps avoiding > complete memory depletion when reading large amount (in other words, I > don't know whether this check is done for large read() request). It's not AFAICS. do_generic_file_read doesn't do the fatal_signal_pending check. > But > I believe that __GFP_KILLABLE (despite the limitation that there are > unkillable waits in the reclaim path) is better solution compared to > scattering around fatal_signal_pending() in the callers. The reason > we check SIGKILL here is to avoid allocating memory more than needed. > If we check SIGKILL in the entry point of __alloc_pages_nodemask() and > retry: label in __alloc_pages_slowpath(), we waste 0 page. Regardless > of whether the OOM killer is invoked, whether memory can be allocated > without direct reclaim operation, not allocating memory unless needed > (in other words, allow page allocator fail immediately if the caller > can give up on SIGKILL and SIGKILL is pending) makes sense. It will > reduce possibility of OOM livelock on CONFIG_MMU=n kernels where the > OOM reaper is not available. I am not really convinced this is a good idea. Put aside the fuzzy semantic of __GFP_KILLABLE, we would have to use this flag in all potentially allocating places from read/write paths and then it is just easier to do the explicit checks in the the loops around those allocations. > > On Wed 25-01-17 14:00:14, Michal Hocko wrote: > > [...] > > > From 362da5cac527146a341300c2ca441245c16043e8 Mon Sep 17 00:00:00 2001 > > > From: Michal Hocko <mhocko@xxxxxxxx> > > > Date: Wed, 25 Jan 2017 11:06:37 +0100 > > > Subject: [PATCH] fs: break out of iomap_file_buffered_write on fatal signals > > > > > > Tetsuo has noticed that an OOM stress test which performs large write > > > requests can cause the full memory reserves depletion. He has tracked > > > this down to the following path > > > __alloc_pages_nodemask+0x436/0x4d0 > > > alloc_pages_current+0x97/0x1b0 > > > __page_cache_alloc+0x15d/0x1a0 mm/filemap.c:728 > > > pagecache_get_page+0x5a/0x2b0 mm/filemap.c:1331 > > > grab_cache_page_write_begin+0x23/0x40 mm/filemap.c:2773 > > > iomap_write_begin+0x50/0xd0 fs/iomap.c:118 > > > iomap_write_actor+0xb5/0x1a0 fs/iomap.c:190 > > > ? iomap_write_end+0x80/0x80 fs/iomap.c:150 > > > iomap_apply+0xb3/0x130 fs/iomap.c:79 > > > iomap_file_buffered_write+0x68/0xa0 fs/iomap.c:243 > > > ? iomap_write_end+0x80/0x80 > > > xfs_file_buffered_aio_write+0x132/0x390 [xfs] > > > ? remove_wait_queue+0x59/0x60 > > > xfs_file_write_iter+0x90/0x130 [xfs] > > > __vfs_write+0xe5/0x140 > > > vfs_write+0xc7/0x1f0 > > > ? syscall_trace_enter+0x1d0/0x380 > > > SyS_write+0x58/0xc0 > > > do_syscall_64+0x6c/0x200 > > > entry_SYSCALL64_slow_path+0x25/0x25 > > > > > > the oom victim has access to all memory reserves to make a forward > > > progress to exit easier. But iomap_file_buffered_write and other callers > > > of iomap_apply loop to complete the full request. We need to check for > > > fatal signals and back off with a short write instead. As the > > > iomap_apply delegates all the work down to the actor we have to hook > > > into those. All callers that work with the page cache are calling > > > iomap_write_begin so we will check for signals there. dax_iomap_actor > > > has to handle the situation explicitly because it copies data to the > > > userspace directly. Other callers like iomap_page_mkwrite work on a > > > single page or iomap_fiemap_actor do not allocate memory based on the > > > given len. > > > > > > Fixes: 68a9f5e7007c ("xfs: implement iomap based buffered write path") > > > Cc: stable # 4.8+ > > > Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > > > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx> > > > --- > > > fs/dax.c | 5 +++++ > > > fs/iomap.c | 3 +++ > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/fs/dax.c b/fs/dax.c > > > index 413a91db9351..0e263dacf9cf 100644 > > > --- a/fs/dax.c > > > +++ b/fs/dax.c > > > @@ -1033,6 +1033,11 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > > > struct blk_dax_ctl dax = { 0 }; > > > ssize_t map_len; > > > > > > + if (fatal_signal_pending(current)) { > > > + ret = -EINTR; > > > + break; > > > + } > > > + > > > dax.sector = dax_iomap_sector(iomap, pos); > > > dax.size = (length + offset + PAGE_SIZE - 1) & PAGE_MASK; > > > map_len = dax_map_atomic(iomap->bdev, &dax); > > > diff --git a/fs/iomap.c b/fs/iomap.c > > > index e57b90b5ff37..691eada58b06 100644 > > > --- a/fs/iomap.c > > > +++ b/fs/iomap.c > > > @@ -114,6 +114,9 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, > > > > > > BUG_ON(pos + len > iomap->offset + iomap->length); > > > > > > + if (fatal_signal_pending(current)) > > > + return -EINTR; > > > + > > > page = grab_cache_page_write_begin(inode->i_mapping, index, flags); > > > if (!page) > > > return -ENOMEM; > > > -- > > > 2.11.0 > > Regarding [1], it helped avoiding the too_many_isolated() issue. I can't > tell whether it has any negative effect, but I got on the first trial that > all allocating threads are blocked on wait_for_completion() from flush_work() > in drain_all_pages() introduced by "mm, page_alloc: drain per-cpu pages from > workqueue context". There was no warn_alloc() stall warning message afterwords. That patch is buggy and there is a follow up [1] which is not sitting in the mmotm (and thus linux-next) yet. I didn't get to review it properly and I cannot say I would be too happy about using WQ from the page allocator. I believe even the follow up needs to have WQ_RECLAIM WQ. [1] http://lkml.kernel.org/r/20170125083038.rzb5f43nptmk7aed@xxxxxxxxxxxxxxxxxxx Thanks for your testing! -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
