Messages in this thread | | | Date | Fri, 13 Oct 2006 15:14:57 -0700 | From | Andrew Morton <> | Subject | Re: [patch 6/6] mm: fix pagecache write deadlocks |
| |
On Fri, 13 Oct 2006 18:44:52 +0200 (CEST) Nick Piggin <npiggin@suse.de> wrote:
> From: Andrew Morton <akpm@osdl.org> and Nick Piggin <npiggin@suse.de> > > The idea is to modify the core write() code so that it won't take a pagefault > while holding a lock on the pagecache page. There are a number of different > deadlocks possible if we try to do such a thing: > > 1. generic_buffered_write > 2. lock_page > 3. prepare_write > 4. unlock_page+vmtruncate > 5. copy_from_user > 6. mmap_sem(r) > 7. handle_mm_fault > 8. lock_page (filemap_nopage) > 9. commit_write > 1. unlock_page > > b. sys_munmap / sys_mlock / others > c. mmap_sem(w) > d. make_pages_present > e. get_user_pages > f. handle_mm_fault > g. lock_page (filemap_nopage) > > 2,8 - recursive deadlock if page is same > 2,8;2,7 - ABBA deadlock is page is different > 2,6;c,g - ABBA deadlock if page is same > > - Instead of copy_from_user(), use inc_preempt_count() and > copy_from_user_inatomic(). > > - If the copy_from_user_inatomic() hits a pagefault, it'll return a short > copy. > > - if the page was not uptodate, we cannot commit the write, because the > uncopied bit could have uninitialised data. Commit zero length copy, > which should do the right thing (ie. not set the page uptodate). > > - if the page was uptodate, commit the copied portion so we make some > progress. > > - unlock_page() > > - go back and try to fault the page in again, redo the lock_page, > prepare_write, copy_from_user_inatomic(), etc. > > - Now we have a non uptodate page, and we keep faulting on a 2nd or later > iovec, this gives a deadlock, because fault_in_pages readable only faults > in the first iovec. To fix this situation, if we fault on a !uptodate page, > make the next iteration only attempt to copy a single iovec. > > - This also showed up a number of buggy prepare_write / commit_write > implementations that were setting the page uptodate in the prepare_write > side: bad! this allows uninitialised data to be read. Fix these.
Well. It's non-buggy under the current protocol because the page remains locked throughout. This patch would make these ->prepare_write() implementations buggy.
> +#ifdef CONFIG_DEBUG_VM > + fault_in_pages_readable(buf, bytes); > +#endif
I'll need to remember to take that out later on. Or reorder the patches, I guess.
> int simple_commit_write(struct file *file, struct page *page, > - unsigned offset, unsigned to) > + unsigned from, unsigned to) > { > - struct inode *inode = page->mapping->host; > - loff_t pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to; > - > - /* > - * No need to use i_size_read() here, the i_size > - * cannot change under us because we hold the i_mutex. > - */ > - if (pos > inode->i_size) > - i_size_write(inode, pos); > - set_page_dirty(page); > + if (to > from) { > + struct inode *inode = page->mapping->host; > + loff_t pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to; > + > + if (to - from == PAGE_CACHE_SIZE) > + SetPageUptodate(page);
This SetPageUptodate() can go away?
> @@ -2317,17 +2320,6 @@ int nobh_prepare_write(struct page *page > > if (is_mapped_to_disk) > SetPageMappedToDisk(page); > - SetPageUptodate(page); > - > - /* > - * Setting the page dirty here isn't necessary for the prepare_write > - * function - commit_write will do that. But if/when this function is > - * used within the pagefault handler to ensure that all mmapped pages > - * have backing space in the filesystem, we will need to dirty the page > - * if its contents were altered. > - */ > - if (dirtied_it) > - set_page_dirty(page); > > return 0;
Local variable `dirtied_it' can go away now.
Or can it? We've modified the page's contents. If the copy_from_user gets a fault and we do a zero-length ->commit_write(), nobody ends up dirtying the page.
> @@ -2450,6 +2436,7 @@ int nobh_truncate_page(struct address_sp > memset(kaddr + offset, 0, PAGE_CACHE_SIZE - offset); > flush_dcache_page(page); > kunmap_atomic(kaddr, KM_USER0); > + SetPageUptodate(page); > set_page_dirty(page); > } > unlock_page(page);
I've already forgotten why this was added. Comment, please ;) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |