From patchwork Mon May 1 22:10:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 757328 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 3wGzBk2xRqz9sD9 for ; Tue, 2 May 2017 08:10:38 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="Qb2VsxWK"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750839AbdEAWKg (ORCPT ); Mon, 1 May 2017 18:10:36 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:34490 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750785AbdEAWKe (ORCPT ); Mon, 1 May 2017 18:10:34 -0400 Received: by mail-pf0-f196.google.com with SMTP id g23so29440768pfj.1; Mon, 01 May 2017 15:10:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=dMkPxGVuDVgqzQ2iRoi6QaMRVOVfQM259Nzn5Gmw1M4=; b=Qb2VsxWKOvpapZH1T3/p80zK/UQfNh/MykcsW0cSJHr3ux4Uau94cD2GZhSvQ6XlbQ Ain3XuByxxHXv85T2nM9Ge57U5hwiR3ao9zY1ch8jjU3Fc1w9DUo42OI4YDUaeCbk7sZ UTWf41B1/4/SKIBtU4lI2zr7XxujyXoXnSPwZQl/bDUTD1shYZKVCCiN3OBMTtri5K0+ i/331gnbafcJa+d+PYm3ah9iKyH3oiSG1Gpyo/A+60jDImRtEM9GWWFZKlFEE1smGSih zhmArGE3Zl1gL1uBgWNGDmw05O2HQCcaDy1WX1wTPN976+pOzsI+onGK1zW7b0jRPjv7 ytzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=dMkPxGVuDVgqzQ2iRoi6QaMRVOVfQM259Nzn5Gmw1M4=; b=kRPwoiKkTojHP9Mz6dtZwtYWezy2jbm5WK2KAxViGmzW7M3ovPhyIeqWFYN3998p5Y /FOA/qgDtW6sI2AO2t6zkrh34rakhvSZC8t2InThFyZBh4RhZDspI1VeyCE/MXfEuS2y tG8DHCXt4tkH5PzzMGlQrSAFuDeYtUBjOzMdvXLkfaUGGIpAclQJRzyKfiItzGxA5qIl UEcDyS0ncBJrsNiAXUX4bsEJstXe0SG/G7gXyKHCpBSiOmeXjC6f2pCEbNOPhm55w/NG TqxNXwevrkz6TN/zB6rs55PTGAZLpWC1oe6Jd1E2HFRBhFgWBklcljVV0NRbxQWGNu2T 9wEQ== X-Gm-Message-State: AN3rC/6LBl0J56F0YjRLnBZAXfPvm2Dr0oxrtC17x+4Fzn4x/rv9Gwi5 tA6qG6EgG6zD1Q== X-Received: by 10.84.192.1 with SMTP id b1mr33217708pld.9.1493676633830; Mon, 01 May 2017 15:10:33 -0700 (PDT) Received: from gmail.com ([2620:0:1008:13:e908:c318:78fc:e7e9]) by smtp.gmail.com with ESMTPSA id x185sm25490513pfx.102.2017.05.01.15.10.32 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 01 May 2017 15:10:33 -0700 (PDT) Date: Mon, 1 May 2017 15:10:31 -0700 From: Eric Biggers To: Theodore Ts'o Cc: Christoph Hellwig , linux-ext4@vger.kernel.org, Andreas Dilger , Nick Alcock , Eric Biggers , stable@vger.kernel.org Subject: Re: [PATCH] ext4: evict inline data when writing to memory map Message-ID: <20170501221031.GA69912@gmail.com> References: <20170313004708.29838-1-ebiggers3@gmail.com> <20170313233313.GA23580@infradead.org> <20170314233239.GA127087@gmail.com> <20170430041357.5nrbpocxzbx5tf7p@thunk.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170430041357.5nrbpocxzbx5tf7p@thunk.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Sun, Apr 30, 2017 at 12:13:57AM -0400, Theodore Ts'o wrote: > On Tue, Mar 14, 2017 at 04:32:39PM -0700, Eric Biggers wrote: > > I'm working on this, and I discovered there's still a bug. After the data is > > written with mwrite, if the filesystem is then mount-cycled, the contents of the > > file are the old contents rather than the new contents. > > > > I believe this is caused by a bug in ext4_convert_inline_data(). Specifically, > > the new block containing the evicted data is journalled using a buffer_head > > associated with the block device. This is wrong because it can overwrite data > > that is later written through non-journalled writeback. > > I'll apply this patch for now, since it fixes a userspace-triggerable > BUG, but you're right. ext4_convert_inline_data() is busted; it > should not be using data journalling, but rather it should check to > make sure the page is already in memory (and creating it if > necessary), and just write it out to memory. > > This is a separate bug, and we should fix it, but the first patch is > correct and should go in. > > - Ted Okay, thanks. A while ago I looked into the second bug a bit, but I never got around to a proper fix. Just in case someone else wants to look into it sooner, this is the xfstest I wrote which reproduces both of these bugs: diff --git a/tests/generic/500 b/tests/generic/500 new file mode 100755 index 00000000..8326791e --- /dev/null +++ b/tests/generic/500 @@ -0,0 +1,82 @@ +#! /bin/bash +# FS QA Test generic/500 +# +# Test writing to a file using a memory map, including a tiny file whose data +# the filesystem may store inline. On ext4 with inline_data, this reproduces +# the crash fixed by: "ext4: evict inline data when writing to memory map". +# +#----------------------------------------------------------------------- +# Copyright (c) 2017 Google, Inc. All Rights Reserved. +# +# Author: Eric Biggers +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $testfile +} + +# get standard environment, filters and checks +. ./common/rc + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here +_supported_fs generic +_require_test + +testfile=$TEST_DIR/test-$seq + +file_sizes=( 4 4096 65999) +mwrite_starts=(1 2048 65500) +mwrite_sizes=( 2 32 100) + +for i in ${!file_sizes[@]}; do + + file_size=${file_sizes[$i]} + echo -e "\n=== File size $file_size ===" + + # create a non-sparse file containing $file_size bytes + rm -f $testfile + yes | tr -d '\n' | head -c $file_size > $testfile + + # sync the file to clean its pages, then write to it with mwrite + $XFS_IO_PROG $testfile \ + -c "fsync" \ + -c "mmap -w 0 1m" \ + -c "mwrite ${mwrite_starts[$i]} ${mwrite_sizes[$i]}" + + # cycle the mount and verify the data we wrote got to disk + _test_cycle_mount + hexdump -C $testfile +done + +# success, all done +status=0 +exit diff --git a/tests/generic/500.out b/tests/generic/500.out new file mode 100644 index 00000000..6e9103fc --- /dev/null +++ b/tests/generic/500.out @@ -0,0 +1,24 @@ +QA output created by 500 + +=== File size 4 === +00000000 79 58 58 79 |yXXy| +00000004 + +=== File size 4096 === +00000000 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 |yyyyyyyyyyyyyyyy| +* +00000800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 |XXXXXXXXXXXXXXXX| +* +00000820 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 |yyyyyyyyyyyyyyyy| +* +00001000 + +=== File size 65999 === +00000000 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 |yyyyyyyyyyyyyyyy| +* +0000ffd0 79 79 79 79 79 79 79 79 79 79 79 79 58 58 58 58 |yyyyyyyyyyyyXXXX| +0000ffe0 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 |XXXXXXXXXXXXXXXX| +* +00010040 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 79 |yyyyyyyyyyyyyyyy| +* +000101cf diff --git a/tests/generic/group b/tests/generic/group index b3051752..1df3b409 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -431,3 +431,4 @@ 426 auto quick exportfs 427 auto quick aio rw 428 auto quick +500 auto quick