[v5,01/12] ext4: move set iomap routines into separate helper ext4_set_iomap()

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@mbobrowski.org>
---
 fs/ext4/inode.c | 59 +++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

On Mon 21-10-19 20:17:31, 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@mbobrowski.org>
> ---
>  fs/ext4/inode.c | 59 +++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 26 deletions(-)

The patch looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

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@mbobrowski.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@linux.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;
>   }
>

On Wed, Oct 23, 2019 at 12:01:51PM +0530, Ritesh Harjani wrote:
> 
> 
> 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@mbobrowski.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).

No objections to this. Just needing to do a little shuffle here and there.

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

Sure, thanks for educating me and making me aware of this.
 
> This patch looks good to me. You may add:
> 
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

Thanks Ritesh!
 
--<M>--