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.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 2E72EC47404 for ; Wed, 9 Oct 2019 08:57:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 02DA3206B6 for ; Wed, 9 Oct 2019 08:57:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=mbobrowski-org.20150623.gappssmtp.com header.i=@mbobrowski-org.20150623.gappssmtp.com header.b="iV/y0soj" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729767AbfJII5c (ORCPT ); Wed, 9 Oct 2019 04:57:32 -0400 Received: from mail-pf1-f194.google.com ([209.85.210.194]:42254 "EHLO mail-pf1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726765AbfJII5b (ORCPT ); Wed, 9 Oct 2019 04:57:31 -0400 Received: by mail-pf1-f194.google.com with SMTP id q12so1181573pff.9 for ; Wed, 09 Oct 2019 01:57:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mbobrowski-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9EKULkn0mMwEe4Xx6ZVvq+TugGXBB8oK4uiG0s1p/NI=; b=iV/y0sojSBbChVlBZiLZch0TQLGFeLZ1nu3NDlbuIbs5aYWTMkfWFUqzBp8Tma+obr Is6rLOQD3jnGUv29097G5YiGuvel4JJOR4wOcn9rDFOngO02hrwdHiJgJTVuXqt6flI6 Fklg3F0ZXFnukjOYuXk+m1fjLnmarbRzM5RQOChZjcRwQMBz+uccs3lFpKm5kcrOTCNe jydDAWVhEyhRv52UOoBFkrlggyukF6ngDvK+HHwdLyj1WbfQAl4B2h3QpWWsDrWXB5c0 oMZVCwbdg2p3NPuwWNYJuk5A4EjwXDyLSFdayh52lfvZn7fNT252hsoC4wUaOBy9THMc gsQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9EKULkn0mMwEe4Xx6ZVvq+TugGXBB8oK4uiG0s1p/NI=; b=XZq2D54JkxCjj2NmxbNGpObzyzn24KrQ1vjjiDyTSfjdIKT9Grvblp+rKM4/IDvKgX 1bYnKRG9YSyKFwdjyMIc7ukB/6gMSAoDZeFABPRsvENM85FakDApYKGVdLS5apaRJvsO 7gUj84+HMZkFxAnBnxFOuKIHK98EsLJZdRJYbdYL3kHPiaLq1CLkdr4R3/jx7rS4FlVv AkpTJbIlfIG+AF07W9RyW2xRDslYTSnlcmUQnCpamh9ftYXLP1Uqc0o/YVh2qvxUOFvy uHOhL4x76syYuPuHlqSpGh8JIZ/hp1PslOZ25gyOBlXa7JCIfXOMrTulepEy66v5LB/H Kodg== X-Gm-Message-State: APjAAAXhNDlN6KX9tU7oaRR3G2sb0T2EegcgQ7qRjvauDsMvu5HYOtUy 9P3vVRqZeGtD32lPnkl7SSm2 X-Google-Smtp-Source: APXvYqwFaNgdeN87wdDlj053TR5ornG+FmdCq+/xm2yIr3VIXxSxcR+6uMSMh5GilJECsdaaw5Ao1A== X-Received: by 2002:a17:90a:8002:: with SMTP id b2mr431206pjn.39.1570611450353; Wed, 09 Oct 2019 01:57:30 -0700 (PDT) Received: from poseidon.bobrowski.net ([114.78.226.167]) by smtp.gmail.com with ESMTPSA id g19sm1714151pgm.63.2019.10.09.01.57.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Oct 2019 01:57:29 -0700 (PDT) Date: Wed, 9 Oct 2019 19:57:23 +1100 From: Matthew Bobrowski To: Jan Kara Cc: tytso@mit.edu, adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, hch@infradead.org, david@fromorbit.com, darrick.wong@oracle.com Subject: Re: [PATCH v4 1/8] ext4: move out iomap field population into separate helper Message-ID: <20191009085721.GA1534@poseidon.bobrowski.net> References: <8b4499e47bea3841194850e1b3eeb924d87e69a5.1570100361.git.mbobrowski@mbobrowski.org> <20191008102709.GD5078@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191008102709.GD5078@quack2.suse.cz> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Tue, Oct 08, 2019 at 12:27:09PM +0200, Jan Kara wrote: > On Thu 03-10-19 21:33:09, Matthew Bobrowski wrote: > > +static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, > > + unsigned long first_block, struct ext4_map_blocks *map) > > +{ > > + u8 blkbits = inode->i_blkbits; > > + > > + iomap->flags = 0; > > + if (ext4_inode_datasync_dirty(inode)) > > + iomap->flags |= IOMAP_F_DIRTY; > > + iomap->bdev = inode->i_sb->s_bdev; > > + iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev; > > + iomap->offset = (u64) first_block << blkbits; > > + iomap->length = (u64) map->m_len << blkbits; > > + > > + if (type) { > > + iomap->type = type; > > + iomap->addr = IOMAP_NULL_ADDR; > > + } else { > > + if (map->m_flags & EXT4_MAP_MAPPED) { > > + iomap->type = IOMAP_MAPPED; > > + } else if (map->m_flags & EXT4_MAP_UNWRITTEN) { > > + iomap->type = IOMAP_UNWRITTEN; > > + } else { > > + WARN_ON_ONCE(1); > > + return -EIO; > > + } > > + iomap->addr = (u64) map->m_pblk << blkbits; > > + } > > Looking at this function now, the 'type' argument looks a bit weird. Can we > perhaps just remove the 'type' argument and change the above to: We can, but refer to the point below. > if (map->m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN)) { > if (map->m_flags & EXT4_MAP_MAPPED) > iomap->type = IOMAP_MAPPED; > else if (map->m_flags & EXT4_MAP_UNWRITTEN) > iomap->type = IOMAP_UNWRITTEN; > iomap->addr = (u64) map->m_pblk << blkbits; > } else { > iomap->type = IOMAP_HOLE; > iomap->addr = IOMAP_NULL_ADDR; > } > > And then in ext4_iomap_begin() we overwrite the type to: > > if (delalloc && iomap->type == IOMAP_HOLE) > iomap->type = IOMAP_DELALLOC; > > That would IMO make ext4_set_iomap() arguments harder to get wrong. I was thinking about this while doing a bunch of other things at work today. I'm kind of aligned with what Christoph mentioned around possibly duplicating some of the post 'iomap->type' setting from both current and any future ext4_set_iomap() callers. In addition to this, my thought was that if we're populating the iomap structure with values respectively, then it would make most sense to encapsulate those routines, if possible, within the ext4_set_iomap() as that's the sole purpose of the function. However, no real strong objections for dropping 'type', but I just wanted to share my thoughts. Also, yes, we probably can drop 'first_block' from the list of arguments here as we can derive that from 'map' and set 'iomap->type' accordingly... ----