From patchwork Thu Nov 30 08:08:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Amir Goldstein X-Patchwork-Id: 10084333 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 B554E602B9 for ; Thu, 30 Nov 2017 08:08:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A052B29EE4 for ; Thu, 30 Nov 2017 08:08:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 93C9029EEA; Thu, 30 Nov 2017 08:08:39 +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.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_TVD_MIME_EPI autolearn=ham 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 62B0929EE4 for ; Thu, 30 Nov 2017 08:08:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751790AbdK3IIi (ORCPT ); Thu, 30 Nov 2017 03:08:38 -0500 Received: from mail-yw0-f179.google.com ([209.85.161.179]:36025 "EHLO mail-yw0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751040AbdK3IIg (ORCPT ); Thu, 30 Nov 2017 03:08:36 -0500 Received: by mail-yw0-f179.google.com with SMTP id r205so2390502ywb.3; Thu, 30 Nov 2017 00:08:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=g8JrUORtSu1yUdvCHUcKFEstuqnQp6m0L7qt6OCBY1I=; b=BQgZ8btwUN8g/U+d5ggOuBar9bf47OTgdKNzfnhGIKW5XS/MZyJVz45nMSyJ/3+QAZ XQbMoZN0wojHvcLEfPxvgfaQ4xDlCPjrfZAc8u7A3gFFWPDDHt4Kb8AmSxDGKk3VwHtn /MiohpxLRjrGn79q931xBuJd5zd/rohlWWsHb6hqznb9CNJGutzr+pJnwELamJVvby4u OgjFNOQxJd5QghdeKsDjBVXl/RwHbqdSTNoJTetUn8ik1bFNiWzaSJ9bGLPqSwuzwd0b DhcAFQ8rF40mnhSq4g2zoQl+17vHdqBf1J1+7qvjDJ3Rmz+hMnf3/9wrN297WM+sTbQL k4kw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=g8JrUORtSu1yUdvCHUcKFEstuqnQp6m0L7qt6OCBY1I=; b=GmnSAGDsU5Wgwd2hs2ftdDFLms1YBLEk63z5rCXbJx2G0cJ2wtoD4n0xkLKl+CtxFZ 25TGQMKwy4+sxm5HiGU7vxXkUZzlex2wdIIr2Trv2cTq3wL/GSIkdEuRArH3dDdynarj STtQw0K3I3Qoi3bw0bAgdRnD9I0zhvgXsPjiDriOhGhg7DLsgzNoOhXPpSP1LJTaDJMY mPvV/1aOx1GBao3t3W05oPMUGy8CPQH0L0VFFxC9mwi8SzOflJoKhlYOzGYw3JOiH7vF QAEEwhkpmozd1F/v1L1oe4WBQNgkf5g7GtkE5Ff28z0TMSW/fuTZi9jVZTd20FS8TVHO D0Cw== X-Gm-Message-State: AJaThX7+yVfLil89rSRXMaAZVdmRV4Xgk9aveELAkkSaUg0JCHgQNkI1 OimDrDjO1MOILzVUKvepqoaMsZdLo4+eWQ96rQc= X-Google-Smtp-Source: AGs4zMaePNsArvERWRnN28VxHnup8H4uBWpwdE+7hWd+te7gzVd3neqT8D8ts/CayvDP7iZmRvuRZ1to/5jqyTQhZs8= X-Received: by 10.129.166.193 with SMTP id d184mr958600ywh.241.1512029315481; Thu, 30 Nov 2017 00:08:35 -0800 (PST) MIME-Version: 1.0 Received: by 10.13.225.135 with HTTP; Thu, 30 Nov 2017 00:08:34 -0800 (PST) In-Reply-To: <20171130072228.GT2749@eguan.usersys.redhat.com> References: <1512009801-11466-1-git-send-email-cgxu@mykernel.net> <20171130072228.GT2749@eguan.usersys.redhat.com> From: Amir Goldstein Date: Thu, 30 Nov 2017 10:08:34 +0200 Message-ID: Subject: Re: [PATCH] generic/470: Add check for different sync modes To: Eryu Guan Cc: Chengguang Xu , overlayfs , fstests Sender: fstests-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: fstests@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Nov 30, 2017 at 9:22 AM, Eryu Guan wrote: > On Thu, Nov 30, 2017 at 07:30:41AM +0200, Amir Goldstein wrote: >> Adding fstests list and maintainer in CC, because this is where this >> patch in meant to go. >> >> Eryu, >> >> This test is expected to fail with overlayfs on current upstream and >> any past version >> AFAIK. >> Do you this it qualifies for a specific overlay/* regression test? or >> that generic/* test >> would be sufficient to cover the overlayfs issue? > > If it reproduces the overlay bug without any special overlay setup, a > generic test would be good, other filesystems could benefit from this > test too. > > And Chengguang, can you please send the full version of the test to > fstests@vger.kernel.org again? I can only see and comment on the trimmed > version now. Yeh, sorry about that. Attaching patch here until v2 is posted to fstests. >> >> On Thu, Nov 30, 2017 at 4:43 AM, Chengguang Xu wrote: >> > generic/470 should be skipped when delalloc is not supported. > > What happens if there's no delalloc support? If test fails due to that's > not a valid setup or wrong usage of overlay (I highly doubt it :), I > agree we should _notrun in this case. If test passes without reproducing > the bug, I'd prefer continuing run the test to get better test coverage, > just write comments to describe this case. > >> > >> >> Test looks very good. One minor nit below. >> >> >> Not sure why you choose the minor detail in the line above as the >> commit description? >> Seems like the text in the top comment of the test would have been also >> appropriate for commit description. > > Yeah, please describe the test purpose in commit log, if you're testing > a specific overlay bug, describe that bug too and refering to the patch > that fixed the bug would be good. And it's worth mentioning the test > behavior when delalloc is not supported. > Test is not_run when delalloc is not supported. I should explain. This test is inspired by a simple test script I wrote https://github.com/amir73il/overlayfs/blob/master/tests/xfs_syncfs.sh At the time when I *thought* I fixed syncfs, I couldn't figure out a way to write a generic test so I left it at that. The delalloc trick, which I recently added to the script is just a very cheap way of testing that inode pages are flushed. If fs doesn't support delalloc, then test would have to use dm-flakey or a like and compare size/md5sum on disk to expected. That is a much more heavy weight test and I can't think of how to combine dm-flakey setup with overlay setup. It is just too much of a hustle considering that the delalloc trick is so cheap and works on several major fs. >> >> > Signed-off-by: Chengguang Xu >> > --- >> > tests/generic/470 | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> > tests/generic/470.out | 2 + >> > tests/generic/group | 1 + >> > 3 files changed, 136 insertions(+) >> > create mode 100755 tests/generic/470 >> > create mode 100644 tests/generic/470.out >> > >> > diff --git a/tests/generic/470 b/tests/generic/470 >> > new file mode 100755 >> > index 0000000..a43fb91 >> > --- /dev/null >> > +++ b/tests/generic/470 >> > @@ -0,0 +1,133 @@ >> > +#! /bin/bash >> > +# FS QA Test No. 470 >> > +# >> > +# Run fsync & fdatasync & syncfs & sync to test sync result. >> > +# >> > +#----------------------------------------------------------------------- >> > +# Copyright (c) 2017 Chengguang Xu . All Rights Reserved. >> > +# >> > +# 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=0 >> > +trap "_cleanup; exit \$status" 0 1 2 3 15 >> > + >> > +_cleanup() >> > +{ >> > + cd / >> > + rm -f $tmp.* >> > +} >> > + >> > +# get standard environment, filters and checks >> > +. ./common/rc >> > +. ./common/filter >> > + >> > +# remove previous $seqres.full before test >> > +rm -f $seqres.full >> > + >> > +# real QA test starts here >> > + >> > +# Modify as appropriate. >> > +_supported_fs generic >> > +_supported_os Linux >> > +_require_test >> > +_require_xfs_io_command "fsync" >> > +_require_xfs_io_command "fdatasync" >> > +_require_xfs_io_command "syncfs" >> > +_require_xfs_io_command "sync" > > Seems not necessary except the "syncfs" case, as older kernel doesn't > support syncfs(2), but I think there's nothing wrong with having them > checked explicitly. > Actually, it is quite not nice not to run the test on old kernels. Better check for availability of syncfs and if it exists add it to SYNC_MODES to be tested. >> > +_require_command "$FILEFRAG_PROG" filefrag >> > + >> > +PREFIX=$TEST_DIR/${seq}-testfile >> > +TESTFILES=$TEST_DIR/${seq}-testfile-* > > Why not just use $PREFIX-*? > >> > +FCNT=1000 >> > + >> > +write_data() >> > +{ >> > + rm -f $TESTFILES >/dev/null 2>&1 >> > + for i in `seq 1 $FCNT`; do >> > + $XFS_IO_PROG -f -c "pwrite 1K 1K" \ >> > + ${PREFIX}-$i >/dev/null 2>&1 >> > + done >> > +} >> > + >> > +check_delalloc_support() >> > +{ >> > + write_data >> > + $FILEFRAG_PROG -e $TESTFILES | grep -w delalloc >/dev/null 2>&1 > > This looks fragile and requires all the files are not written back to > disk, it's unlikely but still possible someone flushed the files before > filefrag run. > > Anyway, I think this function can be dropped if we decide continue to > run test if delalloc is not supported. > This test does not guaranty failure on buggy fs, but it does provide very very high probability of failure on buggy fs. Anything else would be much more complicated. Anything else would also not be as quick, because it will require writing large amounts of data and waiting for sync/fsync/syncfs to flush them after every write. I suggest to merge this test as is and add commentary about the possibility of false negative. Chengguang may continue to work on another test that is more reliable and doesn't require delalloc support if he chooses to. Thanks, Amir. Delivered-To: amir73il@gmail.com Received: by 10.129.112.10 with SMTP id l10csp24825ywc; Wed, 29 Nov 2017 18:44:42 -0800 (PST) X-Google-Smtp-Source: AGs4zMbX/qSN/eyGboOJkhtoHTJt3l/Ii55gW7N0VQ3fOJaXBjqHTLXn5kwxSb+0teafYnGXQ+9r X-Received: by 10.157.25.239 with SMTP id k102mr4093048otk.342.1512009882171; Wed, 29 Nov 2017 18:44:42 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1512009882; cv=none; d=google.com; s=arc-20160816; b=LC2Z4qejlWW2COdWVpvTOfkakd2Pqw1KnwfLkYtKNFLRRUxd5/OimJ98gOjLtYTvmt 1VaOmmtnzi91R2v4GKpL4H4JgR9QqJskKnjOPYJs5Dtm0YFPXtOB+fIeXlK0vzjV+MIb sGGJFz1LHQZyi0IiUP7UoeBdg9FEsb7BV1Yxf73F9tAXSGF8uEk2/pgbCBcb6cDMu9yL 2V3mIHfloBELzVy8hp/QcpmHjgUrEMaOe11lhPEgY9chO9CorBdshsBIMkDjISzAqTYl m5Hm9UiMc3RdnzeRNG1+v1KO5drREJwUCTGao5OY1b1/ivwg+qTF5Ar5pg7sHiP11AFM 9oIg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:date:subject:cc:to:from:arc-authentication-results; bh=BlUBjaWAmCh7MM80UPI5kM9k18JyIHhfon/Q6bu5aHE=; b=T5wjisJCa0cdeBWs12b3Tvtrg2+WNopL3AMxzPhZip61d3gr1ligibvWYs9xF9vT2J twuLv5eAIWwuckasg/OJcpDLE3+GNLbz3KmTRW10YlMigmV4qff3nALNul9kE9JaFK5G q3Tf6gfRQItzl4UHBpJAbH++jL0JvBi9h+OhEIpROetZldgU9JiZUk41bOKfOWZj0cV6 sYMlvJJSuIud8tdQk/cQX0wIiN8BJL7hTknjFXAYQKsS+WXq5DGKYFtVCfPQsCfj8qjM Sg/73N18y6GYyERY5jmOBPvzg6Ux7hRrxTb9Kio8QSnr+jZlMdQj4YdzJ/w7GB5VWU0n kbVQ== ARC-Authentication-Results: i=1; mx.google.com; spf=neutral (google.com: 122.225.81.134 is neither permitted nor denied by best guess record for domain of cgxu@juanniu018037.ss.mogujie.org) smtp.mailfrom=cgxu@juanniu018037.ss.mogujie.org Return-Path: Received: from juanniu018037.ss.mogujie.org ([122.225.81.134]) by mx.google.com with ESMTPS id u144si949957oif.519.2017.11.29.18.44.38 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Nov 2017 18:44:41 -0800 (PST) Received-SPF: neutral (google.com: 122.225.81.134 is neither permitted nor denied by best guess record for domain of cgxu@juanniu018037.ss.mogujie.org) client-ip=122.225.81.134; Authentication-Results: mx.google.com; spf=neutral (google.com: 122.225.81.134 is neither permitted nor denied by best guess record for domain of cgxu@juanniu018037.ss.mogujie.org) smtp.mailfrom=cgxu@juanniu018037.ss.mogujie.org Received: from juanniu018037.ss.mogujie.org (localhost [127.0.0.1]) by juanniu018037.ss.mogujie.org (8.14.7/8.14.7) with ESMTP id vAU2hWoo011563; Thu, 30 Nov 2017 10:43:32 +0800 Received: (from cgxu@localhost) by juanniu018037.ss.mogujie.org (8.14.7/8.14.7/Submit) id vAU2hVHE011562; Thu, 30 Nov 2017 10:43:31 +0800 From: Chengguang Xu To: amir73il@gmail.com Cc: linux-unionfs@vger.kernel.org, Chengguang Xu Subject: [PATCH] generic/470: Add check for different sync modes Date: Thu, 30 Nov 2017 10:43:21 +0800 Message-Id: <1512009801-11466-1-git-send-email-cgxu@mykernel.net> X-Mailer: git-send-email 1.8.3.1 generic/470 should be skipped when delalloc is not supported. Signed-off-by: Chengguang Xu --- tests/generic/470 | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/470.out | 2 + tests/generic/group | 1 + 3 files changed, 136 insertions(+) create mode 100755 tests/generic/470 create mode 100644 tests/generic/470.out diff --git a/tests/generic/470 b/tests/generic/470 new file mode 100755 index 0000000..a43fb91 --- /dev/null +++ b/tests/generic/470 @@ -0,0 +1,133 @@ +#! /bin/bash +# FS QA Test No. 470 +# +# Run fsync & fdatasync & syncfs & sync to test sync result. +# +#----------------------------------------------------------------------- +# Copyright (c) 2017 Chengguang Xu . All Rights Reserved. +# +# 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=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_supported_os Linux +_require_test +_require_xfs_io_command "fsync" +_require_xfs_io_command "fdatasync" +_require_xfs_io_command "syncfs" +_require_xfs_io_command "sync" +_require_command "$FILEFRAG_PROG" filefrag + +PREFIX=$TEST_DIR/${seq}-testfile +TESTFILES=$TEST_DIR/${seq}-testfile-* +FCNT=1000 + +write_data() +{ + rm -f $TESTFILES >/dev/null 2>&1 + for i in `seq 1 $FCNT`; do + $XFS_IO_PROG -f -c "pwrite 1K 1K" \ + ${PREFIX}-$i >/dev/null 2>&1 + done +} + +check_delalloc_support() +{ + write_data + $FILEFRAG_PROG -e $TESTFILES | grep -w delalloc >/dev/null 2>&1 + if [ $? -ne 0 ]; then + _notrun "This test requires delayed allocation support!" + fi +} + +sync_data() +{ + case $1 in + sync) + $XFS_IO_PROG -c "sync" >/dev/null 2>&1 + ;; + syncfs) + $XFS_IO_PROG -c "syncfs" ${PREFIX}-${FCNT} >/dev/null 2>&1 + ;; + fsync) + for i in `seq 1 $FCNT`; do + $XFS_IO_PROG -c "fsync" ${PREFIX}-${i} >/dev/null 2>&1 + done + ;; + fdatasync) + for i in `seq 1 $FCNT`; do + $XFS_IO_PROG -c "fdatasync" ${PREFIX}-${i} >/dev/null 2>&1 + done + ;; + *) + ;; + esac +} + +check_data() +{ + $FILEFRAG_PROG -e $TESTFILES | grep -w delalloc 2>/dev/null + if [ $? -eq 0 ]; then + status=1 + fi +} + +test_sync() +{ + local sync_mode=$1 + + if [ $sync_mode = "sync" ]; then + echo 3 > /proc/sys/vm/drop_caches + fi + + write_data + sync_data $sync_mode + check_data +} + +check_delalloc_support + +for i in fsync fdatasync syncfs sync; do + test_sync $i +done + +rm -f $TESTFILES >/dev/null 2>&1 + +echo "Silence is golden" +exit $status diff --git a/tests/generic/470.out b/tests/generic/470.out new file mode 100644 index 0000000..79fb532 --- /dev/null +++ b/tests/generic/470.out @@ -0,0 +1,2 @@ +QA output created by 470 +Silence is golden diff --git a/tests/generic/group b/tests/generic/group index 6c3bb03..23e325c 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -472,3 +472,4 @@ 467 auto quick exportfs 468 shutdown auto quick metadata 469 auto quick +470 auto quick sync -- 1.8.3.1