From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 97525E92FE4 for ; Fri, 6 Oct 2023 05:17:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230061AbjJFFRZ (ORCPT ); Fri, 6 Oct 2023 01:17:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230017AbjJFFRY (ORCPT ); Fri, 6 Oct 2023 01:17:24 -0400 Received: from mail-pg1-x530.google.com (mail-pg1-x530.google.com [IPv6:2607:f8b0:4864:20::530]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 44F1DBF for ; Thu, 5 Oct 2023 22:17:23 -0700 (PDT) Received: by mail-pg1-x530.google.com with SMTP id 41be03b00d2f7-578b4997decso1291148a12.0 for ; Thu, 05 Oct 2023 22:17:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1696569443; x=1697174243; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=6ulEDdz8AZk6BitB20Er0SVz9TiuW+ROKRm6CoZ+yRY=; b=iHAVvd+zHY5p6RiaiPvCIh5NV+6yVAVaU5fcxHrRzL0DxFUQzUigctQB4+gLhT3SE4 gDRUltTyZXhNJlSQRYHDnPSrZe/C28WRBEvrGyumDYPQ+sEuuKgGdMaZv6xyYDyYad8o V9D+QfWGwibU11fUpHIKzyKNHtOYwj5WIbHQVJIsvCuDB68sABzw6xYFOul5LSCFbsXM iil/ctpt2bv+ag8Sf1zHLu4reyi7WUfKeq4dCEIvJ1llRbFxQvHCapsIWwXfMDV3i2ta aSJsc2KukVcZlhY4IaAe6d/UJg9SDV+Ne+1WuNeRbEz5LHMWkXp6Ryi/DoKubHbTRXl2 V2gg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696569443; x=1697174243; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=6ulEDdz8AZk6BitB20Er0SVz9TiuW+ROKRm6CoZ+yRY=; b=B+WARLdTG9Sa4QLVakc3c7Ur+0o6b/iJlAxpoBZmRIKTNOkeP2/Lfb7vH9O90cOjpU b2p2ELpBuhsOEX81S6zU6pkqbwppqMHNTYSAEBKA6kP3/7dxZKz55m5T8CJJFkh2bBCt cO4gt4MqpR8kDRgBVHHRDmo7JrZbUJmBIw6im/wOmsoChgroesoIfqDFrcwf6zThUOu6 fSyfkY8IjRjOENeQEi2BxD1/5UrC8WMlTCJROy89FbnRulbUw0AQe03YtUt8i3DmHsFz c1paWkZyukr9qrfkwskJ5/e9GfZ/5+dAPRe8uHGPL/sm4BdCuV/2KEasmCU54YIsOvCj wX+Q== X-Gm-Message-State: AOJu0Yz98QoaAGLeQ8XljxQ3P//NKJ9N9U2vEoRq0obHQZFs1Z3QJtk5 2KIU4ZMuCDHQqF57UHIVM63I1A== X-Google-Smtp-Source: AGHT+IEoX7wo9B9uTd4XbcyhCw9+r4IQS1YcDjOQsGvgNycLkbkxrW5aT/HJxIM1jiGoo02r6j2MhA== X-Received: by 2002:a05:6a21:7892:b0:12c:2dc7:74bc with SMTP id bf18-20020a056a21789200b0012c2dc774bcmr8440979pzc.46.1696569442689; Thu, 05 Oct 2023 22:17:22 -0700 (PDT) Received: from dread.disaster.area (pa49-180-20-59.pa.nsw.optusnet.com.au. [49.180.20.59]) by smtp.gmail.com with ESMTPSA id ju8-20020a170903428800b001c72d694ea5sm2752815plb.303.2023.10.05.22.17.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Oct 2023 22:17:22 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qodDD-00A5Ev-2x; Fri, 06 Oct 2023 16:17:19 +1100 Date: Fri, 6 Oct 2023 16:17:19 +1100 From: Dave Chinner To: Qu Wenruo Cc: Anand Jain , fstests@vger.kernel.org, linux-btrfs@vger.kernel.org Subject: Re: [PATCH 2/2] fstests: add configuration option for executing post mkfs commands Message-ID: References: <5485cd32-2308-c9c5-4c97-9ff6c74c64dd@oracle.com> <0a8d40fc-501e-4d85-903a-83d9b3508bf5@gmx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0a8d40fc-501e-4d85-903a-83d9b3508bf5@gmx.com> Precedence: bulk List-ID: X-Mailing-List: fstests@vger.kernel.org On Thu, Sep 28, 2023 at 05:10:25PM +0930, Qu Wenruo wrote: > On 2023/9/28 15:04, Anand Jain wrote: > > On 28/09/2023 12:26, Qu Wenruo wrote: > > > On 2023/9/28 13:53, Anand Jain wrote: > > > > This patch introduces new configuration file parameters, > > > > POST_SCRATCH_MKFS_CMD and POST_SCRATCH_POOL_MKFS_CMD. > > > > > > > > Usage example: > > > > > > > >          POST_SCRATCH_MKFS_CMD="btrfstune -m" > > > >          POST_SCRATCH_POOL_MKFS_CMD="btrfstune -m" > > > > > > Can't we add extra options for mkfs.btrfs to support metadata uuid at > > > mkfs time? > > > > > > We already support quota and all other features, I think it would be > > > much easier to implement metadata_uuid inside mkfs. > > > > > > If this feature is only for metadata_uuid, then I really prefer to do it > > > inside mkfs.btrfs. > > > > Thanks for the comments. > > > > The use of btrfstune -m is just an example; any other command, > > function, or script can be assigned to the variable POST_SCRATCH_xx. > > The last time I tried something like this, I got strong objection from > some guy in the XFS community. That "some guy" has used fstests for 20 years, not to mention was the maintainer for ~4 years and did most filesystem functionality separation work that enabled fstests to become what it is now. Maybe, just maybe, that "some guy" actually has good reasons for suggesting that the functionality is done in a certain way. Both XFS and ext4 already have optional post-mkfs functionality triggered by environment variables (XFS dates back to 2003, ext4 back to 2013) implemented in their filesystem specific mkfs functions. If the configuration requires more than one command to be run, then it can't be encoded easily in an environment variable. Indeed, we shouldn't even be encoding fixed commands in environment variables; environment variables should indicate functionality that needs to be configured, and the fstests code itself implement the commands that do that specific configuration. This allows multiple configurations to be mixed and matched independently and without needing users to encode complex commands into environment variables.... That's the architecture we currently have: filesystem specific configuration operations done at mkfs time should be done in the filesystem specific mkfs routine. > Just good luck if you can have a better chance. Bad attitude doesn't win you friends or influence people. > > Now, regarding updating mkfs.btrfs with the btrfstune -m feature, > > why not? It simplifies testing. However, can we identify a use case > > other than testing? > > For consistency, I believe all (at the ones we can enable) features > should have a mkfs equivalent at least. That's a btrfs development problem, not a fstests issue. > > > > With this configuration option, test cases using _scratch_mkfs(), > > > > scratch_pool_mkfs(), or _scratch_mkfs_sized() will run the above > > > > set value after the mkfs operation. > > > > > > > > Other mkfs functions, such as _mkfs_dev(), are not connected to the > > > > POST_SCRATCH_MKFS_CMD. > > > > > > > > Signed-off-by: Anand Jain > > > > --- > > > >   common/btrfs | 35 +++++++++++++++++++++++++++++++++++ > > > >   1 file changed, 35 insertions(+) > > > > > > > > diff --git a/common/btrfs b/common/btrfs > > > > index 798c899f6233..b0972e882c7c 100644 > > > > --- a/common/btrfs > > > > +++ b/common/btrfs > > > > @@ -690,17 +690,48 @@ _require_btrfs_scratch_logical_resolve_v2() > > > >       _scratch_unmount > > > >   } > > > > > > > > +post_scratch_mkfs_cmd() > > > > +{ > > > > +    if [[ -v POST_SCRATCH_MKFS_CMD ]]; then > > > > +        echo "$POST_SCRATCH_MKFS_CMD $SCRATCH_DEV" > > > > +        $POST_SCRATCH_MKFS_CMD $SCRATCH_DEV > > > > +        return $? > > > > +    fi Ideally this should be something like: export SCRATCH_BTRFS_UUID='' btrfs_tune_dev() { local dev="$1" if [ -n "$SCRATCH_BTRFS_UUID" ]; then btrfstune -m $SCRATCH_BTRFS_UUID $dev return $? fi return 0; } _scratch_pool_mkfs_btrfs() { ..... btrfs_tune_dev $SCRATCH_DEV_POOL ..... } _scratch_mkfs_btrfs() { ..... btrfs_tune_dev $SCRATCH_DEV ..... } See how different it becomes when you stop thinking about filesystem configuration as a generic post-mkfs command and instead think of it as just another configuration option? -Dave. -- Dave Chinner david@fromorbit.com