change sb_writers to use percpu_rw_semaphore

Preparation. get_super_thawed() can do sb_start/end_write() with the same effect, we are going to kill sb_writers->wait_unfrozen.

---
 fs/super.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

fs/super.c +0 -0

index 928c20f..5ea0edd 100644

fs/super.c +0 -0

 		put_super(s);
 	}
 }

Signature · 1 line

1.5.5.1

fs/super.c struct super_block *get_super_thawed(struct block_device *bdev) · line 625

Quoted 9 lines

fs/super.c +2 -2

@@ -621,8 +621,8 @@ struct super_block *get_super_thawed(struct block_device *bdev)
 		if (!s || s->s_writers.frozen == SB_UNFROZEN)
 			return s;
 		up_read(&s->s_umount);
-		wait_event(s->s_writers.wait_unfrozen,
-			   s->s_writers.frozen == SB_UNFROZEN);
+		sb_start_write(s);
+		sb_end_intwrite(s);

This is definitely buggy - you need to start and end freeze protection at the same level...

Hello,

Al, Jan, could you comment? I mean the intent, the patches are obviously not for inclusion yet.

We can remove everything from struct sb_writers except frozen (which can become a boolean, it seems) and add the array of percpu_rw_semaphore's instead.

__sb_start/end_write() can use percpu_down/up_read(), and freeze/thaw_super() can use percpu_down/up_write().

Why:

- Firstly, __sb_start_write() looks simply buggy. I does __sb_end_write() if it sees ->frozen, but if it migrates to another CPU before percpu_counter_dec() sb_wait_write() can wrongly succeed if there is another task which holds the same "semaphore": sb_wait_write() can miss the result of the previous percpu_counter_inc() but see the result of this percpu_counter_dec().

- This code doesn't look simple. It would be better to rely on the generic locking code.

- __sb_start_write() will be a little bit faster, but this is minor.

Todo:

- __sb_start_write(wait => false) always fail.

Thivial, we already have percpu_down_read_trylock() just this patch wasn't merged yet.

- sb_lockdep_release() and sb_lockdep_acquire() play with percpu_rw_semaphore's internals.

Trivial, we need a couple of new helper in percpu-rwsem.c.

- Fix get_super_thawed(), it will spin if MS_RDONLY...

It is not clear to me what exactly should we do, but this doesn't look hard. Perhaps it can just return if MS_RDONLY.

- Most probably I missed something else, and I do not need how to test.

[ Please cc [email protected] on filesystem infrastructure changes! ]

Quoted 5 lines

- sb_lockdep_release() and sb_lockdep_acquire() play with percpu_rw_semaphore's internals.

Trivial, we need a couple of new helper in percpu-rwsem.c.

- try compiling XFS, watch it break on freeze lockdep annotations

Quoted 2 lines

- Most probably I missed something else, and I do not need how to test.

xfstests has many freeze related stress tests. IIRC, generic/068 is the test that historically causes the most problems for freeze infrastructure changes. You'll also need to test at least ext4, XFS and btrfs, because they all stress the freeze code differently. Testing XFS, in particular, is a good idea because it has several custom freeze tests that aren't run on any other filesystem type.

Quoted 3 lines · Dave Chinner

[ Please cc [email protected] on filesystem infrastructure changes! ]

OK, will do.

Quoted 9 lines · Oleg Nesterov

Quoted 5 lines

- sb_lockdep_release() and sb_lockdep_acquire() play with percpu_rw_semaphore's internals.

Trivial, we need a couple of new helper in percpu-rwsem.c.

- try compiling XFS, watch it break on freeze lockdep annotations

Thanks a lot! I see. Still trivial, xfs can use the same helpers rather the abuse lockdep directly.

Quoted 9 lines

Quoted 2 lines

- Most probably I missed something else, and I do not need how to test.

xfstests has many freeze related stress tests. IIRC, generic/068 is the test that historically causes the most problems for freeze infrastructure changes. You'll also need to test at least ext4, XFS and btrfs, because they all stress the freeze code differently. Testing XFS, in particular, is a good idea because it has several custom freeze tests that aren't run on any other filesystem type.

Thanks again.

Do you see something fundamentally wrong with this change?

Al, Jan, could you comment? I mean the intent, the patches are obviously not for inclusion yet.

We can remove everything from struct sb_writers except frozen (which can become a boolean, it seems) and add the array of

percpu_rw_semaphore's instead.

__sb_start/end_write() can use percpu_down/up_read(), and freeze/thaw_super() can use percpu_down/up_write().

Why:

- Firstly, __sb_start_write() looks simply buggy. I does __sb_end_write() if it sees ->frozen, but if it migrates to another CPU before percpu_counter_dec() sb_wait_write() can wrongly succeed if there is another task which holds the same "semaphore": sb_wait_write() can miss the result of the previous percpu_counter_inc() but see the result of this percpu_counter_dec().

- This code doesn't look simple. It would be better to rely on the generic locking code.

- __sb_start_write() will be a little bit faster, but this is minor.

Todo:

- __sb_start_write(wait => false) always fail.

Thivial, we already have percpu_down_read_trylock() just this patch wasn't merged yet.

- sb_lockdep_release() and sb_lockdep_acquire() play with percpu_rw_semaphore's internals.

Trivial, we need a couple of new helper in percpu-rwsem.c.

- Fix get_super_thawed(), it will spin if MS_RDONLY...

It is not clear to me what exactly should we do, but this doesn't look hard. Perhaps it can just return if MS_RDONLY.

- Most probably I missed something else, and I do not need how to test.

Finally. freeze_super() calls synchronize_sched_expedited() 3 times in a row. This is bad and just stupid. But if we change percpu_rw_semaphore to use rcu_sync (see https://lkml.org/lkml/2015/7/11/211) we can avoid this and do synchronize_sched() only once. Just we need some more simple changes in percpu-rwsem.c, so that all sb_writers->rw_sem[] semaphores could use the single sb_writers->rss.

In this case destroy_super() needs some modifications too, percpu_free_rwsem() will be might_sleep(). But this looks simple too.

Hello,

Quoted 2 lines · Oleg Nesterov

Al, Jan, could you comment? I mean the intent, the patches are obviously not for inclusion yet.

Thanks for the patches! Hum, what do people have with freeze protection these days? Noone cared about it for years and sudddently two patch sets within a month :) Anyway, have you seen the patch set from Dave Hansen?

It is here: https://lkml.org/lkml/2015/6/24/682
He modifies the freezing primitives in a different way. AFAICS the resulting performance of the fast path should be about the same.

So unless
I'm missing something and there is a significant performance advantage to Dave's patches I'm all for using a generic primitive you suggest.

Can you perhaps work with Dave on some common resolution?

Quoted 2 lines

- __sb_start_write() will be a little bit faster, but this is minor.

Actually Dave could measure the gain achieved by removing the barrier. It would be good to verify that your patches achieve a similar gain.

Quoted 4 lines

- Fix get_super_thawed(), it will spin if MS_RDONLY...

It is not clear to me what exactly should we do, but this doesn't look hard. Perhaps it can just return if MS_RDONLY.

What's the exact problem here?

Quoted 3 lines · Jan Kara

So unless
I'm missing something and there is a significant performance advantage to Dave's patches I'm all for using a generic primitive you suggest.

I think percpu_rw_semaphore looks a bit better. And even a bit faster. And it will not block __sb_start_write() entirely while freeze_super() sleeps in synchronize_rcu().

freeze_super() should be faster too after rcu_sync changes, but this is not that important.

But again, to me the main advantage is that we can use the generic primitives and remove this nontrivial code in fs/super.c.

Quoted 1 line

Can you perhaps work with Dave on some common resolution?

Dave, what do you think? Will you agree with percpu_rw_semaphore ?

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Jan Kara

So unless
I'm missing something and there is a significant performance advantage to Dave's patches I'm all for using a generic primitive you suggest.

Quoted 3 lines

I think percpu_rw_semaphore looks a bit better. And even a bit faster. And it will not block __sb_start_write() entirely while freeze_super() sleeps in synchronize_rcu().

That's true, but freeze_super() and the code blocked by it is a super-rare path compared with write().

Quoted 6 lines

freeze_super() should be faster too after rcu_sync changes, but this is not that important.

But again, to me the main advantage is that we can use the generic primitives and remove this nontrivial code in fs/super.c.

Quoted 3 lines

Quoted 1 line

Can you perhaps work with Dave on some common resolution?

Dave, what do you think? Will you agree with percpu_rw_semaphore ?

Using my little write-1-byte test (under will-it-scale), your 4 patches improves the number of writes/sec by 12%. My 3 patches improve the number of writes/sec by 32%.

My patches manage to get rid of the memory barriers entirely in the fast path. Your approach keeps the barriers.

Quoted 3 lines

Using my little write-1-byte test (under will-it-scale), your 4 patches improves the number of writes/sec by 12%. My 3 patches improve the number of writes/sec by 32%.

Quoted 3 lines · Oleg Nesterov

Quoted 3 lines

Using my little write-1-byte test (under will-it-scale), your 4 patches improves the number of writes/sec by 12%. My 3 patches improve the number of writes/sec by 32%.

I looked at it again. I tested with this patch in addition to the ones modifying __sb_start/end_write():

	https://lkml.org/lkml/2015/6/24/682

That is where the performance delta came from. Your patches (plus the fsnotify optimization) perform very similarly to my approach.

Yours remove so much code that I think they are the preferable approach.

They don't compile with lockdep on, btw. :)

Quoted 6 lines · Dave Hansen

I looked at it again. I tested with this patch in addition to the ones modifying __sb_start/end_write():

	https://lkml.org/lkml/2015/6/24/682

Quoted 6 lines

That is where the performance delta came from. Your patches (plus the fsnotify optimization) perform very similarly to my approach.

Yours remove so much code that I think they are the preferable approach.

They don't compile with lockdep on, btw. :)

Great, thanks for hashing it out.

So I'm also in favor of Oleg's approach
as well. We just have to wait until he fixes the outstanding issues with his code.

Dave, just send your fsnotify patch separately to AKPM - he usually merges fsnotify stuff.

Quoted 3 lines · Jan Kara

So I'm also in favor of Oleg's approach
as well. We just have to wait until he fixes the outstanding issues with his code.

Yes. I'll try to make the working version, hopefully this week.

But,

Quoted 2 lines

Dave, just send your fsnotify patch separately to AKPM - he usually merges fsnotify stuff.

Perhaps it makes to merge other 2 patches from Dave first? (those which change __sb_start/end_write to rely on RCU). Afaics these changes are straightforward and correct. Although I'd suggest to use preempt_disable() and synchronize_sched() instead. I will be happy to (try to) make this conversion on top of his changes.

Because I do not want to delay the performance improvements and I do not know when exactly I'll send the next version: I need to finish the previous discussion about rcu_sync first. And the necessary changes in fs/super.c depend on whether percpu_rw_semaphore will have rcu_sync or not (not too much, only destroy_super() depends, but still).

And of course, I am worried that I missed something and percpu_rw_semaphore can't work for some reason. The code in fs/super.c looks simple, but it seems that filesystems do the "strange" things with lockdep at least.

So Dave's patches would go in only in the next merge window anyway so we still have like two-three weeks to decide which patchset to take. If you think it will take you longer, then merging Dave's patches makes some sense although I personally don't think the issue is so important that we have to fix it ASAP and eventual delay of one more release would be OK for me.

Quoted 5 lines · Jan Kara

So Dave's patches would go in only in the next merge window anyway so we still have like two-three weeks to decide which patchset to take. If you think it will take you longer, then merging Dave's patches makes some sense although I personally don't think the issue is so important that we have to fix it ASAP and eventual delay of one more release would be OK for me.

I thought of one more thing.

Oleg's patches are fast in the uncontended case. My testing of his code so far was all single-threaded. Will it slow down if we throw a bunch of threads at it an see contention? I don't think my RCU approach will.

I can benchmark in the next few days on a larger machine.

Quoted 13 lines · Oleg Nesterov

Perhaps it makes to merge other 2 patches from Dave first? (those which change __sb_start/end_write to rely on RCU). Afaics these changes are straightforward and correct. Although I'd suggest to use preempt_disable() and synchronize_sched() instead. I will be happy to (try to) make this conversion on top of his changes.

Because I do not want to delay the performance improvements and I do not know when exactly I'll send the next version: I need to finish the previous discussion about rcu_sync first. And the necessary changes in fs/super.c depend on whether percpu_rw_semaphore will have rcu_sync or not (not too much, only destroy_super() depends, but still).

Quoted 3 lines

And of course, I am worried that I missed something and percpu_rw_semaphore can't work for some reason. The code in fs/super.c looks simple, but it seems that filesystems do the "strange" things with lockdep at least.

So Dave's patches would go in only in the next merge window anyway so we still have like two-three weeks to decide which patchset to take.

If you
think it will take you longer,

then merging Dave's patches makes some sense
although I personally don't think the issue is so important that we have to fix it ASAP and eventual delay of one more release would be OK for me.

#ifdef CONFIG_LOCKDEP
/*
* We want lockdep to tell us about possible deadlocks with freezing but
* it's it bit tricky to properly instrument it. Getting a freeze protection
* works as getting a read lock but there are subtle problems. XFS for example
* gets freeze protection on internal level twice in some cases, which is OK

Quoted 7 lines · Oleg Nesterov

#ifdef CONFIG_LOCKDEP
/*
* We want lockdep to tell us about possible deadlocks with freezing but
* it's it bit tricky to properly instrument it. Getting a freeze protection
* works as getting a read lock but there are subtle problems. XFS for example
* gets freeze protection on internal level twice in some cases, which is OK

Sorry, I've missed something here - where is XFS nesting sb_start_intwrite() calls?

Quoted 10 lines · Dave Chinner

Quoted 7 lines · Oleg Nesterov

#ifdef CONFIG_LOCKDEP
/*
* We want lockdep to tell us about possible deadlocks with freezing but
* it's it bit tricky to properly instrument it. Getting a freeze protection
* works as getting a read lock but there are subtle problems. XFS for example
* gets freeze protection on internal level twice in some cases, which is OK

Quoted 2 lines

Sorry, I've missed something here - where is XFS nesting sb_start_intwrite() calls?

Heh ;) I too tried to understand thi but failed. I was not surprized, I know nothing about fs/.

Dave, I didn't write this comment. Please look at acquire_freeze_lock(). If we can remove this logic - great! but this needs a separate change.

Quoted 3 lines · Oleg Nesterov

Dave, I didn't write this comment. Please look at acquire_freeze_lock(). If we can remove this logic - great! but this needs a separate change.

Oh, I think I know what it was - when we duplicate a transaction for a rolling commit, we do it before committing the current transaction is committed. I *think* that used to take a second freeze reference, which only existed until the first transaction was committed. We do things a bit differently now - we hold a state flag on the transaction to indicate it needs to release the freeze reference when it is freed and we pass it to the new transaction so that the first transaction commit doesn't release it.

So, yes, it may well be a stale comment now.

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Dave Chinner

Quoted 7 lines · Oleg Nesterov

#ifdef CONFIG_LOCKDEP
/*
* We want lockdep to tell us about possible deadlocks with freezing but
* it's it bit tricky to properly instrument it. Getting a freeze protection
* works as getting a read lock but there are subtle problems. XFS for example
* gets freeze protection on internal level twice in some cases, which is OK

Quoted 8 lines

Quoted 2 lines

Sorry, I've missed something here - where is XFS nesting sb_start_intwrite() calls?

Heh ;) I too tried to understand thi but failed. I was not surprized, I know nothing about fs/.

Dave, I didn't write this comment. Please look at acquire_freeze_lock(). If we can remove this logic - great! but this needs a separate change.

Oh, I think I know what it was - when we duplicate a transaction for a rolling commit, we do it before committing the current transaction is committed. I *think* that used to take a second freeze reference, which only existed until the first transaction was committed. We do things a bit differently now - we hold a state flag on the transaction to indicate it needs to release the freeze reference when it is freed and we pass it to the new transaction so that the first transaction commit doesn't release it.

So, yes, it may well be a stale comment now.

Quoted 2 lines · Dave Chinner

So, yes, it may well be a stale comment now.

Yeah, as far as I remember this was the reason why I added the comment. So Oleg, feel free to remove the special code and run xfstests with XFS and lockdep enabled to verify there are really no issues. Thanks!