On Thu, Feb 19, 2015 at 8:19 PM, Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx> wrote: > 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 s/reclaimer/allocator/ > have happened right in place where sb_lock is held in normal context, > so all other cpus were stuck on that lock too. > > 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. > > Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx> > --- > fs/fs-writeback.c | 2 +- > fs/super.c | 18 ++++-------------- > 2 files changed, 5 insertions(+), 15 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 073657f..3e92bb7 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -779,7 +779,7 @@ static long __writeback_inodes_wb(struct bdi_writeback *wb, > continue; > } > wrote += writeback_sb_inodes(sb, wb, work); > - drop_super(sb); > + up_read(&sb->s_umount); > > /* refer to the same tests at the end of writeback_sb_inodes */ > if (wrote) { > diff --git a/fs/super.c b/fs/super.c > index 65a53ef..6ae33ed 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -105,7 +105,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink, > freed += sb->s_op->free_cached_objects(sb, sc); > } > > - drop_super(sb); > + up_read(&sb->s_umount); > return freed; > } > > @@ -356,27 +356,17 @@ static int grab_super(struct super_block *s) __releases(sb_lock) > * superblock does not go away while we are working on it. It returns > * false if a reference was not gained, and returns true with the s_umount > * lock held in read mode if a reference is gained. On successful return, > - * the caller must drop the s_umount lock and the passive reference when > - * done. > + * the caller must drop the s_umount lock when done. > */ > bool grab_super_passive(struct super_block *sb) > { > - spin_lock(&sb_lock); > - if (hlist_unhashed(&sb->s_instances)) { > - spin_unlock(&sb_lock); > - return false; > - } > - > - sb->s_count++; > - spin_unlock(&sb_lock); > - > if (down_read_trylock(&sb->s_umount)) { > - if (sb->s_root && (sb->s_flags & MS_BORN)) > + if (!hlist_unhashed(&sb->s_instances) && > + sb->s_root && (sb->s_flags & MS_BORN)) > return true; > up_read(&sb->s_umount); > } > > - put_super(sb); > return false; > } > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@xxxxxxxxx. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>