From patchwork Mon Oct 5 21:55:56 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 526555 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id D22771402A8 for ; Tue, 6 Oct 2015 08:57:27 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=DLPOKCpx; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=linux-foundation.org header.i=@linux-foundation.org header.b=dqxfuAdE; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751255AbbJEVz6 (ORCPT ); Mon, 5 Oct 2015 17:55:58 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:35026 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750780AbbJEVz5 (ORCPT ); Mon, 5 Oct 2015 17:55:57 -0400 Received: by ioiz6 with SMTP id z6so201329544ioi.2; Mon, 05 Oct 2015 14:55:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=M5Atzwb8L7BcGwwa01klH/d8WsvZ0w7WdikQ+wKScbg=; b=DLPOKCpxwUOkknnVcjnB//7kabOvRSHX0Fr0mNrD7D653SQUbGPqBlfRwZZdoyDUb5 cyMsWOwQn5ps0xTz+9+7KhBMwAPPwFx6XmycpTVXYIgZcmoVE430Br93yB2xhcUa+1Iu kpU7Zeawh92MIKdaSy4Rx/xAS0w8VVZvntBTz8y1BeQsPKjf0ze6bJA0dGxF9fEfK4yS O3lGxuXGtc06sD0vCzVE1guoHimzicEPhjf+b2TcWQRefSQHBxbtdwY4GYy4lLbycQD5 weOYBdb+mEj1k5Mfz/B8/CsMM8MDxWDCJj5IBS92LCY2hDdyg56/6BIoHLZsc81IItaF BBnA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=M5Atzwb8L7BcGwwa01klH/d8WsvZ0w7WdikQ+wKScbg=; b=dqxfuAdEDVAwH35WurKP63lXly1Rl/2/lYxJYI0dZohUP9ZUYUMG+xo3FzTakIzOIp Rd+S49gCO+p9BENrom/zsVaXfIeCgCC1bPpGefqLPQj71spZoK8Ei6X5keRXVYG7qPRD I20bcgfPy4dx42VxYNktDKBpEpJCUdYSwrwNg= MIME-Version: 1.0 X-Received: by 10.107.11.166 with SMTP id 38mr31293196iol.186.1444082156281; Mon, 05 Oct 2015 14:55:56 -0700 (PDT) Received: by 10.36.124.195 with HTTP; Mon, 5 Oct 2015 14:55:56 -0700 (PDT) In-Reply-To: References: <20151005152236.GA8140@thunk.org> <5612A3F3.2040609@linux.intel.com> <5612E23B.7070606@linux.intel.com> Date: Mon, 5 Oct 2015 22:55:56 +0100 X-Google-Sender-Auth: V4QzqiGpXmEhJ45fSZg0uXC770s Message-ID: Subject: Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal From: Linus Torvalds To: Dave Hansen Cc: Peter Anvin , "Theodore Ts'o" , Andrew Morton , "linux-ext4@vger.kernel.org" , Linux Kernel Mailing List Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Mon, Oct 5, 2015 at 10:18 PM, Linus Torvalds wrote: > > Your ext4 patch may well fix the issue, and be the right thing to do > (_regardless_ of the revert, in fact - while it might make the revert > unnecessary, it might also be a good idea even if we do revert). Thinking a bit more about your patch, I actually am getting more and more convinced that it's the wrong thing to do. Why? Because the whole "Setting copied=0 will tell the upper layers to repeat the write" just seems a nasty layering violation, where the low-level filesystem uses a magic return code to introduce a special case at the upper layers. But the upper layers are actually already *aware* of the special case, and in fact have a comment about it. So I think that the whole "setting copied to 0" would actually make a lot more sense in the *caller*. Just do it in generic_perform_write() instead. Then all the special cases and the restarting is all together. What do you guys think? This basically simplifies the low-level filesystem rules, and says: - the filesystem will only ever see a partial "->write_end()" for the case where the page was up-to-date, so that there is no issue with "oops, we now have part of the page that may not have been written at all" - if the page wasn't up-to-date before, ->write_end() will either be everything we said we'd do in ->write_begin(), or it will be nothing at all. Hmm? This would seem to keep the special cases at the right layer, and actually allow low-level filesystems to simplify things (ie the special "copied = 0" special case in ext4 goes away. The ext4 side still worries me, though. You made that "page_zero_new_buffers()" conditional on "copied" being non-zero, but I'm not convinced it can be conditional. Even if we retry, that retry may end up failing (for example, because the source isn't mapped, so we return -EFAULT rather than re-doing the write), but we have those new buffers that got allocated in write_begin(), and now nobody has ever written any data to them at all, so they have random stale contents. So I do think this needs more thought. Or at least somebody should explain to me better why it's all ok. I'm attaching the "copied = 0" special case thing at the generic_perform_write() level as a patch for comments. But for now I still think that reverting would seem to be the safer thing (which still possibly leaves things buggy with a racy unmap, but at least it's the old bug that we've never hit in practice). Dave? Ted? Comments? Linus mm/filemap.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/mm/filemap.c b/mm/filemap.c index 72940fb38666..e8d01936817a 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2493,6 +2493,20 @@ again: pagefault_enable(); flush_dcache_page(page); + /* + * If we didn't successfully copy all the data from user space, + * and the target page is not up-to-date, we will have to prefault + * the source. And if the page wasn't up-to-date before the write, + * the "write_end()" may need to *make* it up-to-date, and thus + * overwrite our partial copy. + * + * So for that case, thow away the whole thing and force a full + * restart (see comment above, and iov_iter_fault_in_readable() + * below). + */ + if (copied < bytes && !PageUptodate(page)) + copied = 0; + status = a_ops->write_end(file, mapping, pos, bytes, copied, page, fsdata); if (unlikely(status < 0))