Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754566AbdDLPNF (ORCPT ); Wed, 12 Apr 2017 11:13:05 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:41465 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752650AbdDLPNE (ORCPT ); Wed, 12 Apr 2017 11:13:04 -0400 Subject: Re: [PATCH v2 01/17] mm: drop "wait" parameter from write_one_page To: Matthew Wilcox , Jeff Layton Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, tytso@mit.edu, jack@suse.cz, neilb@suse.com, viro@zeniv.linux.org.uk, JFS Discussion References: <20170412120614.6111-1-jlayton@redhat.com> <20170412120614.6111-2-jlayton@redhat.com> <20170412142726.GA784@bombadil.infradead.org> From: Dave Kleikamp Message-ID: <8fb8b379-37cf-42bd-7343-5b9faf4a916d@oracle.com> Date: Wed, 12 Apr 2017 10:12:09 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 MIME-Version: 1.0 In-Reply-To: <20170412142726.GA784@bombadil.infradead.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2154 Lines: 60 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 >