lkml.org 
[lkml]   [2017]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone
    On Wed 25-01-17 20:09:31, Tetsuo Handa wrote:
    > Michal Hocko wrote:
    > > On Wed 25-01-17 11:19:57, Christoph Hellwig wrote:
    > > > On Wed, Jan 25, 2017 at 11:15:17AM +0100, Michal Hocko wrote:
    > > > > I think we are missing a check for fatal_signal_pending in
    > > > > iomap_file_buffered_write. This means that an oom victim can consume the
    > > > > full memory reserves. What do you think about the following? I haven't
    > > > > tested this but it mimics generic_perform_write so I guess it should
    > > > > work.
    > > >
    > > > Hi Michal,
    > > >
    > > > this looks reasonable to me. But we have a few more such loops,
    > > > maybe it makes sense to move the check into iomap_apply?
    > >
    > > I wasn't sure about the expected semantic of iomap_apply but now that
    > > I've actually checked all the callers I believe all of them should be
    > > able to handle EINTR just fine. Well iomap_file_dirty, iomap_zero_range,
    > > iomap_fiemap and iomap_page_mkwriteseem do not follow the standard
    > > pattern to return the number of written pages or an error but it rather
    > > propagates the error out. From my limited understanding of those code
    > > paths that should just be ok. I was not all that sure about iomap_dio_rw
    > > that is just too convoluted for me. If that one is OK as well then
    > > the following patch should be indeed better.
    >
    > Is "length" in
    >
    > written = actor(inode, pos, length, data, &iomap);
    >
    > call guaranteed to be small enough? If not guaranteed,
    > don't we need to check SIGKILL inside "actor" functions?

    You are right! Checking for signals inside iomap_apply doesn't really
    solve anything because basically all users do iov_iter_count(). Blee. So
    we have loops around iomap_apply which itself loops inside the actor.
    iomap_write_begin seems to be used by most of them which is also where we
    get the pagecache page so I guess this should be the "right" place to
    put the check in. Things like dax_iomap_actor will need an explicit check.
    This is quite unfortunate but I do not see any better solution.
    What do you think Christoph?
    ---
    From 362da5cac527146a341300c2ca441245c16043e8 Mon Sep 17 00:00:00 2001
    From: Michal Hocko <mhocko@suse.com>
    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@I-love.SAKURA.ne.jp>
    Signed-off-by: Michal Hocko <mhocko@suse.com>
    ---
    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

    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2017-01-25 14:00    [W:3.103 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site