Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: Matthew Bobrowski <mbobrowski@mbobrowski.org>
To: Jan Kara <jack@suse.cz>
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
Date: Wed, 9 Oct 2019 19:57:23 +1100
Message-ID: <20191009085721.GA1534@poseidon.bobrowski.net> (raw)
In-Reply-To: <20191008102709.GD5078@quack2.suse.cz>

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...

--<M>--



  reply index

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-03 11:32 [PATCH v4 0/8] ext4: port direct I/O to iomap infrastructure Matthew Bobrowski
2019-10-03 11:33 ` [PATCH v4 1/8] ext4: move out iomap field population into separate helper Matthew Bobrowski
2019-10-08 10:27   ` Jan Kara
2019-10-09  8:57     ` Matthew Bobrowski [this message]
2019-10-09 13:06       ` Jan Kara
2019-10-10  5:39         ` Matthew Bobrowski
2019-10-09  6:02   ` Ritesh Harjani
2019-10-09  7:07     ` Christoph Hellwig
2019-10-09  7:50       ` Ritesh Harjani
2019-10-09  9:09         ` Matthew Bobrowski
2019-10-03 11:33 ` [PATCH v4 2/8] ext4: move out IOMAP_WRITE path " Matthew Bobrowski
2019-10-08 10:31   ` Jan Kara
2019-10-09  9:18     ` Matthew Bobrowski
2019-10-09  6:22   ` Ritesh Harjani
2019-10-09  9:31     ` Matthew Bobrowski
2019-10-03 11:33 ` [PATCH v4 3/8] ext4: introduce new callback for IOMAP_REPORT operations Matthew Bobrowski
2019-10-08 10:42   ` Jan Kara
2019-10-09  9:41     ` Matthew Bobrowski
2019-10-09  6:00   ` Ritesh Harjani
2019-10-09 12:08     ` Matthew Bobrowski
2019-10-09 13:14       ` Ritesh Harjani
2019-10-03 11:34 ` [PATCH v4 4/8] ext4: introduce direct I/O read path using iomap infrastructure Matthew Bobrowski
2019-10-08 10:52   ` Jan Kara
2019-10-09 10:55     ` Matthew Bobrowski
2019-10-09  6:39   ` Ritesh Harjani
2019-10-03 11:34 ` [PATCH v4 5/8] ext4: move inode extension/truncate code out from ->iomap_end() callback Matthew Bobrowski
2019-10-08 11:25   ` Jan Kara
2019-10-09 10:18     ` Matthew Bobrowski
2019-10-09 12:51       ` Jan Kara
2019-10-10  5:44         ` Matthew Bobrowski
2019-10-09  6:27   ` Ritesh Harjani
2019-10-09 10:20     ` Matthew Bobrowski
2019-10-03 11:34 ` [PATCH v4 6/8] ext4: move inode extension checks out from ext4_iomap_alloc() Matthew Bobrowski
2019-10-08 11:27   ` Jan Kara
2019-10-09 10:21     ` Matthew Bobrowski
2019-10-09  6:30   ` Ritesh Harjani
2019-10-09 10:39     ` Matthew Bobrowski
2019-10-03 11:34 ` [PATCH v4 7/8] ext4: reorder map.m_flags checks in ext4_set_iomap() Matthew Bobrowski
2019-10-08 11:30   ` Jan Kara
2019-10-09 10:42     ` Matthew Bobrowski
2019-10-09  6:35   ` Ritesh Harjani
2019-10-09 10:43     ` Matthew Bobrowski
2019-10-03 11:35 ` [PATCH v4 8/8] ext4: introduce direct I/O write path using iomap infrastructure Matthew Bobrowski
2019-10-08 15:12   ` Jan Kara
2019-10-09  7:11     ` Christoph Hellwig
2019-10-09 13:41       ` Jan Kara
2019-10-09 11:53     ` Matthew Bobrowski

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191009085721.GA1534@poseidon.bobrowski.net \
    --to=mbobrowski@mbobrowski.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=hch@infradead.org \
    --cc=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git