On 11/25/19 12:59 AM, Jan Kara wrote:
On Sun 24-11-19 20:20:09, John Hubbard wrote:
Convert from get_user_pages() to pin_user_pages().
As required by pin_user_pages(), release these pages via
put_user_page(). In this case, do so via put_user_pages_dirty_lock().
That has the side effect of calling set_page_dirty_lock(), instead of set_page_dirty(). This is probably more accurate.
As Christoph Hellwig put it, "set_page_dirty() is only safe if we are dealing with a file backed page where we have reference on the inode it hangs off." [1]
- Release each page in mem->hpages[] (instead of mem->hpas[]), because
that is the array that pin_longterm_pages() filled in. This is more accurate and should be a little safer from a maintenance point of view.
Except that this breaks the code. hpages is unioned with hpas...
OK.
@@ -212,10 +211,9 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem) if (!page) continue;
if (mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY)SetPageDirty(page);
put_user_pages_dirty_lock(&mem->hpages[i], 1,MM_IOMMU_TABLE_GROUP_PAGE_DIRTY);And the dirtying condition is wrong here as well. Currently it is always true.
Honza
Yes. Fixed up locally. The function now looks like this (for this patch, not for the entire series, which renames "put" to "unpin"):
static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem) { long i; struct page *page = NULL;
if (!mem->hpas) return;
for (i = 0; i < mem->entries; ++i) { if (!mem->hpas[i]) continue;
page = pfn_to_page(mem->hpas[i] >> PAGE_SHIFT); if (!page) continue;
put_user_pages_dirty_lock(&page, 1, mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY);
mem->hpas[i] = 0; } }
thanks,