From: Peter Zijlstra <a.p.zijlstra@chello.nl>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org, staubach@redhat.com,
hugh@veritas.com
Subject: Re: [patch 2/8] update ctime and mtime for mmaped write
Date: Tue, 06 Mar 2007 21:32:31 +0100 [thread overview]
Message-ID: <1173213151.4718.16.camel@lappy> (raw)
In-Reply-To: <20070306180549.312408559@szeredi.hu>
On Tue, 2007-03-06 at 19:04 +0100, Miklos Szeredi wrote:
> plain text document attachment (mmap_mtime.patch)
> Index: linux/mm/rmap.c
> ===================================================================
> --- linux.orig/mm/rmap.c 2007-03-06 15:17:46.000000000 +0100
> +++ linux/mm/rmap.c 2007-03-06 15:17:46.000000000 +0100
> @@ -498,6 +498,43 @@ int page_mkclean(struct page *page)
> }
>
> /**
> + * is_page_modified - check and clear the dirty bit for all mappings of a page
> + * @page: the page to check
> + */
> +bool is_page_modified(struct page *page)
> +{
> + struct address_space *mapping = page->mapping;
> + pgoff_t pgoff = page->index << (PAGE_CACHE_SHIFT - PAGE_SHIFT);
> + struct vm_area_struct *vma;
> + struct prio_tree_iter iter;
> + bool modified = false;
> +
> + BUG_ON(!mapping);
> + BUG_ON(!page_mapped(page));
> +
> + 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);
> + }
> + }
> + }
> + spin_unlock(&mapping->i_mmap_lock);
> + if (page_test_and_clear_dirty(page))
> + modified = 1;
> + return modified;
> +}
> +
> +/**
> * page_set_anon_rmap - setup new anonymous rmap
> * @page: the page to add the mapping to
> * @vma: the vm area in which the mapping is added
I'm not liking this, its not a constant operation as the name implies.
And it style is a bit out of line with the rest of rmap.
The thing it actually does is page_mkclean(), all it doesn't do is
setting the pte read-only.
I can understand you wanting to avoid the overhead of the minor faults
resulting from using page_mkclean(), but I'm not sure its worth it.
> Index: linux/mm/msync.c
> ===================================================================
> --- linux.orig/mm/msync.c 2007-03-06 15:17:42.000000000 +0100
> +++ linux/mm/msync.c 2007-03-06 15:17:46.000000000 +0100
> @@ -12,6 +12,86 @@
> #include <linux/mman.h>
> #include <linux/file.h>
> #include <linux/syscalls.h>
> +#include <linux/pagemap.h>
> +#include <linux/rmap.h>
> +#include <linux/pagevec.h>
> +
> +/*
> + * Update ctime/mtime on msync().
> + *
> + * POSIX requires, that the times are updated between a modification
> + * of the file through a memory mapping and the next msync for a
> + * region containing the modification. The wording implies that this
> + * must be done even if the modification was through a different
> + * address space. Ugh.
> + *
> + * Non-linear vmas are to hard to handle and they are non-standard
> + * anyway, so they are ignored for now.
> + *
> + * The "file modified" info is collected from two places:
> + *
> + * - AS_CMTIME flag of the mapping
> + * - the dirty bit of the ptes
> + *
> + * For memory backed filesystems all the pages in the range need to be
> + * examined. For non-memory backed filesystems it is enough to look
> + * at the pages with the dirty tag.
> + */
> +static void msync_update_file_time(struct vm_area_struct *vma,
> + unsigned long start, unsigned long end)
> +{
> + struct file *file = vma->vm_file;
> + struct address_space *mapping = file->f_mapping;
> + struct pagevec pvec;
> + pgoff_t index;
> + pgoff_t end_index;
> + bool modified;
> +
> + if (!file || !(vma->vm_flags & VM_SHARED) ||
> + (vma->vm_flags & VM_NONLINEAR))
> + return;
> +
> + 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;
> + struct address_space *mapping = file->f_mapping;
> + 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 &&
> + is_page_modified(page)) {
> + set_page_dirty(page);
> + modified = true;
> + }
> + unlock_page(page);
> + }
> + pagevec_release(&pvec);
> + }
> +
> + if (modified)
> + file_update_time(file);
> +}
>
> /*
> * MS_SYNC syncs the entire file - including mappings.
Something bothers me, although I'm not sure what yet..
next prev parent reply other threads:[~2007-03-06 20:32 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-03-06 18:04 [patch 0/8] VFS/VM patches Miklos Szeredi
2007-03-06 18:04 ` [patch 1/8] fix race in clear_page_dirty_for_io() Miklos Szeredi
2007-03-06 22:25 ` Andrew Morton
2007-03-06 18:04 ` [patch 2/8] update ctime and mtime for mmaped write Miklos Szeredi
2007-03-06 20:32 ` Peter Zijlstra [this message]
2007-03-06 21:24 ` Miklos Szeredi
2007-03-06 21:47 ` Peter Zijlstra
2007-03-06 22:00 ` Miklos Szeredi
2007-03-06 22:07 ` Peter Zijlstra
2007-03-06 22:18 ` Miklos Szeredi
2007-03-06 22:28 ` Peter Zijlstra
2007-03-06 22:36 ` Miklos Szeredi
2007-03-06 18:04 ` [patch 3/8] per backing_dev dirty and writeback page accounting Miklos Szeredi
2007-03-12 6:23 ` David Chinner
2007-03-12 11:40 ` Miklos Szeredi
2007-03-12 21:44 ` David Chinner
2007-03-12 22:36 ` Miklos Szeredi
2007-03-12 23:12 ` David Chinner
2007-03-13 8:21 ` Miklos Szeredi
2007-03-13 22:12 ` David Chinner
2007-03-14 22:09 ` Miklos Szeredi
2007-03-06 18:04 ` [patch 4/8] fix deadlock in balance_dirty_pages Miklos Szeredi
2007-03-06 18:04 ` [patch 5/8] fix deadlock in throttle_vm_writeout Miklos Szeredi
2007-03-06 18:04 ` [patch 6/8] balance dirty pages from loop device Miklos Szeredi
2007-03-06 18:04 ` [patch 7/8] add filesystem subtype support Miklos Szeredi
2007-03-06 18:04 ` [patch 8/8] consolidate generic_writepages and mpage_writepages fix Miklos Szeredi
2007-03-07 20:46 ` Andrew Morton
2007-03-07 21:26 ` Miklos Szeredi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1173213151.4718.16.camel@lappy \
--to=a.p.zijlstra@chello.nl \
--cc=akpm@linux-foundation.org \
--cc=hugh@veritas.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=staubach@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).