On Wed, Mar 20, 2019 at 02:27:47PM +0800, Qu Wenruo wrote:
> In extent_write_cache_pages() since flush_write_bio() can return error,
> we need some extra error handling.
>
> For the first flush_write_bio() since we haven't locked the page, we
> only need to exit the loop.
>
> For the seconds flush_write_bio() call, we have the page locked, despite
> that there is nothing special need to handle.
>
> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> Reviewed-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
I'm leaving this patch out for now.
> ---
> fs/btrfs/extent_io.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 5de18900a3c3..c38058398d75 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4000,7 +4000,10 @@ static int extent_write_cache_pages(struct address_space *mapping,
> */
> if (!trylock_page(page)) {
> ret = flush_write_bio(epd);
> - BUG_ON(ret < 0);
> + if (ret < 0) {
> + done = 1;
> + break;
> + }
> lock_page(page);
> }
>
> @@ -4012,7 +4015,11 @@ static int extent_write_cache_pages(struct address_space *mapping,
> if (wbc->sync_mode != WB_SYNC_NONE) {
> if (PageWriteback(page)) {
> ret = flush_write_bio(epd);
> - BUG_ON(ret < 0);
> + if (ret < 0) {
> + unlock_page(page);
> + done = 1;
I'm not sure about the semantics of 'done' here, in the normal case it
clear, but error handling needs to be taken to context of the whole
writeback and the state.
Your patch looks correct though, so it's a matter of making sure we're
not missing something subtle.
