From: Hou Tao <houtao1@huawei.com>
To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
Michal Hocko <mhocko@kernel.org>,
Matthew Wilcox <willy@infradead.org>
Cc: <phillip@squashfs.org.uk>, <linux-fsdevel@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>
Subject: Re: [PATCH] squashfs: enable __GFP_FS in ->readpage to prevent hang in mem alloc
Date: Tue, 18 Dec 2018 14:06:11 +0800 [thread overview]
Message-ID: <5ba9aba1-e00d-ae07-caf0-3e7eca7de4b6@huawei.com> (raw)
In-Reply-To: <00ff5d2d-a50f-4730-db8a-cea3d7a3eef7@I-love.SAKURA.ne.jp>
Hi,
On 2018/12/17 18:51, Tetsuo Handa wrote:
> On 2018/12/17 18:33, Michal Hocko wrote:
>> On Sun 16-12-18 19:51:57, Matthew Wilcox wrote:
>> [...]
>>> Ah, yes, that makes perfect sense. Thank you for the explanation.
>>>
>>> I wonder if the correct fix, however, is not to move the check for
>>> GFP_NOFS in out_of_memory() down to below the check whether to kill
>>> the current task. That would solve your problem, and I don't _think_
>>> it would cause any new ones. Michal, you touched this code last, what
>>> do you think?
>>
>> What do you mean exactly? Whether we kill a current task or something
>> else doesn't change much on the fact that NOFS is a reclaim restricted
>> context and we might kill too early. If the fs can do GFP_FS then it is
>> obviously a better thing to do because FS metadata can be reclaimed as
>> well and therefore there is potentially less memory pressure on
>> application data.
>>
>
> I interpreted "to move the check for GFP_NOFS in out_of_memory() down to
> below the check whether to kill the current task" as
>
> @@ -1077,15 +1077,6 @@ bool out_of_memory(struct oom_control *oc)
> }
>
> /*
> - * The OOM killer does not compensate for IO-less reclaim.
> - * pagefault_out_of_memory lost its gfp context so we have to
> - * make sure exclude 0 mask - all other users should have at least
> - * ___GFP_DIRECT_RECLAIM to get here.
> - */
> - if (oc->gfp_mask && !(oc->gfp_mask & __GFP_FS))
> - return true;
> -
> - /*
> * Check if there were limitations on the allocation (only relevant for
> * NUMA and memcg) that may require different handling.
> */
> @@ -1104,6 +1095,19 @@ bool out_of_memory(struct oom_control *oc)
> }
>
> select_bad_process(oc);
> +
> + /*
> + * The OOM killer does not compensate for IO-less reclaim.
> + * pagefault_out_of_memory lost its gfp context so we have to
> + * make sure exclude 0 mask - all other users should have at least
> + * ___GFP_DIRECT_RECLAIM to get here.
> + */
> + if ((oc->gfp_mask && !(oc->gfp_mask & __GFP_FS)) && oc->chosen &&
> + oc->chosen != (void *)-1UL && oc->chosen != current) {
> + put_task_struct(oc->chosen);
> + return true;
> + }
> +
> /* Found nothing?!?! */
> if (!oc->chosen) {
> dump_header(oc, NULL);
>
> which is prefixed by "the correct fix is not".
>
> Behaving like sysctl_oom_kill_allocating_task == 1 if __GFP_FS is not used
> will not be the correct fix. But ...
>
> Hou Tao wrote:
>> There is no need to disable __GFP_FS in ->readpage:
>> * It's a read-only fs, so there will be no dirty/writeback page and
>> there will be no deadlock against the caller's locked page
>
> is read-only filesystem sufficient for safe to use __GFP_FS?
>
> Isn't "whether it is safe to use __GFP_FS" depends on "whether fs locks
> are held or not" rather than "whether fs has dirty/writeback page or not" ?
>
In my understanding (correct me if I am wrong), there are three ways through which
reclamation will invoked fs related code and may cause dead-lock:
(1) write-back dirty pages. Not possible for squashfs.
(2) the reclamation of inodes & dentries. The current file is in-use, so it will be not
reclaimed, and for other reclaimable inodes, squashfs_destroy_inode() will
be invoked and it doesn't take any locks.
(3) customized shrinker defined by fs. No customized shrinker in squashfs.
So my point is that even a page lock is already held by squashfs_readpage() and
reclamation invokes back to squashfs code, there will be no dead-lock, so it's
safe to use __GFP_FS.
Regards,
Tao
> .
>
next prev parent reply other threads:[~2018-12-18 6:06 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-04 2:08 [PATCH] squashfs: enable __GFP_FS in ->readpage to prevent hang in mem alloc Hou Tao
2018-12-06 1:14 ` Hou Tao
2018-12-13 2:18 ` Hou Tao
2018-12-15 13:24 ` Hou Tao
2018-12-15 14:38 ` Matthew Wilcox
2018-12-16 9:38 ` Hou Tao
2018-12-17 3:51 ` Matthew Wilcox
2018-12-17 9:33 ` Michal Hocko
2018-12-17 10:51 ` Tetsuo Handa
2018-12-17 12:25 ` Matthew Wilcox
2018-12-17 14:10 ` Michal Hocko
2018-12-17 14:41 ` Matthew Wilcox
2018-12-17 14:49 ` Michal Hocko
2018-12-18 6:06 ` Hou Tao [this message]
2018-12-18 11:32 ` Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5ba9aba1-e00d-ae07-caf0-3e7eca7de4b6@huawei.com \
--to=houtao1@huawei.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
--cc=phillip@squashfs.org.uk \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).