On 17/07/2019 21:09, Tvrtko Ursulin wrote:
On 17/07/2019 15:06, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
On 17/07/2019 14:35, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
On 17/07/2019 14:17, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
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?
It basically tells us if there is a fs around; anything that is
the most
basic of malloc (even tmpfs/shmemfs has page->mapping).
Normal malloc so anonymous pages? Or you meant everything _apart_
from
the most basic malloc?
Aye missed the not.
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.
The problem as I see it is !page->mapping are likely an
insignificant
minority of userptr; as I think even memfd are essentially
shmemfs (or
hugetlbfs) and so have mappings.
Better then nothing, no? If easy to do..
Actually, I erring on the opposite side. Peeking at mm/ internals does
not bode confidence and feels indefensible. I'd much rather throw my
hands up and say "this is the best we can do with the API provided,
please tell us what we should have done." To which the answer is
probably to not have used gup in the first place :|
"""
/*
* set_page_dirty() is racy if the caller has no reference against
* page->mapping->host, and if the page is unlocked. This is
because another
* CPU could truncate the page off the mapping and then free the
mapping.
*
* Usually, the page _is_ locked, or the caller is a user-space
process which
* holds a reference on the inode by having an open file.
*
* In other cases, the page should be locked before running
set_page_dirty().
*/
int set_page_dirty_lock(struct page *page)
"""
Could we hold a reference to page->mapping->host while having pages
and then would be okay to call plain set_page_dirty?
We would then be hitting the warnings in ext4 for unlocked pages again.
Ah true..
Essentially the argument is whether or not that warn is valid, to
which I
think requires inner knowledge of vfs + ext4. To hold a reference on the
host would require us tracking page->mapping (reasonable since we
already hooked into mmu and so will get an invalidate + fresh gup on
any changes), plus iterating over all to acquire the extra reference if
applicable -- and I have no idea what the side-effects of that would be.
Could well be positive side-effects. Just feels like wandering even
further off the beaten path without a map. Good news hmm is just around
the corner (which will probably prohibit this use-case) :|
... can we reach out to someone more knowledgeable in mm matters to
recommend us what to do?
Regards,
Tvrtko
Just a reminder to not let this slip.
We run into userptr bugs in CI quite regularly.
Thanks,
-Lionel
