On Mon, Jul 08, 2019 at 03:31:14PM +0200, Jan Kara wrote: > On Sat 06-07-19 09:31:57, Dave Chinner wrote: > > On Wed, Jul 03, 2019 at 03:04:45AM +0300, Boaz Harrosh wrote: > > > On 20/06/2019 01:37, Dave Chinner wrote: > > > <> > > > > > > > > I'd prefer it doesn't get lifted to the VFS because I'm planning on > > > > getting rid of it in XFS with range locks. i.e. the XFS_MMAPLOCK is > > > > likely to go away in the near term because a range lock can be > > > > taken on either side of the mmap_sem in the page fault path. > > > > > > > <> > > > Sir Dave > > > > > > Sorry if this was answered before. I am please very curious. In the zufs > > > project I have an equivalent rw_MMAPLOCK that I _read_lock on page_faults. > > > (Read & writes all take read-locks ...) > > > The only reason I have it is because of lockdep actually. > > > > > > Specifically for those xfstests that mmap a buffer then direct_IO in/out > > > of that buffer from/to another file in the same FS or the same file. > > > (For lockdep its the same case). > > > > Which can deadlock if the same inode rwsem is taken on both sides of > > the mmap_sem, as lockdep tells you... > > > > > I would be perfectly happy to recursively _read_lock both from the top > > > of the page_fault at the DIO path, and under in the page_fault. I'm > > > _read_locking after all. But lockdep is hard to convince. So I stole the > > > xfs idea of having an rw_MMAPLOCK. And grab yet another _write_lock at > > > truncate/punch/clone time when all mapping traversal needs to stop for > > > the destructive change to take place. (Allocations are done another way > > > and are race safe with traversal) > > > > > > How do you intend to address this problem with range-locks? ie recursively > > > taking the same "lock"? because if not for the recursive-ity and lockdep I would > > > not need the extra lock-object per inode. > > > > As long as the IO ranges to the same file *don't overlap*, it should > > be perfectly safe to take separate range locks (in read or write > > mode) on either side of the mmap_sem as non-overlapping range locks > > can be nested and will not self-deadlock. > > I'd be really careful with nesting range locks. You can have nasty > situations like: > > Thread 1 Thread 2 > read_lock(0,1000) > write_lock(500,1500) -> blocks due to read lock > read_lock(1001,1500) -> blocks due to write lock (or you have to break > fairness and then deal with starvation issues). > > So once you allow nesting of range locks, you have to very carefully define > what is and what is not allowed. Yes. I do understand the problem with rwsem read nesting and how that can translate to reange locks. That's why my range locks don't even try to block on other pending waiters. The case where read nesting vs write might occur like above is something like copy_file_range() vs truncate, but otherwise for IO locks we simply don't have arbitrarily deep nesting of range locks. i.e. for your example my range lock would result in: Thread 1 Thread 2 read_lock(0,1000) write_lock(500,1500) <finds conflicting read lock> <marks read lock as having a write waiter> <parks on range lock wait list> <...> read_lock_nested(1001,1500) <no overlapping range in tree> <gets nested range lock> <....> read_unlock(1001,1500) <stays blocked because nothing is waiting on (1001,1500) so no wakeup> <....> read_unlock(0,1000) <sees write waiter flag, runs wakeup> <gets woken> <retries write lock> <write lock gained> IOWs, I'm not trying to complicate the range lock implementation with complex stuff like waiter fairness or anti-starvation semantics at this point in time. Waiters simply don't impact whether a new lock can be gained or not, and hence the limitations of rwsem semantics don't apply. If such functionality is necessary (and I suspect it will be to prevent AIO from delaying truncate and fallocate-like operations indefinitely) then I'll add a "barrier" lock type (e.g. range_lock_write_barrier()) that will block new range lock attempts across it's span. However, because this can cause deadlocks like the above, a barrier lock will not block new *_lock_nested() or *_trylock() calls, hence allowing runs of nested range locking to complete and drain without deadlocking on a conflicting barrier range. And that still can't be modelled by existing rwsem semantics.... > That's why in my range lock implementation > ages back I've decided to treat range lock as a rwsem for deadlock > verification purposes. IMO, treating a range lock as a rwsem for deadlock purposes defeats the purpose of adding range locks in the first place. The concurrency models are completely different, and some of the limitations on rwsems are a result of implementation choices rather than limitations of a rwsem construct. In reality I couldn't care less about what lockdep can or can't verify. I've always said lockdep is a crutch for people who don't understand locks and the concurrency model of the code they maintain. That obviously extends to the fact that lockdep verification limitations should not limit what we allow new locking primitives to do. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
