Re: [PATCH, RFC] xfs: remove i_iolock and use i_rwsem in the VFS inode instead

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Sep 09, 2016 at 11:06:01AM +1000, Dave Chinner wrote:
> On Thu, Sep 08, 2016 at 08:54:42AM +0200, Peter Zijlstra wrote:

> > We've added strict owner semantics to rwsem a long time ago.
> 
> /me sighs.
> 
> History repeats.

I doubt it, this was 2006 we're talking about.

> From my perspective, lockdep is a very poor replacement for
> architecting a robust locking model and then implementing code that
> directly asserts that the correct locks are held at the correct
> time.

Very few people people architect locking :-/, but lockdep as the assert,
and I've been encouraging people to use that instead of comments like:

  /* this function should be called with foo lock held */

Now, the problem is that lockdep also asserts the caller is the lock
owner, and not some other random thing.

And given the amount of locking fail caught by lockdep (still..) you
really cannot argue against it.

Sure its a pain at times, but so is pretty much everything.

> > you should use
> > {down,up}_read_non_owner().
> > I'm not sure we've got write_non_owner() variants for this.
> 
> For the case Christoph reported, it's the write side context of the
> inode locks that is handed off to other threads. And no, we don't
> have annotations for that.

So the xfs mrlock already uses rwsem, semantics have not changed. So the
case Christoph found should already conform to the owner semantics.

I've not looked at code, but if the worker is synchronized against the
critical section (it'd pretty much have to be to rely on its locking)
there's nothing wrong, its just that the lockdep_assert_held() thingies
cannot work as it.

That is:

	task A				task B

	down_write(&rwsem);
	queue_work(&work);
					worker()
					  lockdep_assert_held(&rwsem);
	flush_work(&work);
	up_write(&rwsem);


Doesn't work. Explicitly so in fact.

Does the worker have a pointer back to taskA ? I could try something
like lockdep_assert_held_by(lock, task), just need to be careful,
the per-task lock state is just that, per-task, no serialization.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs