> A few comments..
Thanks for reviewing.
> > Index: linux-2.6.21-rc4-mm1/mm/rmap.c
> > ===================================================================
> > --- linux-2.6.21-rc4-mm1.orig/mm/rmap.c 2007-03-24 19:03:11.000000000
> > +0100
> > +++ linux-2.6.21-rc4-mm1/mm/rmap.c 2007-03-24 19:34:30.000000000 +0100
> > @@ -507,6 +507,43 @@ int page_mkclean(struct page *page)
> > EXPORT_SYMBOL_GPL(page_mkclean);
> >
> > /**
> > + * test_clear_page_modified - check and clear the dirty bit for all
> > mappings of a page
> > + * @page: the page to check
> > + */
> > +bool test_clear_page_modified(struct page *page)
> > +{
> > + struct address_space *mapping = page->mapping;
>
> page_mapping(page)? Otherwise that BUG_ON(!mapping) a few lines down
> isn't of much use.
OK, removed BUG_ON(). This is called with page locked and mapping
checked, so there's no need to use page_mapping().
> > + spin_lock(&mapping->i_mmap_lock);
> > + vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
> > + if (vma->vm_flags & VM_SHARED) {
> > + struct mm_struct *mm = vma->vm_mm;
> > + unsigned long addr = vma_address(page, vma);
> > + pte_t *pte;
> > + spinlock_t *ptl;
> > +
> > + if (addr != -EFAULT &&
> > + (pte = page_check_address(page, mm, addr, &ptl))) {
> > + if (ptep_clear_flush_dirty(vma, addr, pte))
> > + modified = true;
> > + pte_unmap_unlock(pte, ptl);
> > + }
>
> Its against coding style to do assignments in conditionals.
OK, cleaned up.
> > + if (page_test_and_clear_dirty(page))
> > + modified = true;
> > + return modified;
> > +}
>
> Why not parametrize page_mkclean() to conditionally wrprotect clean
> pages? Something like:
Well, I don't really like this, because for msync, there's really no
need to do the complex ptep operations. Using ptep_clear_flush_dirty()
can save a couple of cycles.
> > + mapping = vma->vm_file->f_mapping;
> > + modified = test_and_clear_bit(AS_CMTIME, &mapping->flags);
> > +
> > + pagevec_init(&pvec, 0);
> > + index = linear_page_index(vma, start);
> > + end_index = linear_page_index(vma, end);
> > + while (index < end_index) {
> > + int i;
> > + int nr_pages = min(end_index - index, (pgoff_t) PAGEVEC_SIZE);
> > +
> > + if (mapping_cap_account_dirty(mapping))
> > + nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
> > + PAGECACHE_TAG_DIRTY, nr_pages);
> > + else
> > + nr_pages = pagevec_lookup(&pvec, mapping, index,
> > + nr_pages);
> > + if (!nr_pages)
> > + break;
> > +
> > + for (i = 0; i < nr_pages; i++) {
> > + struct page *page = pvec.pages[i];
> > +
> > + /* Skip pages which are just being read */
> > + if (!PageUptodate(page))
> > + continue;
> > +
> > + lock_page(page);
> > + index = page->index + 1;
> > + if (page->mapping == mapping &&
> > + test_clear_page_modified(page)) {
>
> page_mkclean(page, 0)
>
> > + set_page_dirty(page);
>
> set_page_dirty_mapping() ?
That would cause the file times to be updated twice, which is wrong.
But if AS_CMTIME were tested/cleared after walking the pages, this
would work. Fixed.
Also realized, that setting AS_CMTIME from the page fault can also
cause a double update, since the PTE dirty bit is also set there.
And also realized, that doing the file update from munmap can also
cause a double update, if this is not the last mapping of the inode.
Fixed all these in the updated patch.
Thanks,
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
