Messages in this thread | | | Date | Tue, 23 Jun 2015 13:09:33 +0200 | From | Jan Kara <> | Subject | Re: [RFC][PATCH 1/2] fs: use RCU for free_super() vs. __sb_start_write() |
| |
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@suse.cz> SUSE Labs, CR
| |