lkml.org 
[lkml]   [2009]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: open(2) says O_DIRECT works on 512 byte boundries?
    On Mon, 2 Feb 2009 23:08:56 +0100
    Andrea Arcangeli <aarcange@redhat.com> wrote:

    > Hi Greg!
    >
    > > Thanks for the pointers, I'll go read the thread and follow up there.
    >
    > If you also run into this final fix is attached below. Porting to
    > mainline is a bit hard because of gup-fast... Perhaps we can use mmu
    > notifiers to fix gup-fast... need to think more about it then I'll
    > post something.
    >
    > Please help testing the below on pre-gup-fast kernels, thanks!
    >
    I commented in FJ-Redhat Path but not forwared from unknown reason ;)
    I comment again.

    1. Why TestSetLockPage() is necessary ?
    It seems not necesary.

    2. This patch doesn't cover HugeTLB.

    3. Why "follow_page() successfully finds a page" case only ?
    not necessary to insert SetPageGUP() in following path ?

    - handle_mm_fault()
    => do_anonymos/swap/wp_page()
    or some.

    BTW, when you write a patch against upstream, please CC me or linux-mm.
    I'll have to add a hook for memory-cgroup.

    Thanks,
    -Kame


    > From: Andrea Arcangeli <aarcange@redhat.com>
    > Subject: fork-o_direct-race
    >
    > Think a thread writing constantly to the last 512bytes of a page, while another
    > thread read and writes to/from the first 512bytes of the page. We can lose
    > O_DIRECT reads (or any other get_user_pages write=1 I/O not just bio/O_DIRECT),
    > the very moment we mark any pte wrprotected because a third unrelated thread
    > forks off a child.
    >
    > This fixes it by never wprotecting anon ptes if there can be any direct I/O in
    > flight to the page, and by instantiating a readonly pte and triggering a COW in
    > the child. The only trouble here are O_DIRECT reads (writes to memory, read
    > from disk). Checking the page_count under the PT lock guarantees no
    > get_user_pages could be running under us because if somebody wants to write to
    > the page, it has to break any cow first and that requires taking the PT lock in
    > follow_page before increasing the page count. We are guaranteed mapcount is 1 if
    > fork is writeprotecting the pte so the PT lock is enough to serialize against
    > get_user_pages->get_page.
    >
    > The COW triggered inside fork will run while the parent pte is readonly to
    > provide as usual the per-page atomic copy from parent to child during fork.
    > However timings will be altered by having to copy the pages that might be under
    > O_DIRECT.
    >
    > The pagevec code calls get_page while the page is sitting in the pagevec
    > (before it becomes PageLRU) and doing so it can generate false positives, so to
    > avoid slowing down fork all the time even for pages that could never possibly
    > be under O_DIRECT write=1, the PG_gup bitflag is added, this eliminates
    > most overhead of the fix in fork.
    >
    > Patch doesn't break kABI despite introducing a new page flag.
    >
    > Fixed version of original patch from Nick Piggin.
    >
    > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
    > ---
    >
    > diff -ur 2/include/linux/page-flags.h 1/include/linux/page-flags.h
    > --- 2/include/linux/page-flags.h 2008-07-10 17:26:44.000000000 +0200
    > +++ 1/include/linux/page-flags.h 2009-02-02 05:33:42.000000000 +0100
    > @@ -86,6 +86,7 @@
    > #define PG_reclaim 17 /* To be reclaimed asap */
    > #define PG_nosave_free 18 /* Free, should not be written */
    > #define PG_buddy 19 /* Page is free, on buddy lists */
    > +#define PG_gup 20 /* Page pin may be because of gup */
    >
    > /* PG_owner_priv_1 users should have descriptive aliases */
    > #define PG_checked PG_owner_priv_1 /* Used by some filesystems */
    > @@ -238,6 +239,10 @@
    > #define __SetPageCompound(page) __set_bit(PG_compound, &(page)->flags)
    > #define __ClearPageCompound(page) __clear_bit(PG_compound, &(page)->flags)
    >
    > +#define SetPageGUP(page) set_bit(PG_gup, &(page)->flags)
    > +#define PageGUP(page) test_bit(PG_gup, &(page)->flags)
    > +#define __ClearPageGUP(page) __clear_bit(PG_gup, &(page)->flags)
    > +
    > /*
    > * PG_reclaim is used in combination with PG_compound to mark the
    > * head and tail of a compound page
    > diff -ur 2/kernel/fork.c 1/kernel/fork.c
    > --- 2/kernel/fork.c 2008-07-10 17:26:43.000000000 +0200
    > +++ 1/kernel/fork.c 2009-02-02 05:24:17.000000000 +0100
    > @@ -368,7 +368,7 @@
    > rb_parent = &tmp->vm_rb;
    >
    > mm->map_count++;
    > - retval = copy_page_range(mm, oldmm, mpnt);
    > + retval = copy_page_range(mm, oldmm, tmp);
    >
    > if (tmp->vm_ops && tmp->vm_ops->open)
    > tmp->vm_ops->open(tmp);
    > Only in 1: ldtest7557.out
    > diff -ur 2/mm/memory.c 1/mm/memory.c
    > --- 2/mm/memory.c 2008-07-10 17:26:44.000000000 +0200
    > +++ 1/mm/memory.c 2009-02-02 06:17:05.000000000 +0100
    > @@ -426,7 +426,7 @@
    > * covered by this vma.
    > */
    >
    > -static inline void
    > +static inline int
    > copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
    > pte_t *dst_pte, pte_t *src_pte, struct vm_area_struct *vma,
    > unsigned long addr, int *rss)
    > @@ -434,6 +434,7 @@
    > unsigned long vm_flags = vma->vm_flags;
    > pte_t pte = *src_pte;
    > struct page *page;
    > + int forcecow = 0;
    >
    > /* pte contains position in swap or file, so copy. */
    > if (unlikely(!pte_present(pte))) {
    > @@ -464,15 +465,6 @@
    > }
    >
    > /*
    > - * If it's a COW mapping, write protect it both
    > - * in the parent and the child
    > - */
    > - if (is_cow_mapping(vm_flags)) {
    > - ptep_set_wrprotect(src_mm, addr, src_pte);
    > - pte = *src_pte;
    > - }
    > -
    > - /*
    > * If it's a shared mapping, mark it clean in
    > * the child
    > */
    > @@ -484,13 +476,41 @@
    > if (page) {
    > get_page(page);
    > page_dup_rmap(page);
    > + if (is_cow_mapping(vm_flags) && PageAnon(page) &&
    > + PageGUP(page)) {
    > + if (unlikely(TestSetPageLocked(page)))
    > + forcecow = 1;
    > + else {
    > + if (unlikely(page_count(page) !=
    > + page_mapcount(page)
    > + + !!PageSwapCache(page)))
    > + forcecow = 1;
    > + unlock_page(page);
    > + }
    > + }
    > rss[!!PageAnon(page)]++;
    > }
    >
    > + /*
    > + * If it's a COW mapping, write protect it both
    > + * in the parent and the child.
    > + */
    > + if (is_cow_mapping(vm_flags)) {
    > + ptep_set_wrprotect(src_mm, addr, src_pte);
    > + pte = pte_wrprotect(pte);
    > + }
    > +
    > out_set_pte:
    > set_pte_at(dst_mm, addr, dst_pte, pte);
    > +
    > + return forcecow;
    > }
    >
    > +static void fork_pre_cow(struct mm_struct *mm, struct vm_area_struct *vma,
    > + unsigned long address,
    > + pte_t *src_pte, pte_t *dst_pte,
    > + struct page *pre_cow_page);
    > +
    > static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
    > pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
    > unsigned long addr, unsigned long end)
    > @@ -499,17 +519,30 @@
    > spinlock_t *src_ptl, *dst_ptl;
    > int progress = 0;
    > int rss[2];
    > + int forcecow;
    > + struct page *pre_cow_page = NULL;
    >
    > again:
    > + if (!pre_cow_page) {
    > + pre_cow_page = alloc_page_vma(GFP_HIGHUSER, vma, addr);
    > + if (!pre_cow_page)
    > + return -ENOMEM;
    > + }
    > + forcecow = 0;
    > rss[1] = rss[0] = 0;
    > dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
    > - if (!dst_pte)
    > + if (!dst_pte) {
    > + page_cache_release(pre_cow_page);
    > return -ENOMEM;
    > + }
    > src_pte = pte_offset_map_nested(src_pmd, addr);
    > src_ptl = pte_lockptr(src_mm, src_pmd);
    > spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
    >
    > do {
    > + if (forcecow)
    > + break;
    > +
    > /*
    > * We are holding two locks at this point - either of them
    > * could generate latencies in another task on another CPU.
    > @@ -525,10 +558,26 @@
    > progress++;
    > continue;
    > }
    > - copy_one_pte(dst_mm, src_mm, dst_pte, src_pte, vma, addr, rss);
    > + forcecow = copy_one_pte(dst_mm, src_mm, dst_pte, src_pte, vma, addr, rss);
    > progress += 8;
    > } while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
    >
    > + if (unlikely(forcecow)) {
    > + /*
    > + * Try to COW the child page as direct I/O is working
    > + * on the parent page, and so we've to mark the parent
    > + * pte read-write before dropping the PT lock to avoid
    > + * the page to be cowed in the parent and any direct
    > + * I/O to get lost.
    > + */
    > + fork_pre_cow(dst_mm, vma, addr-PAGE_SIZE,
    > + src_pte-1, dst_pte-1, pre_cow_page);
    > + /* after the page copy set the parent pte writeable */
    > + set_pte_at(src_mm, addr-PAGE_SIZE, src_pte-1,
    > + pte_mkwrite(*(src_pte-1)));
    > + pre_cow_page = NULL;
    > + }
    > +
    > spin_unlock(src_ptl);
    > pte_unmap_nested(src_pte - 1);
    > add_mm_rss(dst_mm, rss[0], rss[1]);
    > @@ -536,6 +585,8 @@
    > cond_resched();
    > if (addr != end)
    > goto again;
    > + if (pre_cow_page)
    > + page_cache_release(pre_cow_page);
    > return 0;
    > }
    >
    > @@ -956,8 +1007,11 @@
    > if (unlikely(!page))
    > goto unlock;
    >
    > - if (flags & FOLL_GET)
    > + if (flags & FOLL_GET) {
    > get_page(page);
    > + if ((flags & FOLL_WRITE) && PageAnon(page) && !PageGUP(page))
    > + SetPageGUP(page);
    > + }
    > if (flags & FOLL_TOUCH) {
    > if ((flags & FOLL_WRITE) &&
    > !pte_dirty(pte) && !PageDirty(page))
    > @@ -1607,6 +1661,30 @@
    > copy_user_highpage(dst, src, va);
    > }
    >
    > +static void fork_pre_cow(struct mm_struct *mm, struct vm_area_struct *vma,
    > + unsigned long address,
    > + pte_t *src_pte, pte_t *dst_pte,
    > + struct page *pre_cow_page)
    > +{
    > + pte_t entry;
    > + struct page *old_page;
    > +
    > + old_page = vm_normal_page(vma, address, *src_pte);
    > + BUG_ON(!old_page);
    > + cow_user_page(pre_cow_page, old_page, address);
    > + page_remove_rmap(old_page);
    > + page_cache_release(old_page);
    > +
    > + flush_cache_page(vma, address, pte_pfn(*src_pte));
    > + entry = mk_pte(pre_cow_page, vma->vm_page_prot);
    > + entry = maybe_mkwrite(pte_mkdirty(entry), vma);
    > + page_add_new_anon_rmap(pre_cow_page, vma, address);
    > + lru_cache_add_active(pre_cow_page);
    > + set_pte_at(mm, address, dst_pte, entry);
    > + update_mmu_cache(vma, address, entry);
    > + lazy_mmu_prot_update(entry);
    > +}
    > +
    > /*
    > * This routine handles present pages, when users try to write
    > * to a shared page. It is done by copying the page to a new address
    > diff -ur 2/mm/page_alloc.c 1/mm/page_alloc.c
    > --- 2/mm/page_alloc.c 2008-07-10 17:26:44.000000000 +0200
    > +++ 1/mm/page_alloc.c 2009-02-02 05:33:18.000000000 +0100
    > @@ -154,6 +154,7 @@
    > 1 << PG_slab |
    > 1 << PG_swapcache |
    > 1 << PG_writeback |
    > + 1 << PG_gup |
    > 1 << PG_buddy );
    > set_page_count(page, 0);
    > reset_page_mapcount(page);
    > @@ -400,6 +401,8 @@
    > bad_page(page);
    > if (PageDirty(page))
    > __ClearPageDirty(page);
    > + if (PageGUP(page))
    > + __ClearPageGUP(page);
    > /*
    > * For now, we report if PG_reserved was found set, but do not
    > * clear it, and do not free the page. But we shall soon need
    > @@ -546,6 +549,7 @@
    > 1 << PG_swapcache |
    > 1 << PG_writeback |
    > 1 << PG_reserved |
    > + 1 << PG_gup |
    > 1 << PG_buddy ))))
    > bad_page(page);
    >
    >



    \
     
     \ /
      Last update: 2009-02-03 02:33    [W:0.057 / U:1.304 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site