From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ipmail01.adl6.internode.on.net ([150.101.137.136]:41446 "EHLO ipmail01.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751217AbeEKX3N (ORCPT ); Fri, 11 May 2018 19:29:13 -0400 Date: Sat, 12 May 2018 09:29:10 +1000 From: Dave Chinner Subject: Re: [PATCH] xfs: limit xfs_growfs size if test with --large-fs Message-ID: <20180511232910.GY23861@dastard> References: <20180427082254.25524-1-zlang@redhat.com> <20180510221859.GS10363@dastard> <20180511034150.GF1963@hp-dl360g9-06.rhts.eng.pek2.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180511034150.GF1963@hp-dl360g9-06.rhts.eng.pek2.redhat.com> Sender: fstests-owner@vger.kernel.org To: Zorro Lang Cc: fstests@vger.kernel.org, darrick.wong@oracle.org, sandeen@redhat.com List-ID: On Fri, May 11, 2018 at 11:41:50AM +0800, Zorro Lang wrote: > On Fri, May 11, 2018 at 08:18:59AM +1000, Dave Chinner wrote: > > On Fri, Apr 27, 2018 at 04:22:54PM +0800, Zorro Lang wrote: > > > When test on large SCRATCH_DEV, grow a small XFS to huge size is a > > > horrible thing (e.g grow 128m to 500T). So add a helper named > > > _scratch_xfs_growfs_limited() to do below things: > > > > > > 1) If --large-fs is used, limit growfs size. > > > 2) If a limit size parameter is specified, make sure growfs won't > > > beyond this size. > > > > > > Signed-off-by: Zorro Lang > > > > I think I originally just didn't run growfs tests like this on large > > filesystems. i.e. require_no_largefs.... > > Hmm... Sorry, am I facing different review-points from 3 different XFS maintainers? ... I'm not a maintainer, I'm just the guy who added this functionality to xfstests originally. Deciding what is to be done needs to start from an understanding of the criteria I used for skipping tests on large devices. In this case, I never intended to have multiple order magnitude growfs tests run on large scratch devices. When I added large device support, I tried to avoid tests that we already had substantial coverage for. i.e. if inreasing the space used by the test doesn't increase test coverage but only increased test runtime, then I skipped it. In this case, we already test small to large size growfs via loopback devices on small scratch devices (e.g. xfs/078), so doing it on extremely large scratch devices doesn't reallycover any new code or error conditions. Hence, based on my original criteria for deciding what tests to run on large filesystems, I would have skipped this test if it caused excessive runtime. I was testing on sparse devices on SSDs, so seek times for growfs did not impact performance, hence I probably didn't skip it... > Dave: require_no_largefs is better. > Darrick: nearly ack this patch. > Eric: > 2018-04-27 04:03 < sandeen> [15:01] [00:55:47] I think maybe use _require_no_large_scratch_dev for xfs/002 will be better. Grow a 128M XFS to large size is 'horrible' > 2018-04-27 04:03 < sandeen> just limit growfs to something smaller. > > What should I do next? Make your own decision about how best to proceed based on the feedback you've received. Or ask the fstests maintainer to decide what is best.... :P Cheers, Dave. -- Dave Chinner david@fromorbit.com