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.
>
> 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.
[1] http://lkml.kernel.org/r/20170106141845.24362-1-mhocko@xxxxxxxxxx
--
Michal Hocko
SUSE Labs
