linux-kernel - Re: [PATCH 06/16] f2fs: add node operations

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:	Thu, 11 Oct 2012 09:17:12 +1100
From:	NeilBrown <neilb@...e.de>
To:	김재극 <jaegeuk.kim@...sung.com>
Cc:	viro@...iv.linux.org.uk, 'Theodore Ts'o' <tytso@....edu>,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	chur.lee@...sung.com, cm224.lee@...sung.com,
	jooyoung.hwang@...sung.com
Subject: Re: [PATCH 06/16] f2fs: add node operations

On Fri, 05 Oct 2012 21:00:29 +0900 김재극 <jaegeuk.kim@...sung.com> wrote:

> +static struct nat_entry *grab_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid)
> +{
> +	struct nat_entry *new;
> +
> +	new = kmem_cache_alloc(nat_entry_slab, __GFP_HIGH | __GFP_NOFAIL);
> +	memset(new, 0, sizeof(struct nat_entry));
> +	nat_set_nid(new, nid);
> +	__add_to_nat_cache(nm_i, new);
> +	return new;
> +}
> +
> +static void cache_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid,
> +						struct f2fs_nat_entry *ne)
> +{
> +	struct nat_entry *e;
> +
> +	write_lock(&nm_i->nat_tree_lock);
> +	e = __lookup_nat_cache(nm_i, nid);
> +	if (!e) {
> +		e = grab_nat_entry(nm_i, nid);
> +		nat_set_blkaddr(e, le32_to_cpu(ne->block_addr));
> +		nat_set_ino(e, le32_to_cpu(ne->ino));
> +		nat_set_version(e, ne->version);
> +		e->checkpointed = true;
> +	}
> +	write_unlock(&nm_i->nat_tree_lock);
> +}

Hi,

cache_nat_entry takes an rwlock (like a spinlock), then calls grab_nat_cache,
which calls kmem_cache_alloc().  Doing mem alloc under a spinlock is not
really a good idea, though it is sometimes OK for GFP_ATOMIC.

You use __GFP_HIGH which is equivalent to GFP_ATOMIC, but add __GFP_NOFAIL.
I'm not really sure exactly what this will do when memory is tight, but I
suspect it will spin trying to find memory and there by slow down any other
code that is trying to free memory.

Also, there is a comment in page_alloc.c:
			 * __GFP_NOFAIL is not to be used in new code.
			 *
			 * All __GFP_NOFAIL callers should be fixed so that they
			 * properly detect and handle allocation failures.
			 *

I suggest you fix this code to follow the standard pattern where if the
lookup_nat_cache fails you check if you have already allocated something and
if so use it.  If not, drop the lock, do the allocation with GFP_KERNEL, then
loop back to the top.
At the end, if you allocated something without using it, kfree it.

> +static int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink)
> +{
> +	struct f2fs_nm_info *nm_i = NM_I(sbi);
> +
> +	if (nm_i->nat_cnt < 2 * NM_WOUT_THRESHOLD)
> +		return 0;
> +
> +	write_lock(&nm_i->nat_tree_lock);
> +	while (nr_shrink-- && !list_empty(&nm_i->nat_entries)) {
> +		struct nat_entry *ne;
> +		ne = list_first_entry(&nm_i->nat_entries,
> +					struct nat_entry, list);
> +		__del_from_nat_cache(nm_i, ne);
> +	}
> +	write_unlock(&nm_i->nat_tree_lock);
> +	return nr_shrink;
> +}

This code looks wrong to me, in a small way.
The return value is only ever tested for whether it is zero or not.
For that last 'return', nr_shrink will only be zero if the original nr_shrink
is exactly one more than the number of items in nat_entries.  If nr_shrink is
more than that, the function will return "-1".
I suspect that is not what is desired.

I would suggest changing the test in the while loop to
      while (nr_shrink && !list_empty(...)) {
and add
      nr_shrink--;
somewhere inside the loop.

Regards,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)