2006-05-01 19:01:11

by Wendy Cheng

[permalink] [raw]
Subject: Re: [PATCH 04/16] GFS2: Daemons and address space operations

Steven Whitehouse wrote

>+static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
>+{
>+ struct inode *inode = page->mapping->host;
>+ struct gfs2_inode *ip = page->mapping->host->u.generic_ip;
>+ struct gfs2_sbd *sdp = ip->i_sbd;
>+ loff_t i_size = i_size_read(inode);
>+ pgoff_t end_index = i_size >> PAGE_CACHE_SHIFT;
>+ unsigned offset;
>+ int error;
>+ int done_trans = 0;
>+
>+ if (gfs2_assert_withdraw(sdp, gfs2_glock_is_held_excl(ip->i_gl))) {
>+ unlock_page(page);
>+ return -EIO;
>+ }
>+ if (current->journal_info)
>+ goto out_ignore;
>+
>+ /* Is the page fully outside i_size? (truncate in progress) */
>+ offset = i_size & (PAGE_CACHE_SIZE-1);
>+ if (page->index >= end_index+1 || !offset) {
>+ page->mapping->a_ops->invalidatepage(page, 0);
>+ unlock_page(page);
>+ return 0; /* don't care */
>+ }
>
>
>
Will above "|| !offset" unconditionally drop the page if the file size
happens to be multiples of PAGE_CACHE_SIZE ? Maybe this truncate
handling should be removed to let block_write_full_page() handle all the
cases ?

>+ error = block_write_full_page(page, get_block_noalloc, wbc);
>+ if (done_trans)
>+ gfs2_trans_end(sdp);
>+ gfs2_meta_cache_flush(ip);
>+ return error;
>+
>+out_ignore:
>+ redirty_page_for_writepage(wbc, page);
>+ unlock_page(page);
>+ return 0;
>+}
>+
>
>
-- Wendy


2006-05-02 12:39:48

by Steven Whitehouse

[permalink] [raw]
Subject: Re: [PATCH 04/16] GFS2: Daemons and address space operations

Hi,

On Mon, 2006-05-01 at 15:00 -0400, Wendy Cheng wrote:
> Steven Whitehouse wrote
>
> >+static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
> >+{
> >+ struct inode *inode = page->mapping->host;
> >+ struct gfs2_inode *ip = page->mapping->host->u.generic_ip;
> >+ struct gfs2_sbd *sdp = ip->i_sbd;
> >+ loff_t i_size = i_size_read(inode);
> >+ pgoff_t end_index = i_size >> PAGE_CACHE_SHIFT;
> >+ unsigned offset;
> >+ int error;
> >+ int done_trans = 0;
> >+
> >+ if (gfs2_assert_withdraw(sdp, gfs2_glock_is_held_excl(ip->i_gl))) {
> >+ unlock_page(page);
> >+ return -EIO;
> >+ }
> >+ if (current->journal_info)
> >+ goto out_ignore;
> >+
> >+ /* Is the page fully outside i_size? (truncate in progress) */
> >+ offset = i_size & (PAGE_CACHE_SIZE-1);
> >+ if (page->index >= end_index+1 || !offset) {
> >+ page->mapping->a_ops->invalidatepage(page, 0);
> >+ unlock_page(page);
> >+ return 0; /* don't care */
> >+ }
> >
> >
> >
> Will above "|| !offset" unconditionally drop the page if the file size
> happens to be multiples of PAGE_CACHE_SIZE ? Maybe this truncate
> handling should be removed to let block_write_full_page() handle all the
> cases ?
>
This test was incorrectly borrowed from block_write_full_page in order
that we would avoid having to start a transaction, or add buffers to
pages, in case the page had already been truncated. I've just pushed a
patch to fix this:

http://www.kernel.org/git/?p=linux/kernel/git/steve/gfs2-2.6.git;a=commitdiff;h=d2d7b8a2a756fb520792ca3db3abdeed9214ae5b

Steve.