Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

[Dave Chinner]

[ Please cc linux-fsdevel@vger.kernel.org on filesystem
infrastructure changes! ]

On Mon, Jul 13, 2015 at 11:25:36PM +0200, Oleg Nesterov wrote:
> 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.

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

> 	- 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.

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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

[Oleg Nesterov]

On 07/14, Dave Chinner wrote:
>
> [ Please cc linux-fsdevel@vger.kernel.org on filesystem
> infrastructure changes! ]

OK, will do.

> On Mon, Jul 13, 2015 at 11:25:36PM +0200, Oleg Nesterov wrote:
> >
> > 	- 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.

> > 	- 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?

Oleg.

Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

[Dave Chinner]

On Tue, Jul 14, 2015 at 12:42:37AM +0200, Oleg Nesterov wrote:
> On 07/14, Dave Chinner wrote:
> >
> > [ Please cc linux-fsdevel@vger.kernel.org on filesystem
> > infrastructure changes! ]
> 
> OK, will do.
> 
> > On Mon, Jul 13, 2015 at 11:25:36PM +0200, Oleg Nesterov wrote:
> > >
> > > 	- 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.
> 
> > > 	- 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?

I haven't looked particularly closely at the implementation, just
enough to get an idea of the semantics of the new infrasructure (I
didn't know that per-cpu rwsems existed!). The freeze code is
essentially a multi-level read-optimised read/write barrier and
AFAICT the per-cpu rw-sem has those semantics. From that perspective
I don't see any fundamental problems, but there may be details that
I've missed....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com