On Fri 19-06-15 15:32:23, Dave Hansen wrote: > > Currently, __sb_start_write() and freeze_super() can race with > each other. __sb_start_write() uses a smp_mb() to ensure that > freeze_super() can see its write to sb->s_writers.counter and > that it can see freeze_super()'s update to sb->s_writers.frozen. > This all seems to work fine. > > But, this smp_mb() makes __sb_start_write() the single hottest > function in the kernel if I sit in a loop and do tiny write()s to > tmpfs over and over. This is on a very small 2-core system, so > it will only get worse on larger systems. > > This _seems_ like an ideal case for RCU. __sb_start_write() is > the RCU read-side and is in a very fast, performance-sensitive > path. freeze_super() is the RCU writer and is in an extremely > rare non-performance-sensitive path. > > Instead of doing and smp_wmb() in __sb_start_write(), we do > rcu_read_lock(). This ensures that a CPU doing freeze_super() > can not proceed past its synchronize_rcu() until the grace > period has ended and the 's_writers.frozen = SB_FREEZE_WRITE' > is visible to __sb_start_write(). > > One question here: Does the work that __sb_start_write() does in > a previous grace period becomes visible to freeze_super() after > its call to synchronize_rcu()? It _seems_ like it should, but it > seems backwards to me since __sb_start_write() is the "reader" in > this case. > > This patch increases the number of writes/second that I can do > by 10.4%. > > Does anybody see any holes with this? Nice speed up and looks good to me. Just one question below. > @@ -1340,7 +1344,7 @@ int freeze_super(struct super_block *sb) > printk(KERN_ERR > "VFS:Filesystem freeze failed\n"); > sb->s_writers.frozen = SB_UNFROZEN; > - smp_wmb(); > + synchronize_rcu(); Do we really need synchronize_rcu() here? We just need to make sure write to sb->s_writers.frozen happens before we start waking processes... > wake_up(&sb->s_writers.wait_unfrozen); > deactivate_locked_super(sb); > return ret; > @@ -1387,7 +1391,7 @@ int thaw_super(struct super_block *sb) > > out: > sb->s_writers.frozen = SB_UNFROZEN; > - smp_wmb(); > + synchronize_rcu(); > wake_up(&sb->s_writers.wait_unfrozen); And here as well... Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
