Re: [PATCH 2/6] mm: Only enforce stable page writes if the backingdevice requires it
From: Darrick J. Wong
Date: Thu Jan 17 2013 - 20:28:15 EST
On Tue, Jan 15, 2013 at 10:59:26AM +0000, Steven Whitehouse wrote:
> Hi,
>
> On Mon, 2013-01-14 at 21:42 -0800, Darrick J. Wong wrote:
> > Create a helper function to check if a backing device requires stable page
> > writes and, if so, performs the necessary wait. Then, make it so that all
> > points in the memory manager that handle making pages writable use the helper
> > function. This should provide stable page write support to most filesystems,
> > while eliminating unnecessary waiting for devices that don't require the
> > feature.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/buffer.c | 2 +-
> > fs/ext4/inode.c | 2 +-
> > fs/gfs2/file.c | 2 +-
> > fs/nilfs2/file.c | 2 +-
> > include/linux/pagemap.h | 1 +
> > mm/filemap.c | 3 ++-
> > mm/page-writeback.c | 20 ++++++++++++++++++++
> > 7 files changed, 27 insertions(+), 5 deletions(-)
> >
> Acked-by: Steven Whitehouse <swhiteho@xxxxxxxxxx>
>
> Are there any other places that need patching in order to have stable
> page writes working with GFS2? Do we need to unmap in certain places or
> are the hooks for that already in the core code?
>From my semi-brief readthrough of gfs2, I think it's fine, though if you want
to test, you could create a "DIF" checksumming disk via scsi-debug:
# modprobe scsi-debug dif=1 dix=199 ato=1 [more params...]
and create a gfs2 instance atop that.
One other thing to look for is writing metadata objects to buffer_heads or
pages without locking them first. If the fs is dirtying metadata concurrent
with it being written out to disk, that'll cause problems.
Thanks for testing!
--D
>
> Steve.
>
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index c017a2d..2981449 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -2359,7 +2359,7 @@ int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
> > if (unlikely(ret < 0))
> > goto out_unlock;
> > set_page_dirty(page);
> > - wait_on_page_writeback(page);
> > + wait_for_stable_page(page);
> > return 0;
> > out_unlock:
> > unlock_page(page);
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index cbfe13b..cd818d8b 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -4968,7 +4968,7 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> > 0, len, NULL,
> > ext4_bh_unmapped)) {
> > /* Wait so that we don't change page under IO */
> > - wait_on_page_writeback(page);
> > + wait_for_stable_page(page);
> > ret = VM_FAULT_LOCKED;
> > goto out;
> > }
> > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> > index 991ab2d..b9e0ca2 100644
> > --- a/fs/gfs2/file.c
> > +++ b/fs/gfs2/file.c
> > @@ -483,7 +483,7 @@ out:
> > gfs2_holder_uninit(&gh);
> > if (ret == 0) {
> > set_page_dirty(page);
> > - wait_on_page_writeback(page);
> > + wait_for_stable_page(page);
> > }
> > sb_end_pagefault(inode->i_sb);
> > return block_page_mkwrite_return(ret);
> > diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> > index 6194688..bec4af6 100644
> > --- a/fs/nilfs2/file.c
> > +++ b/fs/nilfs2/file.c
> > @@ -126,7 +126,7 @@ static int nilfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> > nilfs_transaction_commit(inode->i_sb);
> >
> > mapped:
> > - wait_on_page_writeback(page);
> > + wait_for_stable_page(page);
> > out:
> > sb_end_pagefault(inode->i_sb);
> > return block_page_mkwrite_return(ret);
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index 6da609d..0e38e13 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -414,6 +414,7 @@ static inline void wait_on_page_writeback(struct page *page)
> > }
> >
> > extern void end_page_writeback(struct page *page);
> > +void wait_for_stable_page(struct page *page);
> >
> > /*
> > * Add an arbitrary waiter to a page's wait queue
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 83efee7..5577dc8 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1728,6 +1728,7 @@ int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> > * see the dirty page and writeprotect it again.
> > */
> > set_page_dirty(page);
> > + wait_for_stable_page(page);
> > out:
> > sb_end_pagefault(inode->i_sb);
> > return ret;
> > @@ -2274,7 +2275,7 @@ repeat:
> > return NULL;
> > }
> > found:
> > - wait_on_page_writeback(page);
> > + wait_for_stable_page(page);
> > return page;
> > }
> > EXPORT_SYMBOL(grab_cache_page_write_begin);
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 0713bfb..9c5af4d 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -2289,3 +2289,23 @@ int mapping_tagged(struct address_space *mapping, int tag)
> > return radix_tree_tagged(&mapping->page_tree, tag);
> > }
> > EXPORT_SYMBOL(mapping_tagged);
> > +
> > +/**
> > + * wait_for_stable_page() - wait for writeback to finish, if necessary.
> > + * @page: The page to wait on.
> > + *
> > + * This function determines if the given page is related to a backing device
> > + * that requires page contents to be held stable during writeback. If so, then
> > + * it will wait for any pending writeback to complete.
> > + */
> > +void wait_for_stable_page(struct page *page)
> > +{
> > + struct address_space *mapping = page_mapping(page);
> > + struct backing_dev_info *bdi = mapping->backing_dev_info;
> > +
> > + if (!bdi_cap_stable_pages_required(bdi))
> > + return;
> > +
> > + wait_on_page_writeback(page);
> > +}
> > +EXPORT_SYMBOL_GPL(wait_for_stable_page);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/