| Message ID | 20170412120614.6111-2-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 12-04-17 08:05:58, Jeff Layton wrote: > The callers all set it to 1. Also, make it clear that this function will > not set any sort of AS_* error, and that the caller must do so if > necessary. No existing caller uses this on normal files, so none of them > need it. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> Looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/exofs/dir.c | 2 +- > fs/ext2/dir.c | 2 +- > fs/jfs/jfs_metapage.c | 4 ++-- > fs/minix/dir.c | 2 +- > fs/sysv/dir.c | 2 +- > fs/ufs/dir.c | 2 +- > include/linux/mm.h | 2 +- > mm/page-writeback.c | 14 +++++++------- > 8 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c > index 42f9a0a0c4ca..e163ed980c20 100644 > --- a/fs/exofs/dir.c > +++ b/fs/exofs/dir.c > @@ -72,7 +72,7 @@ static int exofs_commit_chunk(struct page *page, loff_t pos, unsigned len) > set_page_dirty(page); > > if (IS_DIRSYNC(dir)) > - err = write_one_page(page, 1); > + err = write_one_page(page); > else > unlock_page(page); > > diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c > index d9650c9508e4..e2709695b177 100644 > --- a/fs/ext2/dir.c > +++ b/fs/ext2/dir.c > @@ -100,7 +100,7 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len) > } > > if (IS_DIRSYNC(dir)) { > - err = write_one_page(page, 1); > + err = write_one_page(page); > if (!err) > err = sync_inode_metadata(dir, 1); > } else { > diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c > index 489aaa1403e5..744fa3c079e6 100644 > --- a/fs/jfs/jfs_metapage.c > +++ b/fs/jfs/jfs_metapage.c > @@ -711,7 +711,7 @@ void force_metapage(struct metapage *mp) > get_page(page); > lock_page(page); > set_page_dirty(page); > - write_one_page(page, 1); > + write_one_page(page); > clear_bit(META_forcewrite, &mp->flag); > put_page(page); > } > @@ -756,7 +756,7 @@ void release_metapage(struct metapage * mp) > set_page_dirty(page); > if (test_bit(META_sync, &mp->flag)) { > clear_bit(META_sync, &mp->flag); > - write_one_page(page, 1); > + write_one_page(page); > lock_page(page); /* write_one_page unlocks the page */ > } > } else if (mp->lsn) /* discard_metapage doesn't remove it */ > diff --git a/fs/minix/dir.c b/fs/minix/dir.c > index 7edc9b395700..baa9721f1299 100644 > --- a/fs/minix/dir.c > +++ b/fs/minix/dir.c > @@ -57,7 +57,7 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len) > mark_inode_dirty(dir); > } > if (IS_DIRSYNC(dir)) > - err = write_one_page(page, 1); > + err = write_one_page(page); > else > unlock_page(page); > return err; > diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c > index 5bdae85ceef7..f5191cb2c947 100644 > --- a/fs/sysv/dir.c > +++ b/fs/sysv/dir.c > @@ -45,7 +45,7 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len) > mark_inode_dirty(dir); > } > if (IS_DIRSYNC(dir)) > - err = write_one_page(page, 1); > + err = write_one_page(page); > else > unlock_page(page); > return err; > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c > index de01b8f2aa78..48609f1d9580 100644 > --- a/fs/ufs/dir.c > +++ b/fs/ufs/dir.c > @@ -53,7 +53,7 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len) > mark_inode_dirty(dir); > } > if (IS_DIRSYNC(dir)) > - err = write_one_page(page, 1); > + err = write_one_page(page); > else > unlock_page(page); > return err; > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 00a8fa7e366a..f25b76486645 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2187,7 +2187,7 @@ extern void filemap_map_pages(struct vm_fault *vmf, > extern int filemap_page_mkwrite(struct vm_fault *vmf); > > /* mm/page-writeback.c */ > -int write_one_page(struct page *page, int wait); > +int write_one_page(struct page *page); > void task_dirty_inc(struct task_struct *tsk); > > /* readahead.c */ > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index d8ac2a7fb9e7..de0dbf12e2c1 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -2361,15 +2361,16 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc) > } > > /** > - * write_one_page - write out a single page and optionally wait on I/O > + * write_one_page - write out a single page and wait on I/O > * @page: the page to write > - * @wait: if true, wait on writeout > * > * The page must be locked by the caller and will be unlocked upon return. > * > - * write_one_page() returns a negative error code if I/O failed. > + * write_one_page() returns a negative error code if I/O failed. Note that > + * the address_space is not marked for error. The caller must do this if > + * needed. > */ > -int write_one_page(struct page *page, int wait) > +int write_one_page(struct page *page) > { > struct address_space *mapping = page->mapping; > int ret = 0; > @@ -2380,13 +2381,12 @@ int write_one_page(struct page *page, int wait) > > BUG_ON(!PageLocked(page)); > > - if (wait) > - wait_on_page_writeback(page); > + wait_on_page_writeback(page); > > if (clear_page_dirty_for_io(page)) { > get_page(page); > ret = mapping->a_ops->writepage(page, &wbc); > - if (ret == 0 && wait) { > + if (ret == 0) { > wait_on_page_writeback(page); > if (PageError(page)) > ret = -EIO; > -- > 2.9.3 >
On Wed, Apr 12, 2017 at 08:05:58AM -0400, Jeff Layton wrote: > The callers all set it to 1. Also, make it clear that this function will > not set any sort of AS_* error, and that the caller must do so if > necessary. No existing caller uses this on normal files, so none of them > need it. So ... anyone who doesn't check the error code loses an error indication. > +++ b/fs/jfs/jfs_metapage.c > @@ -711,7 +711,7 @@ void force_metapage(struct metapage *mp) > get_page(page); > lock_page(page); > set_page_dirty(page); > - write_one_page(page, 1); > + write_one_page(page); > clear_bit(META_forcewrite, &mp->flag); > put_page(page); > } > @@ -756,7 +756,7 @@ void release_metapage(struct metapage * mp) > set_page_dirty(page); > if (test_bit(META_sync, &mp->flag)) { > clear_bit(META_sync, &mp->flag); > - write_one_page(page, 1); > + write_one_page(page); > lock_page(page); /* write_one_page unlocks the page */ > } > } else if (mp->lsn) /* discard_metapage doesn't remove it */ This looks quite bad. If my reading is right, these pages are part of the journal. I think somebody who knows JFS needs to figure out what should happen here ... > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 00a8fa7e366a..f25b76486645 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2187,7 +2187,7 @@ extern void filemap_map_pages(struct vm_fault *vmf, > extern int filemap_page_mkwrite(struct vm_fault *vmf); > > /* mm/page-writeback.c */ > -int write_one_page(struct page *page, int wait); > +int write_one_page(struct page *page); > void task_dirty_inc(struct task_struct *tsk); > > /* readahead.c */ Can we mark this as __must_check so JFS picks up a couple of warnings? Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
On Wed, 2017-04-12 at 07:27 -0700, Matthew Wilcox wrote: > On Wed, Apr 12, 2017 at 08:05:58AM -0400, Jeff Layton wrote: > > The callers all set it to 1. Also, make it clear that this function will > > not set any sort of AS_* error, and that the caller must do so if > > necessary. No existing caller uses this on normal files, so none of them > > need it. > > So ... anyone who doesn't check the error code loses an error indication. > > > +++ b/fs/jfs/jfs_metapage.c > > @@ -711,7 +711,7 @@ void force_metapage(struct metapage *mp) > > get_page(page); > > lock_page(page); > > set_page_dirty(page); > > - write_one_page(page, 1); > > + write_one_page(page); > > clear_bit(META_forcewrite, &mp->flag); > > put_page(page); > > } > > @@ -756,7 +756,7 @@ void release_metapage(struct metapage * mp) > > set_page_dirty(page); > > if (test_bit(META_sync, &mp->flag)) { > > clear_bit(META_sync, &mp->flag); > > - write_one_page(page, 1); > > + write_one_page(page); > > lock_page(page); /* write_one_page unlocks the page */ > > } > > } else if (mp->lsn) /* discard_metapage doesn't remove it */ > > This looks quite bad. If my reading is right, these pages are part of > the journal. I think somebody who knows JFS needs to figure out what > should happen here ... > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 00a8fa7e366a..f25b76486645 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -2187,7 +2187,7 @@ extern void filemap_map_pages(struct vm_fault *vmf, > > extern int filemap_page_mkwrite(struct vm_fault *vmf); > > > > /* mm/page-writeback.c */ > > -int write_one_page(struct page *page, int wait); > > +int write_one_page(struct page *page); > > void task_dirty_inc(struct task_struct *tsk); > > > > /* readahead.c */ > > Can we mark this as __must_check so JFS picks up a couple of warnings? > > Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com> Good idea -- I'll roll that in with this patch.
On 04/12/2017 09:27 AM, Matthew Wilcox wrote: > On Wed, Apr 12, 2017 at 08:05:58AM -0400, Jeff Layton wrote: >> The callers all set it to 1. Also, make it clear that this function will >> not set any sort of AS_* error, and that the caller must do so if >> necessary. No existing caller uses this on normal files, so none of them >> need it. > > So ... anyone who doesn't check the error code loses an error indication. > >> +++ b/fs/jfs/jfs_metapage.c >> @@ -711,7 +711,7 @@ void force_metapage(struct metapage *mp) >> get_page(page); >> lock_page(page); >> set_page_dirty(page); >> - write_one_page(page, 1); >> + write_one_page(page); >> clear_bit(META_forcewrite, &mp->flag); >> put_page(page); >> } >> @@ -756,7 +756,7 @@ void release_metapage(struct metapage * mp) >> set_page_dirty(page); >> if (test_bit(META_sync, &mp->flag)) { >> clear_bit(META_sync, &mp->flag); >> - write_one_page(page, 1); >> + write_one_page(page); >> lock_page(page); /* write_one_page unlocks the page */ >> } >> } else if (mp->lsn) /* discard_metapage doesn't remove it */ > > This looks quite bad. If my reading is right, these pages are part of > the journal. I think somebody who knows JFS needs to figure out what > should happen here ... Actually, these are pages containing file system metadata, so we shouldn't be silently ignoring errors. Probably the only real recovery JFS can make at this point is report the error and mark the file system dirty. > >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index 00a8fa7e366a..f25b76486645 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -2187,7 +2187,7 @@ extern void filemap_map_pages(struct vm_fault *vmf, >> extern int filemap_page_mkwrite(struct vm_fault *vmf); >> >> /* mm/page-writeback.c */ >> -int write_one_page(struct page *page, int wait); >> +int write_one_page(struct page *page); >> void task_dirty_inc(struct task_struct *tsk); >> >> /* readahead.c */ > > Can we mark this as __must_check so JFS picks up a couple of warnings? Sure. that'll make me fix it. > > Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com> >
diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c index 42f9a0a0c4ca..e163ed980c20 100644 --- a/fs/exofs/dir.c +++ b/fs/exofs/dir.c @@ -72,7 +72,7 @@ static int exofs_commit_chunk(struct page *page, loff_t pos, unsigned len) set_page_dirty(page); if (IS_DIRSYNC(dir)) - err = write_one_page(page, 1); + err = write_one_page(page); else unlock_page(page); diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c index d9650c9508e4..e2709695b177 100644 --- a/fs/ext2/dir.c +++ b/fs/ext2/dir.c @@ -100,7 +100,7 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len) } if (IS_DIRSYNC(dir)) { - err = write_one_page(page, 1); + err = write_one_page(page); if (!err) err = sync_inode_metadata(dir, 1); } else { diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c index 489aaa1403e5..744fa3c079e6 100644 --- a/fs/jfs/jfs_metapage.c +++ b/fs/jfs/jfs_metapage.c @@ -711,7 +711,7 @@ void force_metapage(struct metapage *mp) get_page(page); lock_page(page); set_page_dirty(page); - write_one_page(page, 1); + write_one_page(page); clear_bit(META_forcewrite, &mp->flag); put_page(page); } @@ -756,7 +756,7 @@ void release_metapage(struct metapage * mp) set_page_dirty(page); if (test_bit(META_sync, &mp->flag)) { clear_bit(META_sync, &mp->flag); - write_one_page(page, 1); + write_one_page(page); lock_page(page); /* write_one_page unlocks the page */ } } else if (mp->lsn) /* discard_metapage doesn't remove it */ diff --git a/fs/minix/dir.c b/fs/minix/dir.c index 7edc9b395700..baa9721f1299 100644 --- a/fs/minix/dir.c +++ b/fs/minix/dir.c @@ -57,7 +57,7 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len) mark_inode_dirty(dir); } if (IS_DIRSYNC(dir)) - err = write_one_page(page, 1); + err = write_one_page(page); else unlock_page(page); return err; diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c index 5bdae85ceef7..f5191cb2c947 100644 --- a/fs/sysv/dir.c +++ b/fs/sysv/dir.c @@ -45,7 +45,7 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len) mark_inode_dirty(dir); } if (IS_DIRSYNC(dir)) - err = write_one_page(page, 1); + err = write_one_page(page); else unlock_page(page); return err; diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c index de01b8f2aa78..48609f1d9580 100644 --- a/fs/ufs/dir.c +++ b/fs/ufs/dir.c @@ -53,7 +53,7 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len) mark_inode_dirty(dir); } if (IS_DIRSYNC(dir)) - err = write_one_page(page, 1); + err = write_one_page(page); else unlock_page(page); return err; diff --git a/include/linux/mm.h b/include/linux/mm.h index 00a8fa7e366a..f25b76486645 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2187,7 +2187,7 @@ extern void filemap_map_pages(struct vm_fault *vmf, extern int filemap_page_mkwrite(struct vm_fault *vmf); /* mm/page-writeback.c */ -int write_one_page(struct page *page, int wait); +int write_one_page(struct page *page); void task_dirty_inc(struct task_struct *tsk); /* readahead.c */ diff --git a/mm/page-writeback.c b/mm/page-writeback.c index d8ac2a7fb9e7..de0dbf12e2c1 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2361,15 +2361,16 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc) } /** - * write_one_page - write out a single page and optionally wait on I/O + * write_one_page - write out a single page and wait on I/O * @page: the page to write - * @wait: if true, wait on writeout * * The page must be locked by the caller and will be unlocked upon return. * - * write_one_page() returns a negative error code if I/O failed. + * write_one_page() returns a negative error code if I/O failed. Note that + * the address_space is not marked for error. The caller must do this if + * needed. */ -int write_one_page(struct page *page, int wait) +int write_one_page(struct page *page) { struct address_space *mapping = page->mapping; int ret = 0; @@ -2380,13 +2381,12 @@ int write_one_page(struct page *page, int wait) BUG_ON(!PageLocked(page)); - if (wait) - wait_on_page_writeback(page); + wait_on_page_writeback(page); if (clear_page_dirty_for_io(page)) { get_page(page); ret = mapping->a_ops->writepage(page, &wbc); - if (ret == 0 && wait) { + if (ret == 0) { wait_on_page_writeback(page); if (PageError(page)) ret = -EIO;