From patchwork Sun Mar 5 14:40:54 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 9604659 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 64409602B4 for ; Sun, 5 Mar 2017 14:41:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5707A28068 for ; Sun, 5 Mar 2017 14:41:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4BD622843C; Sun, 5 Mar 2017 14:41:12 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.4 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EE79728068 for ; Sun, 5 Mar 2017 14:41:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752597AbdCEOk7 (ORCPT ); Sun, 5 Mar 2017 09:40:59 -0500 Received: from mail-qk0-f178.google.com ([209.85.220.178]:36674 "EHLO mail-qk0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752528AbdCEOk6 (ORCPT ); Sun, 5 Mar 2017 09:40:58 -0500 Received: by mail-qk0-f178.google.com with SMTP id 1so119757979qkl.3 for ; Sun, 05 Mar 2017 06:40:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=3UliHnxSEhhEqKeam84a5AFAEi0mJDgvn0YGK2IRhBE=; b=Cl1hIl1fQ4WKfwHewHLEs0NASZo90LKm/eSSGK99SqS+xRbTikZd78k5jNk560Zg8W 25biM37J8WWeCfu4l6AeEM8uynlhERvJrr1EMKyN5cflTR4jpESVx0B7qNc9R75RKcz9 V1CuOhVL8sU6MakosvqoWGiYOsqr7KGQT1hmlkgNyy6Q28e7pFE9LmBMA9h2RC3dYUkz IfxOTMb/TnlUe2eYK8Ku4M4Bogr3XJhe8e9WO0jAL8J6KZW2jl2/+N6+2MQ3GMYBYRRa 1gOTa1Wgmg5hemJoANpC1GE3KyU+UyfbpfeV635SqhnOgIxKKZIb725rRbnnwlGbsQ6c gryw== X-Gm-Message-State: AMke39n+f1puGDNuHOgJqcZzTNGfpXhVuTVDojds0CQhUuUT/M4As2LQkEsf1K3GQtK5S47l X-Received: by 10.55.80.135 with SMTP id e129mr10695901qkb.192.1488724856790; Sun, 05 Mar 2017 06:40:56 -0800 (PST) Received: from cpe-2606-A000-1125-405B-DECE-A663-7B68-BD3E.dyn6.twc.com (cpe-2606-A000-1125-405B-DECE-A663-7B68-BD3E.dyn6.twc.com. [2606:a000:1125:405b:dece:a663:7b68:bd3e]) by smtp.gmail.com with ESMTPSA id j4sm11557290qkc.40.2017.03.05.06.40.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 05 Mar 2017 06:40:56 -0800 (PST) Message-ID: <1488724854.2925.6.camel@redhat.com> Subject: Re: [PATCH 0/3] mm/fs: get PG_error out of the writeback reporting business From: Jeff Layton To: viro@zeniv.linux.org.uk, konishi.ryusuke@lab.ntt.co.jp Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-nilfs@vger.kernel.org, NeilBrown , Ross Zwisler Date: Sun, 05 Mar 2017 09:40:54 -0500 In-Reply-To: <20170305133535.6516-1-jlayton@redhat.com> References: <20170305133535.6516-1-jlayton@redhat.com> X-Mailer: Evolution 3.22.5 (3.22.5-1.fc25) Mime-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sun, 2017-03-05 at 08:35 -0500, Jeff Layton wrote: > I recently did some work to wire up -ENOSPC handling in ceph, and found > I could get back -EIO errors in some cases when I should have instead > gotten -ENOSPC. The problem was that the ceph writeback code would set > PG_error on a writeback error, and that error would clobber the mapping > error. > I should also note that relying on PG_error to report writeback errors is inherently unreliable as well. If someone calls sync() before your fsync gets in there, then you'll likely lose it anyway. filemap_fdatawait_keep_errors will preserve the error in the mapping, but not the individual PG_error flags, so I think we do want to ensure that the mapping error is set when there is a writeback error and not rely on PG_error bit for that. > While I fixed that problem by simply not setting that bit on errors, > that led me down a rabbit hole of looking at how PG_error is being > handled in the kernel. > > This patch series is a few fixes for things that I 100% noticed by > inspection. I don't have a great way to test these since they involve > error handling. I can certainly doctor up a kernel to inject errors > in this code and test by hand however if these look plausible up front. > > Jeff Layton (3): > nilfs2: set the mapping error when calling SetPageError on writeback > mm: don't TestClearPageError in __filemap_fdatawait_range > mm: set mapping error when launder_pages fails > > fs/nilfs2/segment.c | 1 + > mm/filemap.c | 19 ++++--------------- > mm/truncate.c | 6 +++++- > 3 files changed, 10 insertions(+), 16 deletions(-) > (cc'ing Ross...) Just when I thought that only NILFS2 needed a little work here, I see another spot... I think that we should also need to fix dax_writeback_mapping_range to set a mapping error on writeback as well. It looks like that's not happening today. Something like the patch below (obviously untested). I'll also plan to follow up with a patch to vfs.txt to outline how writeback errors should be handled by filesystems, assuming that this patchset isn't completely off base. -------------------8<----------------------- [PATCH] dax: set error in mapping when writeback fails In order to get proper error codes from fsync, we must set an error in the mapping range when writeback fails. Signed-off-by: Jeff Layton --- fs/dax.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/dax.c b/fs/dax.c index c45598b912e1..9005d90deeda 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -888,8 +888,10 @@ int dax_writeback_mapping_range(struct address_space *mapping, ret = dax_writeback_one(bdev, mapping, indices[i], pvec.pages[i]); - if (ret < 0) + if (ret < 0) { + mapping_set_error(mapping, ret); return ret; + } } } return 0;