On Mon, Oct 28, 2019 at 09:51:31PM +1100, Matthew Bobrowski wrote:
> Separate the iomap field population code that is currently within
> ext4_iomap_begin() into a separate helper ext4_set_iomap(). The intent
> of this function is self explanatory, however the rationale behind
> taking this step is to reeduce the overall clutter that we currently
> have within the ext4_iomap_begin() callback.
>
> Signed-off-by: Matthew Bobrowski <mbobrowski@xxxxxxxxxxxxxx>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
> Reviewed-by: Ritesh Harjani <riteshh@xxxxxxxxxxxxx>
> ---
> fs/ext4/inode.c | 92 +++++++++++++++++++++++++++----------------------
> 1 file changed, 50 insertions(+), 42 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index da2ca81e3d9c..073b7c873bb2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3406,10 +3406,56 @@ 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;
> +
> + /*
> + * Writes that span EOF might trigger an I/O size update on completion,
> + * so consider them to be dirty for the purpose of O_DSYNC, even if
> + * there is no other metadata changes being made or are pending.
> + */
> + iomap->flags = 0;
> + if (ext4_inode_datasync_dirty(inode) ||
> + offset + length > i_size_read(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)) {
/me wonders if this would be easier to follow if it was less indenty:
/*
* <giant comment from below>
*/
if (m_flags & EXT4_MAP_UNWRITTEN) {
iomap->type = IOMAP_UNWRITTEN;
iomap->addr = ...
} else if (m_flags & EXT4_MAP_MAPPED) {
iomap->type = IOAMP_MAPPED;
iomap->addr = ...
} else {
iomap->type = IOMAP_HOLE;
iomap->addr = IOMAP_NULL_ADDR;
}
Rather than double-checking m_flags?
Otherwise looks fine to me...
--D
> + /*
> + * Flags passed to ext4_map_blocks() for direct I/O writes can
> + * result in m_flags having both EXT4_MAP_MAPPED and
> + * EXT4_MAP_UNWRITTEN bits set. In order for any allocated
> + * unwritten extents to be converted into written extents
> + * correctly within the ->end_io() handler, we need to ensure
> + * that the iomap->type is set appropriately. Hence, the reason
> + * why we need to check whether the EXT4_MAP_UNWRITTEN bit has
> + * been set first.
> + */
> + if (map->m_flags & EXT4_MAP_UNWRITTEN)
> + iomap->type = IOMAP_UNWRITTEN;
> + else if (map->m_flags & EXT4_MAP_MAPPED)
> + iomap->type = IOMAP_MAPPED;
> +
> + 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 iomap *srcmap)
> {
> - 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,47 +3569,9 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> return ret;
> }
>
> - /*
> - * Writes that span EOF might trigger an I/O size update on completion,
> - * so consider them to be dirty for the purposes of O_DSYNC, even if
> - * there is no other metadata changes being made or are pending here.
> - */
> - iomap->flags = 0;
> - if (ext4_inode_datasync_dirty(inode) ||
> - offset + length > i_size_read(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 {
> - /*
> - * Flags passed into ext4_map_blocks() for direct I/O writes
> - * can result in m_flags having both EXT4_MAP_MAPPED and
> - * EXT4_MAP_UNWRITTEN bits set. In order for any allocated
> - * unwritten extents to be converted into written extents
> - * correctly within the ->end_io() handler, we need to ensure
> - * that the iomap->type is set appropriately. Hence the reason
> - * why we need to check whether EXT4_MAP_UNWRITTEN is set
> - * first.
> - */
> - if (map.m_flags & EXT4_MAP_UNWRITTEN) {
> - iomap->type = IOMAP_UNWRITTEN;
> - } else if (map.m_flags & EXT4_MAP_MAPPED) {
> - iomap->type = IOMAP_MAPPED;
> - } 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;
> }
> --
> 2.20.1
>
