linux-ext4 - Re: [PATCH v5 01/12] ext4: move set iomap routines into separate helper ext4_set

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <20191023063152.EEE024C050@d06av22.portsmouth.uk.ibm.com>
Date:   Wed, 23 Oct 2019 12:01:51 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Matthew Bobrowski <mbobrowski@...browski.org>, tytso@....edu,
        jack@...e.cz, adilger.kernel@...ger.ca
Cc:     linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        hch@...radead.org, david@...morbit.com, darrick.wong@...cle.com
Subject: Re: [PATCH v5 01/12] ext4: move set iomap routines into separate
 helper ext4_set_iomap()



On 10/21/19 2:47 PM, Matthew Bobrowski wrote:
> Separate the iomap field population chunk of code that is currently
> within ext4_iomap_begin() into a new helper called
> ext4_set_iomap(). The intent of this function is self explanatory,
> however the rationale behind doing so is to also reduce the overall
> clutter that we currently have within the ext4_iomap_begin() callback.
> 
> Signed-off-by: Matthew Bobrowski <mbobrowski@...browski.org>

Could you please re-arrange patch sequence in this fashion.

1. Patch-11 (re-ordering of unwritten flags)
2. Patch-8 (trylock in IOCB_NOWAIT cases)
3. Patch-2 (should explain offset & len in this patch)
4. Patch-1 (this patch).

This is so that some of these are anyway fixes or refactoring
which can be picked up easily, either for backporting or
sometimes this helps in getting some of the patches in, if the patch
series gets bigger.
Also others (like me) can also pick some of these changes then to meet
their dependency. :)


This patch looks good to me. You may add:

Reviewed-by: Ritesh Harjani <riteshh@...ux.ibm.com>



> ---
>   fs/ext4/inode.c | 59 +++++++++++++++++++++++++++----------------------
>   1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 516faa280ced..158eea9a1944 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3406,10 +3406,39 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
>   	return inode->i_state & I_DIRTY_DATASYNC;
>   }
>   
> +static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
> +			   struct ext4_map_blocks *map, loff_t offset,
> +			   loff_t length)
> +{
> +	u8 blkbits = inode->i_blkbits;
> +
> +	iomap->flags = 0;
> +	if (ext4_inode_datasync_dirty(inode))
> +		iomap->flags |= IOMAP_F_DIRTY;
> +
> +	if (map->m_flags & EXT4_MAP_NEW)
> +		iomap->flags |= IOMAP_F_NEW;
> +
> +	iomap->bdev = inode->i_sb->s_bdev;
> +	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
> +	iomap->offset = (u64) map->m_lblk << blkbits;
> +	iomap->length = (u64) map->m_len << blkbits;
> +
> +	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;
> +	}
> +}
> +
>   static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   			    unsigned flags, struct iomap *iomap)
>   {
> -	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>   	unsigned int blkbits = inode->i_blkbits;
>   	unsigned long first_block, last_block;
>   	struct ext4_map_blocks map;
> @@ -3523,31 +3552,9 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   			return ret;
>   	}
>   
> -	iomap->flags = 0;
> -	if (ext4_inode_datasync_dirty(inode))
> -		iomap->flags |= IOMAP_F_DIRTY;
> -	iomap->bdev = inode->i_sb->s_bdev;
> -	iomap->dax_dev = sbi->s_daxdev;
> -	iomap->offset = (u64)first_block << blkbits;
> -	iomap->length = (u64)map.m_len << blkbits;
> -
> -	if (ret == 0) {
> -		iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
> -		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;
> -	}
> -
> -	if (map.m_flags & EXT4_MAP_NEW)
> -		iomap->flags |= IOMAP_F_NEW;
> +	ext4_set_iomap(inode, iomap, &map, offset, length);
> +	if (delalloc && iomap->type == IOMAP_HOLE)
> +		iomap->type = IOMAP_DELALLOC;
>   
>   	return 0;
>   }
>