From patchwork Thu Sep 7 23:38:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Chinner X-Patchwork-Id: 9942991 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 6DD88604D5 for ; Thu, 7 Sep 2017 23:39:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 66E8D204BA for ; Thu, 7 Sep 2017 23:39:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5B50B26E96; Thu, 7 Sep 2017 23:39:20 +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.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI 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 B0197204BA for ; Thu, 7 Sep 2017 23:39:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751689AbdIGXjT (ORCPT ); Thu, 7 Sep 2017 19:39:19 -0400 Received: from ipmail07.adl2.internode.on.net ([150.101.137.131]:59455 "EHLO ipmail07.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750739AbdIGXjS (ORCPT ); Thu, 7 Sep 2017 19:39:18 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: A2DAAgAy2LFZ//yBpztcGgEBAQECAQEBAQgBAQEBhSwngyyLYI9rAQEGgSqNGIkeggSFQgQCAoRaAQIBAQEBAQJrKIUYAQEBAwEnExwjBQsIAxgJJQ8FJQMhE4okBQetVTqLOQEBAQEBAQQBAQEBAQEiIIMKgwmFUoRdRIMXgjEFiX+JEI1llESCIIllhnmJfIxoV4ENMiEIHBWHdi42h10CJAQDghQBAQE Received: from ppp59-167-129-252.static.internode.on.net (HELO dastard) ([59.167.129.252]) by ipmail07.adl2.internode.on.net with ESMTP; 08 Sep 2017 09:08:45 +0930 Received: from dave by dastard with local (Exim 4.80) (envelope-from ) id 1dq6Nh-0007nw-05; Fri, 08 Sep 2017 09:38:45 +1000 Date: Fri, 8 Sep 2017 09:38:44 +1000 From: Dave Chinner To: Chandan Rajendra Cc: linux-xfs@vger.kernel.org Subject: Re: [PATCH 00/42] mkfs: factor the crap out of the code Message-ID: <20170907233844.GD17782@dastard> References: <20170829235052.21050-1-david@fromorbit.com> <16359260.pvgM9Ovr5r@localhost.localdomain> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <16359260.pvgM9Ovr5r@localhost.localdomain> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Sep 07, 2017 at 04:01:57PM +0530, Chandan Rajendra wrote: > On Wednesday, August 30, 2017 5:20:10 AM IST Dave Chinner wrote: > > Everyone who tries to modify mkfs quickly learns that it is a pile > > of spaghetti, the only difference in opinion is whether it is a > > steaming, cold or rotten pile. This patchset attempts to untangle > > the ball of pasta and turn it into a set of clear, obvious > > operations that lead to a filesystem being formatted correctly. > > > > The patch series is really in three parts, splitting the code up > > into roughly three modules. The first part introduces a mkfs > > parameters structure and factors the on-disk formatting code to use > > only information in that structure. The second part introduces a > > command line input structure and factors the input parsing to use > > it. This requires a bunch of temporary code to keep the rest of > > the code working. The third part is factoring the input validation > > and geometry calculation code to use the input structure and put > > the output into the mkfs parameter structure and to remove all the > > temporary support code. > > > > The result is three modules - input parsing, validation+calculations > > and formatting - with well defined data flow between them. This also > > paves the way to supporting config files to set defaults via a > > separate (new) module. The overall data flow now looks like this: > > > > Build defaults --\ > > ---> mkfs_default_params -> CLI -> mkfs_params > > config file -----/ > > > > It is not worth spending a lot of time reviewing the temporary code > > that is added - it gets removed before the end of the series. No > > attempt has been made to ensure that mkfs works 100% correctly after > > each patch is applied - the only guarantee is that it will build > > cleanly. It /should/ work if a bisect lands in the middle of the > > series, but trying to exhaustively test each patch is OK would take > > more effort than it is worth. As such, testing has only been > > performed on the whole series. > > > > The new output from mkfs to indicate where it has sourced the > > defaults from causes xfstests to have conniptions. This requires > > some updates to the mkfs output filters that are already in place > > but it is a fairly trivial update. Test xfs/191 has a couple of new > > failures, but that is because the new code now correctly parses > > things like agsize so that block and sector size based > > specifications work with default mkfs values. This will require test > > updates. > > > > Future work will be to split the xfs_mkfs.c file into a file per > > module (i.e. seperate files for CLI parsing, mkfs formating, > > validation+calculation and, finally, one for config file support), > > but otherwise the majority of the factoring work is now complete. > > > > Comments, flames, etc all welcome. > > > > Hi Dave, > > For 4k blocksized xfs filesystem on ppc64, xfs/058 fails with mkfs refactor > patchset applied. > > [root@localhost xfstests-dev]# ./check xfs/058 > FSTYP -- xfs (debug) > PLATFORM -- Linux/ppc64 localhost 4.13.0-next-20170905-00001-g9730219 > MKFS_OPTIONS -- -f -bsize=4096 /dev/loop1 > MOUNT_OPTIONS -- /dev/loop1 /mnt/btrfs-xfstest-scratch > > xfs/058 2s ... - output mismatch (see /root/repos/xfstests-dev/results//xfs/058.out.bad) > --- tests/xfs/058.out 2017-09-03 02:23:13.432063287 -0500 > +++ /root/repos/xfstests-dev/results//xfs/058.out.bad 2017-09-07 05:07:49.850183977 -0500 > @@ -13,16 +13,16 @@ > fdblocks = 9223372036854775807 > Test verb middlebit > Allowing fuzz of corrupted data with good CRC > -fdblocks = 9223372034707292159 > +fdblocks = 9223372036854775807 > Test verb lastbit > Allowing fuzz of corrupted data with good CRC > ... > (Run 'diff -u tests/xfs/058.out /root/repos/xfstests-dev/results//xfs/058.out.bad' to see the entire diff) > Ran: xfs/058 > Failures: xfs/058 > Failed 1 of 1 tests That's a bug in xfs_db, not my patchset. Fixed by commit 592c10154f99 ("xfs_db: bit fuzzing should read the right bit when flipping"). > Also, xfs/206 fails because the line > "Default configuration sourced from package build definitions" isn't getting > filtered out. Lots of tests fail that way. I have a local xfstests patch that filters these out that I haven't sent out yet. Attached below. Cheers, Dave. diff --git a/common/xfs b/common/xfs index 2c34e0887747..3969582866cf 100644 --- a/common/xfs +++ b/common/xfs @@ -81,7 +81,8 @@ _scratch_mkfs_xfs() { local mkfs_cmd="`_scratch_mkfs_xfs_opts`" local mkfs_filter="sed -e '/less than device physical sector/d' \ - -e '/switching to logical sector/d'" + -e '/switching to logical sector/d' \ + -e '/Default configuration/d'" local tmp=`mktemp -u` local mkfs_status diff --git a/tests/xfs/206 b/tests/xfs/206 index 70997e3fe83e..01782b7b93a9 100755 --- a/tests/xfs/206 +++ b/tests/xfs/206 @@ -84,7 +84,8 @@ mkfs_filter() -e "s/\(sectsz\)\(=[0-9]* *\)/\1=512 /" \ -e "s/\(sunit=\)\([0-9]* blks,\)/\10 blks,/" \ -e "s/, lazy-count=[0-9]//" \ - -e "/.*crc=/d" + -e "/.*crc=/d" \ + -e "/^Default configuration/d" } # mkfs slightly smaller than that, small log for speed.