| [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] |
Thanks, Daniel ... see questions below.
-- Ben --
Opinions are mine, not Intel's
> -----Original Message-----
> From: opengfs-devel-admin@lists.sourceforge.net
> [mailto:opengfs-devel-admin@lists.sourceforge.net]On Behalf Of Daniel
> Phillips
> Sent: Friday, October 03, 2003 8:23 PM
> To: opengfs-devel@lists.sourceforge.net
> Subject: [ogfs-dev][RFC][PATCH] Eliminate mmap race
>
>
> This patch uses the do_no_page hook to eliminate the mmap
> race described in
> the previous mail. The regular do_no_page function is
> wrapped with a glock
> that protects both reading in the page and entering it into
> the page table.
So this is wrapping the glock around a higher level call, i.e. acquiring the glock earlier and releasing it later?
>
> This is only very lightly tested. In particular, I didn't
> test SMP at all
> because I don't have an SMP client at the moment. That said,
> it's possible
> the SMP locking is correct.
I notice that you eliminated the lock_kernel()/unlock_kernel() pair. Would you be willing to look for others that could be eliminated??
> The main issue is the handling
> of the page
> table spinlock: it has to be released before the global lock
> can be acquired,
Is this simply because it might take some significant time to acquire the glock?
> and reacquired later to satisfy the entry conditions for
> do_no_page. This
> is actually useless work, because the first thing do_no_page does is
> release the lock. This can be fixed by bringing all the
> functionality of
> do_no_page (which actually isn't very much) inside
> ogfs_do_no_page and not
> writing it as a shell. But that is premature optimization.
>
> It's quite possible that another task on the same machine
> could fault in
> the same page while we are waiting to acquire the glock. I
> did not check
> explicitly for that after reacquiring the page table lock,
> because both
> filemap_nopage and do_no_page seem to be able to deal with it.
>
> Regards,
>
> Daniel
>
> --- opengfs-0.3.0.clean/src/fs/arch_linux_2_4/file.c
> 2003-07-13 07:23:46.000000000 +0200
> +++ opengfs-0.3.0/src/fs/arch_linux_2_4/file.c
> 2003-10-04 00:30:32.000000000 +0200
> @@ -41,6 +41,8 @@
> #include "plock.h"
> #include "trans.h"
>
> +#define OGFS_SHARED_WRITEABLE_MMAP
> +
> struct ogfs_filldir_struct {
> filldir_t vfs_filldir;
> void *vfs_opaque;
> @@ -1068,47 +1070,32 @@
>
> #ifdef OGFS_SHARED_WRITEABLE_MMAP
> /**
> - * ogfs_shared_nopage - support shared writable mappings
> - * @area - vm_area to pass through to filemap_nopage
> - * @address - pass through to filemap_nopage
> - * @no_share - pass through to filemap_nopage
> + * ogfs_do_no_page: wraps do_no_page in a glock, same
> interface as do_no_page.
> *
> * Currently we don't support the shared state. Thus, if
> somebody owns a page
> * in the file, they also have an exclusive lock on the whole file.
> - *
> - * Returns: the new page
> */
> -
> -static struct page *
> -ogfs_shared_nopage(struct vm_area_struct *area,
> - unsigned long address, int no_share)
> +static int ogfs_do_no_page(struct mm_struct *mm, struct
> vm_area_struct *vma,
> + unsigned long address, int write_access, pte_t *page_table)
> {
> - ogfs_inode_t *ip = vi2ino(area->vm_file->f_dentry->d_inode);
> - struct page *result = NULL;
> - int error;
> + ogfs_inode_t *inode = vi2ino(vma->vm_file->f_dentry->d_inode);
> + int result = 0;
>
> ENTER(GFN_SHARED_NOPAGE);
> -
> - lock_kernel();
> -
> - error = ogfs_glock_i(ip, 0);
> - if (error)
> - goto out;
> -
> - result = filemap_nopage(area, address, no_share);
> -
> - ogfs_gunlock_i(ip, 0);
> -
> -out:
> - unlock_kernel();
> -
> + spin_unlock(&mm->page_table_lock);
> + if (!ogfs_glock_i(inode, 0)) {
> + spin_lock(&mm->page_table_lock);
> + result = do_no_page(mm, vma, address,
> write_access, page_table);
> + ogfs_gunlock_i(inode, 0);
> + }
> EXIT(GFN_SHARED_NOPAGE);
>
> return result;
> }
>
> static struct vm_operations_struct ogfs_shared_mmap = {
> - nopage: ogfs_shared_nopage,
> + nopage: filemap_nopage,
> + do_no_page: ogfs_do_no_page,
> };
> #endif /*
> OGFS_SHARED_WRITEABLE_MMAP */
>
> @@ -1170,8 +1157,7 @@
>
> ENTER(GFN_FILE_MMAP);
>
> - if ((vma->vm_flags & (VM_MAYWRITE | VM_SHARED)) ==
> - (VM_MAYWRITE | VM_SHARED))
> + if ((vma->vm_flags & (VM_MAYWRITE | VM_SHARED)) ==
> (VM_MAYWRITE | VM_SHARED))
> #ifdef OGFS_SHARED_WRITEABLE_MMAP
> vma->vm_ops = &ogfs_shared_mmap;
> #else
>
>
>
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Opengfs-devel mailing list
> Opengfs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/opengfs-devel
>
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Opengfs-devel mailing list
Opengfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opengfs-devel