linux-ext4 - Re: [PATCH v4 1/8] ext4: move out iomap field population into separate helper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191009085721.GA1534@poseidon.bobrowski.net>
Date:   Wed, 9 Oct 2019 19:57:23 +1100
From:   Matthew Bobrowski <mbobrowski@...browski.org>
To:     Jan Kara <jack@...e.cz>
Cc:     tytso@....edu, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        hch@...radead.org, david@...morbit.com, darrick.wong@...cle.com
Subject: Re: [PATCH v4 1/8] ext4: move out iomap field population into
 separate helper

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