linux-kernel - Re: [PATCH] fs: avoid locking sb_lock in grab_super

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]

Message-Id: <20150220150731.e79cd30dc6ecf3c7a3f5caa3@linux-foundation.org>
Date:	Fri, 20 Feb 2015 15:07:31 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
Cc:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Dave Chinner <david@...morbit.com>
Subject: Re: [PATCH] fs: avoid locking sb_lock in grab_super_passive()

On Thu, 19 Feb 2015 20:19:35 +0300 Konstantin Khlebnikov <khlebnikov@...dex-team.ru> wrote:
>

Please cc Dave Chinner on this.

> I've noticed significant locking contention in memory reclaimer around
> sb_lock inside grab_super_passive(). Grab_super_passive() is called from
> two places: in icache/dcache shrinkers (function super_cache_scan) and
> from writeback (function __writeback_inodes_wb). Both are required for
> progress in memory reclaimer.
> 
> Also this lock isn't irq-safe. And I've seen suspicious livelock under
> serious memory pressure where reclaimer was called from interrupt which
> have happened right in place where sb_lock is held in normal context,
> so all other cpus were stuck on that lock too.

You mean someone is calling grab_super_passive() (ie: fs writeback)
from interrupt context?  What's the call path?

> Grab_super_passive() acquires sb_lock to increment sb->s_count and check
> sb->s_instances. It seems sb->s_umount locked for read is enough here:
> super-block deactivation always runs under sb->s_umount locked for write.
> Protecting super-block itself isn't a problem: in super_cache_scan() sb
> is protected by shrinker_rwsem: it cannot be freed if its slab shrinkers
> are still active. Inside writeback super-block comes from inode from bdi
> writeback list under wb->list_lock.
> 
> This patch removes locking sb_lock and checks s_instances under s_umount:
> generic_shutdown_super() unlinks it under sb->s_umount locked for write.
> Now successful grab_super_passive() only locks semaphore, callers must
> call up_read(&sb->s_umount) instead of drop_super(sb) when they're done.
> 

The patch looks reasonable to me, but the grab_super_passive()
documentation needs further updating, please.

- It no longer "acquires a reference".  All it does is to acquire an rwsem.

- What the heck is a "passive reference" anyway?  It appears to be
  the situation where we increment s_count without incrementing s_active.

  After your patch, this superblock state no longer exists(?), so
  perhaps the entire "passive reference" concept and any references to
  it can be expunged from the kernel.

  And grab_super_passive() should be renamed anyway.  It no longer
  "grabs" anything - it attempts to acquire ->s_umount. 
  "super_trylock", maybe?

- While we're dicking with the grab_super_passive() documentation,
  let's turn it into kerneldoc by adding the /**.  
--
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/