Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 16/07/2019 16:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>
>> On 16/07/2019 13:49, Chris Wilson wrote:
>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>> must avoid recursively locking the pages ourselves -- which means that
>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>> else risk calling set_page_dirty() without a lock and so risk fs
>>> corruption.
>>
>> So if trylock randomly fail we get data corruption in whatever data set
>> application is working on, which is what the original patch was trying
>> to avoid? Are we able to detect the backing store type so at least we
>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> 
> page->mapping???

Would page->mapping work? What is it telling us?

> We still have the issue that if there is a mapping we should be taking
> the lock, and we may have both a mapping and be inside try_to_unmap().

Is this a problem? On a path with mappings we trylock and so solve the 
set_dirty_locked and recursive deadlock issues, and with no mappings 
with always dirty the page and avoid data corruption.

> I think it's lose-lose. The only way to win is not to userptr :)

It's looking more and more like this indeed.

Regards,

Tvrtko