linux-kernel - Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150713222309.GM3902@dastard>
Date:	Tue, 14 Jul 2015 08:23:09 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Al Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Daniel Wagner <daniel.wagner@...-carit.de>,
	Davidlohr Bueso <dave@...olabs.net>,
	Ingo Molnar <mingo@...hat.com>, Tejun Heo <tj@...nel.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

[ Please cc linux-fsdevel@...r.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@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/