History

trylock_super(): replacement for 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 allocator. 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. New variant is called trylock_super() and since it only locks semaphore, callers must call up_read(&sb->s_umount) instead of drop_super(sb) when they're done.

Konstantin Khlebnikov committed 11 years ago

Commit eb6ef3d1 parent 54f2a2f3 files · +22 −26Browse files

Unified · Split

3 files
  1. fs/fs-writeback.c +3 −3
  2. fs/internal.h +1 −1
  3. fs/super.c +18 −22
fs/fs-writeback.c+3−3
@@ -769,9 +769,9 @@ static long __writeback_inodes_wb(struct bdi_writeback *wb, · 768 unmodified lines
769 struct inode *inode = wb_inode(wb->b_io.prev);769 struct inode *inode = wb_inode(wb->b_io.prev);
770 struct super_block *sb = inode->i_sb;770 struct super_block *sb = inode->i_sb;
771 771
772 if (!grab_super_passive(sb)) {772 if (!trylock_super(sb)) {
773 /*773 /*
774 * grab_super_passive() may fail consistently due to774 * trylock_super() may fail consistently due to
775 * s_umount being grabbed by someone else. Don't use775 * s_umount being grabbed by someone else. Don't use
776 * requeue_io() to avoid busy retrying the inode/sb.776 * requeue_io() to avoid busy retrying the inode/sb.
777 */777 */
@@ -779,7 +779,7 @@ static long __writeback_inodes_wb(struct bdi_writeback *wb, · 1 unmodified line
779 continue;779 continue;
780 }780 }
781 wrote += writeback_sb_inodes(sb, wb, work);781 wrote += writeback_sb_inodes(sb, wb, work);
782 drop_super(sb);782 up_read(&sb->s_umount);
783 783
784 /* refer to the same tests at the end of writeback_sb_inodes */784 /* refer to the same tests at the end of writeback_sb_inodes */
785 if (wrote) {785 if (wrote) {
fs/internal.h+1−1
@@ -84,7 +84,7 @@ extern struct file *get_empty_filp(void); · 83 unmodified lines
84 * super.c84 * super.c
85 */85 */
86extern int do_remount_sb(struct super_block *, int, void *, int);86extern int do_remount_sb(struct super_block *, int, void *, int);
87extern bool grab_super_passive(struct super_block *sb);87extern bool trylock_super(struct super_block *sb);
88extern struct dentry *mount_fs(struct file_system_type *,88extern struct dentry *mount_fs(struct file_system_type *,
89 int, const char *, void *);89 int, const char *, void *);
90extern struct super_block *user_get_super(dev_t);90extern struct super_block *user_get_super(dev_t);
fs/super.c+18−22
@@ -71,7 +71,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink, · 70 unmodified lines
71 if (!(sc->gfp_mask & __GFP_FS))71 if (!(sc->gfp_mask & __GFP_FS))
72 return SHRINK_STOP;72 return SHRINK_STOP;
73 73
74 if (!grab_super_passive(sb))74 if (!trylock_super(sb))
75 return SHRINK_STOP;75 return SHRINK_STOP;
76 76
77 if (sb->s_op->nr_cached_objects)77 if (sb->s_op->nr_cached_objects)
@@ -105,7 +105,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink, · 27 unmodified lines
105 freed += sb->s_op->free_cached_objects(sb, sc);105 freed += sb->s_op->free_cached_objects(sb, sc);
106 }106 }
107 107
108 drop_super(sb);108 up_read(&sb->s_umount);
109 return freed;109 return freed;
110}110}
111 111
@@ -118,7 +118,7 @@ static unsigned long super_cache_count(struct shrinker *shrink, · 6 unmodified lines
118 sb = container_of(shrink, struct super_block, s_shrink);118 sb = container_of(shrink, struct super_block, s_shrink);
119 119
120 /*120 /*
121 * Don't call grab_super_passive as it is a potential121 * Don't call trylock_super as it is a potential
122 * scalability bottleneck. The counts could get updated122 * scalability bottleneck. The counts could get updated
123 * between super_cache_count and super_cache_scan anyway.123 * between super_cache_count and super_cache_scan anyway.
124 * Call to super_cache_count with shrinker_rwsem held124 * Call to super_cache_count with shrinker_rwsem held
@@ -348,35 +348,31 @@ static int grab_super(struct super_block *s) __releases(sb_lock) · 223 unmodified lines
348}348}
349 349
350/*350/*
351 * grab_super_passive - acquire a passive reference351 * trylock_super - try to grab ->s_umount shared
352 * @sb: reference we are trying to grab352 * @sb: reference we are trying to grab
353 *353 *
354 * Tries to acquire a passive reference. This is used in places where we354 * Try to prevent fs shutdown. This is used in places where we
355 * cannot take an active reference but we need to ensure that the355 * cannot take an active reference but we need to ensure that the
356 * superblock does not go away while we are working on it. It returns356 * filesystem is not shut down while we are working on it. It returns
357 * false if a reference was not gained, and returns true with the s_umount357 * false if we cannot acquire s_umount or if we lose the race and
358 * lock held in read mode if a reference is gained. On successful return,358 * filesystem already got into shutdown, and returns true with the s_umount
359 * the caller must drop the s_umount lock and the passive reference when359 * lock held in read mode in case of success. On successful return,
360 * done.360 * the caller must drop the s_umount lock when done.
361 *
362 * Note that unlike get_super() et.al. this one does *not* bump ->s_count.
363 * The reason why it's safe is that we are OK with doing trylock instead
364 * of down_read(). There's a couple of places that are OK with that, but
365 * it's very much not a general-purpose interface.
361 */366 */
362bool grab_super_passive(struct super_block *sb)367bool trylock_super(struct super_block *sb)
363{368{
364 spin_lock(&sb_lock);
365 if (hlist_unhashed(&sb->s_instances)) {
366 spin_unlock(&sb_lock);
367 return false;
368 }
369
370 sb->s_count++;
371 spin_unlock(&sb_lock);
372
373 if (down_read_trylock(&sb->s_umount)) {369 if (down_read_trylock(&sb->s_umount)) {
374 if (sb->s_root && (sb->s_flags & MS_BORN))370 if (!hlist_unhashed(&sb->s_instances) &&
371 sb->s_root && (sb->s_flags & MS_BORN))
375 return true;372 return true;
376 up_read(&sb->s_umount);373 up_read(&sb->s_umount);
377 }374 }
378 375
379 put_super(sb);
380 return false;376 return false;
381}377}
382 378

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t