linux-kernel - Re: [PATCH RFC] mm: abort inode pruning if it has active pages

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 for Android: free password hash cracker in your pocket

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

Date:	Fri, 18 Nov 2011 12:14:44 +0400
From:	Konstantin Khlebnikov <khlebnikov@...nvz.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] mm: abort inode pruning if it has active pages

Andrew Morton wrote:
> On Wed, 16 Nov 2011 17:47:47 +0300
> Konstantin Khlebnikov<khlebnikov@...nvz.org>  wrote:
>
>> Inode cache pruning can throw out some usefull data from page cache.
>> This patch aborts inode invalidation and keep inode alive if it still has
>> active pages.
>>
>
> hm, I suppose so.
>
> I also suppose there are various risks related to failing to reclaim
> inodes due to ongoing userspace activity and then running out of lowmem
> pages.

Ok, I think we can bypass active-page protection if CONFIG_HIGHMEM=y and
there is no __GFP_HIGHMEM in gfp_mask.

>
>> It improves interaction between inode cache and page cache.
>
> Well, this is the key part of the patch and it is the thing which we
> are most interested in.  But you didn't tell us anything about it!
>
> So please, provide us with much more detailed information on the
> observed benefits.

Currently this is based only on thought experiment.
I think rising pginodesteal and kswapd_inodesteal in /proc/vstat is sign of
inefficient memory reclaiming, because page-cache lru has a much more detailed
information about memory activity.

I plan to run some containers related tests/benchmarks,
something like multiple heavy-loaded web-servers.

>
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 1f6c48d..8d55a63 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -663,8 +663,8 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
>>   			spin_unlock(&inode->i_lock);
>>   			spin_unlock(&sb->s_inode_lru_lock);
>>   			if (remove_inode_buffers(inode))
>> -				reap += invalidate_mapping_pages(&inode->i_data,
>> -								0, -1);
>> +				reap += invalidate_inode_inactive_pages(
>> +						&inode->i_data, 0, -1);
>>   			iput(inode);
>>   			spin_lock(&sb->s_inode_lru_lock);
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 0c4df26..05875d7 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -2211,6 +2211,8 @@ extern int invalidate_partition(struct gendisk *, int);
>>   #endif
>>   unsigned long invalidate_mapping_pages(struct address_space *mapping,
>>   					pgoff_t start, pgoff_t end);
>> +unsigned long invalidate_inode_inactive_pages(struct address_space *mapping,
>> +					pgoff_t start, pgoff_t end);
>>
>>   static inline void invalidate_remote_inode(struct inode *inode)
>>   {
>> diff --git a/mm/truncate.c b/mm/truncate.c
>> index 632b15e..ac739bc 100644
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@ -379,6 +379,52 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
>>   EXPORT_SYMBOL(invalidate_mapping_pages);
>>
>>   /*
>> + * This is like invalidate_mapping_pages(),
>> + * except it aborts invalidation at the first active page.
>> + */
>> +unsigned long invalidate_inode_inactive_pages(struct address_space *mapping,
>> +					    pgoff_t start, pgoff_t end)
>> +{
>> +	struct pagevec pvec;
>> +	pgoff_t index = start;
>> +	unsigned long ret;
>> +	unsigned long count = 0;
>> +	int i;
>> +
>> +	pagevec_init(&pvec, 0);
>> +	while (index<= end&&  pagevec_lookup(&pvec, mapping, index,
>> +			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
>> +
>> +		mem_cgroup_uncharge_start();
>> +		for (i = 0; i<  pagevec_count(&pvec); i++) {
>> +			struct page *page = pvec.pages[i];
>> +
>> +			if (PageActive(page)) {
>> +				index = end;
>> +				break;
>> +			}
>> +
>> +			/* We rely upon deletion not changing page->index */
>> +			index = page->index;
>> +			if (index>  end)
>> +				break;
>> +
>> +			if (!trylock_page(page))
>> +				continue;
>> +			WARN_ON(page->index != index);
>> +			ret = invalidate_inode_page(page);
>> +			unlock_page(page);
>> +			count += ret;
>> +		}
>> +		pagevec_release(&pvec);
>> +		mem_cgroup_uncharge_end();
>> +		cond_resched();
>> +		index++;
>> +	}
>> +	return count;
>> +}
>
> We shouldn't just copy-n-paste invalidate_mapping_pages() like this.
> Can't we share the function by passing in a pointer to a callback
> function (invalidate_inode_page or a new
> invalidate_inode_page_unless_it_is_active).
>

Ok, I'll think how to implement this more accurate.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/