On 02/20/2018 03:05 AM, Michal Hocko wrote:
> On Tue 20-02-18 09:30:53, Dave Chinner wrote:
>> On Mon, Feb 19, 2018 at 08:02:26AM -0600, Goldwyn Rodrigues wrote:
>>> From: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
>>>
>>> While we are at it, remove the flags parameter from iomap_write_begin()
>>> since we passed AOP_FLAG_NOFS to it.
>>>
>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
>>> ---
>>> fs/iomap.c | 15 +++++++++------
>>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/iomap.c b/fs/iomap.c
>>> index afd163586aa0..afaf6ffff34f 100644
>>> --- a/fs/iomap.c
>>> +++ b/fs/iomap.c
>>> @@ -27,6 +27,7 @@
>>> #include <linux/task_io_accounting_ops.h>
>>> #include <linux/dax.h>
>>> #include <linux/sched/signal.h>
>>> +#include <linux/sched/mm.h>
>>>
>>> #include "internal.h"
>>>
>>> @@ -109,19 +110,22 @@ iomap_write_failed(struct inode *inode, loff_t pos, unsigned len)
>>> }
>>>
>>> static int
>>> -iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
>>> +iomap_write_begin(struct inode *inode, loff_t pos, unsigned len,
>>> struct page **pagep, struct iomap *iomap)
>>> {
>>> pgoff_t index = pos >> PAGE_SHIFT;
>>> struct page *page;
>>> int status = 0;
>>> + unsigned nofs_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);
>>> + nofs_flags = memalloc_nofs_save();
>>> + page = grab_cache_page_write_begin(inode->i_mapping, index, 0);
>>> + memalloc_nofs_restore(nofs_flags);
>>
>> This is wrong. flags are provided by the caller, so the caller
>> decides on the memory allocation scope, not this function.
All callers are calling with the same parameter. However, this is not
the place to eliminate this. I agree the scope is too narrow.
>>
>> As it is, I can't say I like this whole patchset. This is not what
>> the scoped memory allocation API was intended for. i.e. it was
>> intended to mark large regions of memory allocation restrictions for
>> disjoint operations, not replace a neat, clean flag interface
>> with a mess of save/restore calls and new ways to screw up error
>> handling....
>
> 100% agreed! My original plan was to identify reclaim recursion
> sensitive contexts per fs and then remove explicit GFP_NOFS called
> solely from those contexts. [1] should help to identify the later. The
> harderst part is the first part of course. It requires the detailed
> knowledge of the fs reclaim implementation of course.
Thanks. I misunderstood the scope concept.
In a previous test, I tried converting GFP_NOFS to GFP_KERNEL in order
to hang/crash the kernel with heavy I/O in low memory but failed. I
tried it with three filesystems. Hopefully, these patches will extract
out the positions which need attention.
>
> [1] http://lkml.kernel.org/r/20170106141845.24362-1-mhocko@xxxxxxxxxx
>
--
Goldwyn
