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>