fs: avoid locking sb_lock in grab_super_passive()

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.

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 <[email protected]>

---
 fs/fs-writeback.c |    2 +-
 fs/super.c        |   18 ++++--------------
 2 files changed, 5 insertions(+), 15 deletions(-)

fs/fs-writeback.c +1 -1

index 073657f..3e92bb7 100644
@@ -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) {

fs/super.c +4 -15

index 65a53ef..6ae33ed 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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.

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.

- 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.

Quoted 4 lines · Andrew Morton

- 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.

Reference to struct super_block that guarantees only that its memory won't be freed until we drop it.

Quoted 1 line

After your patch, this superblock state no longer exists(?),

Yes, it does. The _only_ reason why that patch isn't outright bogus is that we do only down_read_trylock() on ->s_umount - try to pull off the same thing with down_read() and you'll get a nasty race.

Take a look at e.g.
get_super(). Or user_get_super(). Or iterate_supers()/iterate_supers_type(), where we don't return such references, but pass them to a callback instead. In all those cases we end up with passive reference taken, ->s_umount taken shared (_NOT_ with trylock) and fs checked for being still alive. Then it's guaranteed to stay alive until we do drop_super().

I agree that the name blows, BTW - something like try_get_super() might have been more descriptive, but with this change it actually becomes a bad name as well, since after it we need a different way to release the obtained ref; not the same as after get_super(). Your variant might be OK, but I'd probably make it trylock_super(), to match the verb-object order of the rest of identifiers in that area...

Quoted 3 lines

so
perhaps the entire "passive reference" concept and any references to it can be expunged from the kernel.

Nope.

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.

Quoted 10 lines · Konstantin Khlebnikov

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.

Excuse me, but this part is BS - its call is immediately preceded by

        if (!(sc->gfp_mask & __GFP_FS))
                return SHRINK_STOP;

and if we *ever* hit GFP_FS allocation from interrupt, we are really screwed. If nothing else, both prune_dcache_sb() and prune_icache_sb() can wait for all kinds of IO; you really don't want that called in an interrupt context. The same goes for writeback_sb_inodes(), while we are at it.

If you ever see that in an interrupt context, you have a very bad problem on hands.

Said that, not bothering with sb_lock (and ->s_count) in those two callers makes sense. Applied, with name changed to trylock_super().

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>