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
| ||
|
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/
Powered by blists - more mailing lists