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 X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86E1CC43444 for ; Wed, 16 Jan 2019 12:07:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 62162206C2 for ; Wed, 16 Jan 2019 12:07:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389916AbfAPMH5 (ORCPT ); Wed, 16 Jan 2019 07:07:57 -0500 Received: from mx2.suse.de ([195.135.220.15]:60626 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1731741AbfAPMH5 (ORCPT ); Wed, 16 Jan 2019 07:07:57 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id D3648AE25; Wed, 16 Jan 2019 12:07:54 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 300A71E1580; Wed, 16 Jan 2019 13:07:54 +0100 (CET) Date: Wed, 16 Jan 2019 13:07:54 +0100 From: Jan Kara To: Linus Torvalds Cc: Matthew Wilcox , Jan Kara , linux-fsdevel , linux-ext4@vger.kernel.org, Al Viro , Phillip Potter , Amir Goldstein Subject: Re: [GIT PULL] dtype handling cleanup for v4.21-rc1 Message-ID: <20190116120754.GF26069@quack2.suse.cz> References: <20190102174736.GB29127@quack2.suse.cz> <20190108100500.GA15801@quack2.suse.cz> <20190115092414.GA4138@quack2.suse.cz> <20190115180156.GB6310@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org [Added Phillip and Amir to CC (authors)] On Wed 16-01-19 07:25:01, Linus Torvalds wrote: > On Wed, Jan 16, 2019 at 6:01 AM Matthew Wilcox wrote: > > > > The ext2/ext4 patches don't show much improvement. The other patches show > > more: > > > > fs/nilfs2/dir.c | 52 ++++++++++-------------------- > > include/uapi/linux/nilfs2_ondisk.h | 1 + > > 2 files changed, 18 insertions(+), 35 deletions(-) > > > > (for example). > > > > UFS ends up benefiting the most. You can see the whole diffstat here: > > > > https://lore.kernel.org/lkml/20181023201952.GA15676@pathfinder/ > > Well, even with _all_ the filesystems converted, you actually have > more lines added than removed by this "cleanup". > > Sharing code just isn't a win here. > > That said, it's not really the number of lines per se that make me > question this, I think that's really more of a symptom than the root > cause. The root cause for the newly adde lines is that this whole > approach requires that all the numbers are in sync, but then they have > different *names*. > > Honestly, my gut feel is that I should not pull this in this form. > > I have a suggestion: if people want to do this, and actually share the > transformation, then the filesystems that use this common code should > simply *NOT* have their own private names for the enumerations. They > should actually use those standard names. > > So if the patch for ext2 (for example) were to entirely get rid of the > whole EXT2_FT_DIR define entirely, and ext2 would just use the actual > FT_DIR define, than I'd be ok with it. At that point you don't add a > pointless and expensive abstraction. At that point you say "ext2 uses > the standard values, so ext2 can just use the standard #defines > directly". OK, I'm fine with that. We just have to have a big fat warning at FT_ definitions that these are on-disk values for several filesystems and thus cannot ever change. As Amir mentioned in another email, the original motivation for this is that quite a few filesystems copy-pasted ext2 code and that is slightly buggy. So I still do think there's value in this cleanup excercise. > See my argument? > > I think it is completely disgsting to have stuff like this: > > + BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN); > + BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE); > + BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR); > + BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV); > + BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV); > + BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO); > + BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK); > + BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK); > + BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX); > > the above is just *garbage*. > > If you fundamentally need the values to be the same, then you simply > shouldn't have two different set of #defines. > > Get rid of the EXT2_FT_xyz enumeration entirely, and the whole > craziness goes away. > > > We'd see a lot more improvement in line count if Philip weren't quite > > so paranoid about checking FOOFS_FT_* == FT_* at build time; eg for btrfs: > > Exact same issue. > > So the more I look at this, the less I like it. > > But if people are actually willing to use *truly* shared code, instead > of using their own values and then having the crazy "they need to > match", then it would be a different issue. As it is, I think the > patch series adds complexity rather than helping anything. > > More complexity and more lines of code? There is absolutely zero upside. OK, understood. Phillip, could you please rework the patches as Linus suggests? Thanks! Honza -- Jan Kara SUSE Labs, CR