On Mon, Jun 11, 2018 at 8:41 AM, Jan Kara <[email protected]> wrote: > On Fri 08-06-18 16:51:14, Dan Williams wrote: >> In preparation for implementing support for memory poison (media error) >> handling via dax mappings, implement a lock_page() equivalent. Poison >> error handling requires rmap and needs guarantees that the page->mapping >> association is maintained / valid (inode not freed) for the duration of >> the lookup. >> >> In the device-dax case it is sufficient to simply hold a dev_pagemap >> reference. In the filesystem-dax case we need to use the entry lock. >> >> Export the entry lock via dax_lock_page() that uses rcu_read_lock() to >> protect against the inode being freed, and revalidates the page->mapping >> association under xa_lock(). >> >> Signed-off-by: Dan Williams <[email protected]> > > Some comments below... > >> diff --git a/fs/dax.c b/fs/dax.c >> index cccf6cad1a7a..b7e71b108fcf 100644 >> --- a/fs/dax.c >> +++ b/fs/dax.c >> @@ -361,6 +361,82 @@ static void dax_disassociate_entry(void *entry, struct >> address_space *mapping, >> } >> } >> >> +struct page *dax_lock_page(unsigned long pfn) >> +{ > > Why do you return struct page here? Any reason behind that?
Unlike lock_page() there is no guarantee that we can lock a mapping entry given a pfn. There is a chance that we lose a race and can't validate the pfn to take the lock. So returning 'struct page *' was there to indicate that we successfully validated the pfn and were able to take the lock. I'll rework it to just return bool. > Because struct > page exists and can be accessed through pfn_to_page() regardless of result > of this function so it looks a bit confusing. Also dax_lock_page() name > seems a bit confusing. Maybe dax_lock_pfn_mapping_entry()? Ok. _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
