From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:37924 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751521AbeBTMQL (ORCPT ); Tue, 20 Feb 2018 07:16:11 -0500 Subject: Re: [PATCH 06/10] fs: iomap use memalloc_nofs_* scope API To: Michal Hocko , Dave Chinner Cc: linux-fsdevel@vger.kernel.org, Goldwyn Rodrigues References: <20180219140230.5077-1-rgoldwyn@suse.de> <20180219140230.5077-7-rgoldwyn@suse.de> <20180219223053.GJ6778@dastard> <20180220090544.GU21134@dhcp22.suse.cz> From: Goldwyn Rodrigues Message-ID: Date: Tue, 20 Feb 2018 06:16:07 -0600 MIME-Version: 1.0 In-Reply-To: <20180220090544.GU21134@dhcp22.suse.cz> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 >>> >>> 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 >>> --- >>> 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 >>> #include >>> #include >>> +#include >>> >>> #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@kernel.org > -- Goldwyn