Re: xfstests 344 deadlock on NOVA

[Andiry Xu <jix024@eng.ucsd.edu>]

On Tue, Feb 27, 2018 at 1:41 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Feb 27, 2018 at 11:15:55AM -0800, Andiry Xu wrote:
>> Hi,
>>
>> I encounter a problem when running xfstests on NOVA. I appreciate your
>> help very much.
>>
>> Some background: NOVA adopts a per-inode logging design. Metadata
>> changes are appended to the log and persisted before returning to the
>> user space. For example, a write() in NOVA works like this:
>>
>> Allocate new pmem blocks and fill with user data
>> Append the metadata that describes this write to the end of the inode log
>> Update the log tail pointer atomically to commit the write
>> Update in-DRAM radix tree to point to the metadata (for fast lookup)
>>
>> The log appending and radix tree update are protected by inode_lock().
>>
>> For mmap, nova_dax_get_blocks (similar to ext2_get_blocks)  needs to
>> persist the metadata for new allocations. So it has to append the new
>> allocation metadata to the log, and hence needs to lock the inode.
>> This causes deadlock in xfstests 344 with concurrent pwrite and mmap
>> write:
>>
>> Thread 1:
>> pwrite
>> -> nova_dax_file_write()
>> ---> inode_lock()
>> -----> invalidate_inode_pages2_range()
>> -------> schedule()
>
> Why did this thread schedule here?
>
>> Thread 2:
>> dax_fault
>> -> nova_dax_get_blocks()
>> ---> inode_lock() // deadlock
>
> It's just waiting on an inode_lock() to be released by another
> thread. What resource is it holding locked that the first thread
> needs to make progress?
>
>> If I remove invalidate_inode_pages2_range() in write path, xfstests
>> 344 passed, but 428 will fail.
>>
>> It appears to me that ext2/ext4 does not have this issue because they
>> don't persist metadata immediately and hence do not take inode lock.
>
> Did you realise that you can't take the inode->i_rwsem in the page
> fault path (i.e. under the mmap_sem) because that's a known deadlock
> vector against the read/write IO path?
>
> (i.e. you can use a mmap'd buffer over a range of the same file as
> the data buffer for the IO, then take a page fault when trying to
> copy data into/out of that buffer while holding the inode->i_rwsem)
>
>> But nova_dax_get_blocks() has to persist the metadata and needs to
>> lock the inode to access the log. Is there a way to workaround this?
>> Thank you very much.
>
> I'm pretty sure you don't want to use inode->i_rwsem for internal
> metadata serialisation requirements. XFS uses xfs_inode->i_ilock for
> this, ext4 uses a combination of other locks, etc, and it's done to
> separate internal serialisation requirements from user data access
> and VFS serialisation requirements...
>

Thank you. I have fixed the issue by using a different semaphore.

Thanks,
Andiry

> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com