linux-kernel - Re: [PATCH 1/3] f2fs: avoid using __GFP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Nov 2017 00:08:25 +0800
From:   Chao Yu <chao@...nel.org>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     jaegeuk@...nel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, Chao Yu <yuchao0@...wei.com>
Subject: Re: [PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

Hi Michal,

On 2017/11/6 22:08, Michal Hocko wrote:
> On Sun 05-11-17 21:53:28, Chao Yu wrote:
>> From: Chao Yu <yuchao0@...wei.com>
>>
>> We will keep __add_ino_entry success all the time, for ENOMEM failure
>> case, we have already handled it with an opened loop code, so we don't
>> have to use redundant __GFP_NOFAIL in radix_tree_preload, remove it.
> 
> Why do you think an open coded allocation retry loop is better than
> having the MM do all it can when the nofail is requested explicitly?> E.g. giving it an access to memory reserves to allow forward progress.

Well, just want to remove one redundant implementation. But, as you said
MM has done lots of work to grab free memory including accessing memory
reserves, I think it's not bad for f2fs to use this flag instead of opened
loop code. :)

BTW, I notice the comments of __GFP_NOFAIL, what does this mean?
 *   Using this flag for costly allocations is _highly_ discouraged.

Thanks,

> 
>> Signed-off-by: Chao Yu <yuchao0@...wei.com>
>> ---
>>  fs/f2fs/checkpoint.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 98777c1ae70c..43ee9d97fd8f 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -405,10 +405,11 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>>  {
>>  	struct inode_management *im = &sbi->im[type];
>>  	struct ino_entry *e, *tmp;
>> +	bool preloaded;
>>  
>>  	tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
>>  retry:
>> -	radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
>> +	preloaded = !radix_tree_preload(GFP_NOFS);
>>  
>>  	spin_lock(&im->ino_lock);
>>  	e = radix_tree_lookup(&im->ino_root, ino);
>> @@ -416,7 +417,8 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>>  		e = tmp;
>>  		if (radix_tree_insert(&im->ino_root, ino, e)) {
>>  			spin_unlock(&im->ino_lock);
>> -			radix_tree_preload_end();
>> +			if (preloaded)
>> +				radix_tree_preload_end();
>>  			goto retry;
>>  		}
>>  		memset(e, 0, sizeof(struct ino_entry));
>> @@ -431,7 +433,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>>  		f2fs_set_bit(devidx, (char *)&e->dirty_device);
>>  
>>  	spin_unlock(&im->ino_lock);
>> -	radix_tree_preload_end();
>> +
>> +	if (preloaded)
>> +		radix_tree_preload_end();
>>  
>>  	if (e != tmp)
>>  		kmem_cache_free(ino_entry_slab, tmp);
>> -- 
>> 2.14.1.145.gb3622a4ee
>