Re: [PATCH v2 01/17] mm: drop "wait" parameter from write_one_page

From: Dave Kleikamp <dave.kleikamp@oracle.com>
To: Matthew Wilcox <willy@infradead.org>, Jeff Layton <jlayton@redhat.com>
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 <jfs-discussion@lists.sourceforge.net>
Subject: Re: [PATCH v2 01/17] mm: drop "wait" parameter from write_one_page
Date: Wed, 12 Apr 2017 10:12:09 -0500	[thread overview]
Message-ID: <8fb8b379-37cf-42bd-7343-5b9faf4a916d@oracle.com> (raw)
In-Reply-To: <20170412142726.GA784@bombadil.infradead.org>

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>
> 