diff mbox series

[v4,3/8] ext4: introduce new callback for IOMAP_REPORT operations

Message ID cb2dcb6970da1b53bdf85583f13ba2aaf1684e96.1570100361.git.mbobrowski@mbobrowski.org
State Superseded
Headers show
Series ext4: port direct I/O to iomap infrastructure | expand

Commit Message

Matthew Bobrowski Oct. 3, 2019, 11:33 a.m. UTC 
As part of ext4_iomap_begin() cleanups and port across direct I/O path
to make use of iomap infrastructure, we split IOMAP_REPORT operations
into a separate ->iomap_begin() handler.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
---
 fs/ext4/ext4.h  |   1 +
 fs/ext4/file.c  |   6 ++-
 fs/ext4/inode.c | 129 ++++++++++++++++++++++++++++--------------------
 3 files changed, 80 insertions(+), 56 deletions(-)

Comments

Jan Kara Oct. 8, 2019, 10:42 a.m. UTC | #1 
On Thu 03-10-19 21:33:45, Matthew Bobrowski wrote:
> As part of ext4_iomap_begin() cleanups and port across direct I/O path
> to make use of iomap infrastructure, we split IOMAP_REPORT operations
> into a separate ->iomap_begin() handler.
> 
> Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>

The patch looks good to me. You can add:

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

It would just need small adjustments if you change patch 1 as I suggested:

> +static u16 ext4_iomap_check_delalloc(struct inode *inode,
> +				     struct ext4_map_blocks *map)
> +{
> +	struct extent_status es;
> +	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
> +
> +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
> +				  end, &es);
> +
> +	/* Entire range is a hole */
> +	if (!es.es_len || es.es_lblk > end)
> +		return IOMAP_HOLE;
> +	if (es.es_lblk <= map->m_lblk) {
> +		ext4_lblk_t offset = 0;
> +
> +		if (es.es_lblk < map->m_lblk)
> +			offset = map->m_lblk - es.es_lblk;
> +		map->m_lblk = es.es_lblk + offset;
> +		map->m_len = es.es_len - offset;
> +		return IOMAP_DELALLOC;
> +	}
> +
> +	/* Range starts with a hole */
> +	map->m_len = es.es_lblk - map->m_lblk;
> +	return IOMAP_HOLE;
> +}

This function would then be IMO better off to directly update 'iomap' as
needed after ext4_set_iomap() sets hole there.

								Honza

> +
> +static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
> +				   loff_t length, unsigned flags,
> +				   struct iomap *iomap)
> +{
> +	int ret;
> +	u16 type = 0;
> +	struct ext4_map_blocks map;
> +	u8 blkbits = inode->i_blkbits;
> +	unsigned long first_block, last_block;
> +
> +	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
> +		return -EINVAL;
> +	first_block = offset >> blkbits;
> +	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
> +			   EXT4_MAX_LOGICAL_BLOCK);
> +
> +	if (ext4_has_inline_data(inode)) {
> +		ret = ext4_inline_data_iomap(inode, iomap);
> +		if (ret != -EAGAIN) {
> +			if (ret == 0 && offset >= iomap->length)
> +				ret = -ENOENT;
> +			return ret;
> +		}
> +	}
> +
> +	map.m_lblk = first_block;
> +	map.m_len = last_block = first_block + 1;
> +	ret = ext4_map_blocks(NULL, inode, &map, 0);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0)
> +		type = ext4_iomap_check_delalloc(inode, &map);
> +	return ext4_set_iomap(inode, iomap, type, first_block, &map);
> +}
> +
> +const struct iomap_ops ext4_iomap_report_ops = {
> +	.iomap_begin = ext4_iomap_begin_report,
> +};
> +
>  static int ext4_iomap_alloc(struct inode *inode,
>  			    unsigned flags,
>  			    unsigned long first_block,
> @@ -3498,12 +3564,10 @@ static int ext4_iomap_alloc(struct inode *inode,
>  static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>  			    unsigned flags, struct iomap *iomap)
>  {
> -	u16 type = 0;
> -	unsigned int blkbits = inode->i_blkbits;
> -	unsigned long first_block, last_block;
> -	struct ext4_map_blocks map;
> -	bool delalloc = false;
>  	int ret;
> +	struct ext4_map_blocks map;
> +	u8 blkbits = inode->i_blkbits;
> +	unsigned long first_block, last_block;
>  
>  	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
>  		return -EINVAL;
> @@ -3511,64 +3575,21 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>  	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
>  			   EXT4_MAX_LOGICAL_BLOCK);
>  
> -	if (flags & IOMAP_REPORT) {
> -		if (ext4_has_inline_data(inode)) {
> -			ret = ext4_inline_data_iomap(inode, iomap);
> -			if (ret != -EAGAIN) {
> -				if (ret == 0 && offset >= iomap->length)
> -					ret = -ENOENT;
> -				return ret;
> -			}
> -		}
> -	} else {
> -		if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
> -			return -ERANGE;
> -	}
> +	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
> +		return -ERANGE;
>  
>  	map.m_lblk = first_block;
>  	map.m_len = last_block - first_block + 1;
>  
> -	if (flags & IOMAP_REPORT) {
> -		ret = ext4_map_blocks(NULL, inode, &map, 0);
> -		if (ret < 0)
> -			return ret;
> -
> -		if (ret == 0) {
> -			ext4_lblk_t end = map.m_lblk + map.m_len - 1;
> -			struct extent_status es;
> -
> -			ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
> -						  map.m_lblk, end, &es);
> -
> -			if (!es.es_len || es.es_lblk > end) {
> -				/* entire range is a hole */
> -			} else if (es.es_lblk > map.m_lblk) {
> -				/* range starts with a hole */
> -				map.m_len = es.es_lblk - map.m_lblk;
> -			} else {
> -				ext4_lblk_t offs = 0;
> -
> -				if (es.es_lblk < map.m_lblk)
> -					offs = map.m_lblk - es.es_lblk;
> -				map.m_lblk = es.es_lblk + offs;
> -				map.m_len = es.es_len - offs;
> -				delalloc = true;
> -			}
> -		}
> -	} else if (flags & IOMAP_WRITE) {
> +	if (flags & IOMAP_WRITE)
>  		ret = ext4_iomap_alloc(inode, flags, first_block, &map);
> -	} else {
> +	else
>  		ret = ext4_map_blocks(NULL, inode, &map, 0);
> -		if (ret < 0)
> -			return ret;
> -	}
>  
>  	if (ret < 0)
>  		return ret;
> -
> -	if (!ret)
> -		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
> -	return ext4_set_iomap(inode, iomap, type, first_block, &map);
> +	return ext4_set_iomap(inode, iomap, ret ? 0 : IOMAP_HOLE, first_block,
> +			      &map);
>  }
>  
>  static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
> -- 
> 2.20.1
>
Ritesh Harjani Oct. 9, 2019, 6 a.m. UTC | #2 
On 10/3/19 5:03 PM, Matthew Bobrowski wrote:
> As part of ext4_iomap_begin() cleanups and port across direct I/O path
> to make use of iomap infrastructure, we split IOMAP_REPORT operations
> into a separate ->iomap_begin() handler.
> 
> Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> ---
>   fs/ext4/ext4.h  |   1 +
>   fs/ext4/file.c  |   6 ++-
>   fs/ext4/inode.c | 129 ++++++++++++++++++++++++++++--------------------
>   3 files changed, 80 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 03db3e71676c..d0d88f411a44 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3379,6 +3379,7 @@ static inline void ext4_clear_io_unwritten_flag(ext4_io_end_t *io_end)
>   }
> 
>   extern const struct iomap_ops ext4_iomap_ops;
> +extern const struct iomap_ops ext4_iomap_report_ops;
> 
>   static inline int ext4_buffer_uptodate(struct buffer_head *bh)
>   {
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 8d2bbcc2d813..ab75aee3e687 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -494,12 +494,14 @@ loff_t ext4_llseek(struct file *file, loff_t offset, int whence)
>   						maxbytes, i_size_read(inode));
>   	case SEEK_HOLE:
>   		inode_lock_shared(inode);
> -		offset = iomap_seek_hole(inode, offset, &ext4_iomap_ops);
> +		offset = iomap_seek_hole(inode, offset,
> +					 &ext4_iomap_report_ops);
>   		inode_unlock_shared(inode);
>   		break;
>   	case SEEK_DATA:
>   		inode_lock_shared(inode);
> -		offset = iomap_seek_data(inode, offset, &ext4_iomap_ops);
> +		offset = iomap_seek_data(inode, offset,
> +					 &ext4_iomap_report_ops);
>   		inode_unlock_shared(inode);
>   		break;
>   	}
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index caeb3dec0dec..1dace576b8bd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3439,6 +3439,72 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
>   	return 0;
>   }
> 
> +static u16 ext4_iomap_check_delalloc(struct inode *inode,
> +				     struct ext4_map_blocks *map)
> +{
> +	struct extent_status es;
> +	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
> +
> +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
> +				  end, &es);
> +
> +	/* Entire range is a hole */
> +	if (!es.es_len || es.es_lblk > end)
> +		return IOMAP_HOLE;
> +	if (es.es_lblk <= map->m_lblk) {
> +		ext4_lblk_t offset = 0;
> +
> +		if (es.es_lblk < map->m_lblk)
> +			offset = map->m_lblk - es.es_lblk;
> +		map->m_lblk = es.es_lblk + offset;
This looks redundant no? map->m_lblk never changes actually.
So this is not needed here.


> +		map->m_len = es.es_len - offset;
> +		return IOMAP_DELALLOC;
> +	}
> +
> +	/* Range starts with a hole */
> +	map->m_len = es.es_lblk - map->m_lblk;
> +	return IOMAP_HOLE;
> +}
> +
> +static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
> +				   loff_t length, unsigned flags,
> +				   struct iomap *iomap)
> +{
> +	int ret;
> +	u16 type = 0;
> +	struct ext4_map_blocks map;
> +	u8 blkbits = inode->i_blkbits;
> +	unsigned long first_block, last_block;
> +
> +	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
> +		return -EINVAL;
> +	first_block = offset >> blkbits;
> +	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
> +			   EXT4_MAX_LOGICAL_BLOCK);
> +
> +	if (ext4_has_inline_data(inode)) {
> +		ret = ext4_inline_data_iomap(inode, iomap);
> +		if (ret != -EAGAIN) {
> +			if (ret == 0 && offset >= iomap->length)
> +				ret = -ENOENT;
> +			return ret;
> +		}
> +	}
> +
> +	map.m_lblk = first_block;
> +	map.m_len = last_block = first_block + 1;
> +	ret = ext4_map_blocks(NULL, inode, &map, 0);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0)
> +		type = ext4_iomap_check_delalloc(inode, &map);
> +	return ext4_set_iomap(inode, iomap, type, first_block, &map);
We don't need to send first_block here. Since map->m_lblk
is same as first_block.
Also with Jan comment, we don't even need 'type' parameter.
Then we should be able to rename the function
ext4_set_iomap ==> ext4_map_to_iomap. This better reflects what it is
doing. Thoughts?


> +}
> +
> +const struct iomap_ops ext4_iomap_report_ops = {
> +	.iomap_begin = ext4_iomap_begin_report,
> +};
> +
>   static int ext4_iomap_alloc(struct inode *inode,
>   			    unsigned flags,
>   			    unsigned long first_block,
> @@ -3498,12 +3564,10 @@ static int ext4_iomap_alloc(struct inode *inode,
>   static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   			    unsigned flags, struct iomap *iomap)
>   {
> -	u16 type = 0;
> -	unsigned int blkbits = inode->i_blkbits;
> -	unsigned long first_block, last_block;
> -	struct ext4_map_blocks map;
> -	bool delalloc = false;
>   	int ret;
> +	struct ext4_map_blocks map;
> +	u8 blkbits = inode->i_blkbits;
> +	unsigned long first_block, last_block;
> 
>   	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
>   		return -EINVAL;
> @@ -3511,64 +3575,21 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
>   			   EXT4_MAX_LOGICAL_BLOCK);
> 
> -	if (flags & IOMAP_REPORT) {
> -		if (ext4_has_inline_data(inode)) {
> -			ret = ext4_inline_data_iomap(inode, iomap);
> -			if (ret != -EAGAIN) {
> -				if (ret == 0 && offset >= iomap->length)
> -					ret = -ENOENT;
> -				return ret;
> -			}
> -		}
> -	} else {
> -		if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
> -			return -ERANGE;
> -	}
> +	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
> +		return -ERANGE;
> 
>   	map.m_lblk = first_block;
>   	map.m_len = last_block - first_block + 1;
> 
> -	if (flags & IOMAP_REPORT) {
> -		ret = ext4_map_blocks(NULL, inode, &map, 0);
> -		if (ret < 0)
> -			return ret;
> -
> -		if (ret == 0) {
> -			ext4_lblk_t end = map.m_lblk + map.m_len - 1;
> -			struct extent_status es;
> -
> -			ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
> -						  map.m_lblk, end, &es);
> -
> -			if (!es.es_len || es.es_lblk > end) {
> -				/* entire range is a hole */
> -			} else if (es.es_lblk > map.m_lblk) {
> -				/* range starts with a hole */
> -				map.m_len = es.es_lblk - map.m_lblk;
> -			} else {
> -				ext4_lblk_t offs = 0;
> -
> -				if (es.es_lblk < map.m_lblk)
> -					offs = map.m_lblk - es.es_lblk;
> -				map.m_lblk = es.es_lblk + offs;
> -				map.m_len = es.es_len - offs;
> -				delalloc = true;
> -			}
> -		}
> -	} else if (flags & IOMAP_WRITE) {
> +	if (flags & IOMAP_WRITE)
>   		ret = ext4_iomap_alloc(inode, flags, first_block, &map);
> -	} else {
> +	else
>   		ret = ext4_map_blocks(NULL, inode, &map, 0);
> -		if (ret < 0)
> -			return ret;
> -	}
> 
>   	if (ret < 0)
>   		return ret;
> -
> -	if (!ret)
> -		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
> -	return ext4_set_iomap(inode, iomap, type, first_block, &map);
> +	return ext4_set_iomap(inode, iomap, ret ? 0 : IOMAP_HOLE, first_block,
> +			      &map);
>   }
> 
>   static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>
Matthew Bobrowski Oct. 9, 2019, 9:41 a.m. UTC | #3 
On Tue, Oct 08, 2019 at 12:42:09PM +0200, Jan Kara wrote:
> On Thu 03-10-19 21:33:45, Matthew Bobrowski wrote:
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks Jan! :)

> It would just need small adjustments if you change patch 1 as I suggested:

I will await what you say in response to what my thoughts were aronud
ext4_set_iomap() before doing any updates here.

> > +static u16 ext4_iomap_check_delalloc(struct inode *inode,
> > +				     struct ext4_map_blocks *map)
> > +{
> > +	struct extent_status es;
> > +	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
> > +
> > +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
> > +				  end, &es);
> > +
> > +	/* Entire range is a hole */
> > +	if (!es.es_len || es.es_lblk > end)
> > +		return IOMAP_HOLE;
> > +	if (es.es_lblk <= map->m_lblk) {
> > +		ext4_lblk_t offset = 0;
> > +
> > +		if (es.es_lblk < map->m_lblk)
> > +			offset = map->m_lblk - es.es_lblk;
> > +		map->m_lblk = es.es_lblk + offset;
> > +		map->m_len = es.es_len - offset;
> > +		return IOMAP_DELALLOC;
> > +	}
> > +
> > +	/* Range starts with a hole */
> > +	map->m_len = es.es_lblk - map->m_lblk;
> > +	return IOMAP_HOLE;
> > +}
> 
> This function would then be IMO better off to directly update 'iomap' as
> needed after ext4_set_iomap() sets hole there.

As mentioned in 1/8, it would be nice to leave all iomap setting up to
ext4_set_iomap(), but if we're strongly against passing 'type', then
I'm happy to change it and update this to pass an 'iomap'.

--<M>--
Matthew Bobrowski Oct. 9, 2019, 12:08 p.m. UTC | #4 
On Wed, Oct 09, 2019 at 11:30:21AM +0530, Ritesh Harjani wrote:
> > +static u16 ext4_iomap_check_delalloc(struct inode *inode,
> > +				     struct ext4_map_blocks *map)
> > +{
> > +	struct extent_status es;
> > +	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
> > +
> > +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
> > +				  end, &es);
> > +
> > +	/* Entire range is a hole */
> > +	if (!es.es_len || es.es_lblk > end)
> > +		return IOMAP_HOLE;
> > +	if (es.es_lblk <= map->m_lblk) {
> > +		ext4_lblk_t offset = 0;
> > +
> > +		if (es.es_lblk < map->m_lblk)
> > +			offset = map->m_lblk - es.es_lblk;
> > +		map->m_lblk = es.es_lblk + offset;
> This looks redundant no? map->m_lblk never changes actually.
> So this is not needed here.

Well, it depends if map->m_lblk == es.es_lblk + offset prior to the
assignment? If that's always true, then sure, it'd be redundant. But
honestly, I don't know what the downstream effect would be if this was
removed. I'd have to look at the code, perform some tests, and figure
it out.

> > +	map.m_lblk = first_block;
> > +	map.m_len = last_block = first_block + 1;
> > +	ret = ext4_map_blocks(NULL, inode, &map, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret == 0)
> > +		type = ext4_iomap_check_delalloc(inode, &map);
> > +	return ext4_set_iomap(inode, iomap, type, first_block, &map);
> We don't need to send first_block here. Since map->m_lblk
> is same as first_block.
> Also with Jan comment, we don't even need 'type' parameter.
> Then we should be able to rename the function
> ext4_set_iomap ==> ext4_map_to_iomap. This better reflects what it is
> doing. Thoughts?

Depends on what we conclude in 1/8. :)

I'm for removing 'first_block', but still not convinced removing
'type' is heading down the right track if I were to forward think a
little.

--<M>--
Ritesh Harjani Oct. 9, 2019, 1:14 p.m. UTC | #5 
On 10/9/19 5:38 PM, Matthew Bobrowski wrote:
> On Wed, Oct 09, 2019 at 11:30:21AM +0530, Ritesh Harjani wrote:
>>> +static u16 ext4_iomap_check_delalloc(struct inode *inode,
>>> +				     struct ext4_map_blocks *map)
>>> +{
>>> +	struct extent_status es;
>>> +	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
>>> +
>>> +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
>>> +				  end, &es);
>>> +
>>> +	/* Entire range is a hole */
>>> +	if (!es.es_len || es.es_lblk > end)
>>> +		return IOMAP_HOLE;
>>> +	if (es.es_lblk <= map->m_lblk) {
>>> +		ext4_lblk_t offset = 0;
>>> +
>>> +		if (es.es_lblk < map->m_lblk)
>>> +			offset = map->m_lblk - es.es_lblk;
>>> +		map->m_lblk = es.es_lblk + offset;
>> This looks redundant no? map->m_lblk never changes actually.
>> So this is not needed here.
> 
> Well, it depends if map->m_lblk == es.es_lblk + offset prior to the
> assignment? If that's always true, then sure, it'd be redundant. But
> honestly, I don't know what the downstream effect would be if this was
> removed. I'd have to look at the code, perform some tests, and figure
> it out.

<snip>
3334         if (es.es_lblk <= map->m_lblk) {
3335                 ext4_lblk_t offset = 0;
3336
3337                 if (es.es_lblk < map->m_lblk)
3338                         offset = map->m_lblk - es.es_lblk;
3339                 map->m_lblk = es.es_lblk + offset;
3340                 map->m_len = es.es_len - offset;
3341                 return IOMAP_DELALLOC;
3342         }

I saw it this way-

In condition "if (es.es_lblk <= map->m_lblk)" there are 2 cases.

Case 1: es.es_lblk is equal to map->m_lblk (equality)
    For this case, "offset" will remain 0.
    So map->lblk = es.es_lblk + 0 (but since es.es_lblk is same as
map->m_lblk in equality case, so it is redundant).


Case 2: es.es_lblk < map->m_lblk (less than)
In this case "offset = map->m_lblk - es.es_lblk"
Now replacing this val of offset in "map->m_lblk = es.es_lblk + offset"
map->m_lblk = es.es_lblk + map->m_lblk - es.es_lblk
which again is map->m_lblk = map->m_lblk - again redundant.


What did I miss?
But sure feel free to test as per your convenience.


> 
>>> +	map.m_lblk = first_block;
>>> +	map.m_len = last_block = first_block + 1;
>>> +	ret = ext4_map_blocks(NULL, inode, &map, 0);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	if (ret == 0)
>>> +		type = ext4_iomap_check_delalloc(inode, &map);
>>> +	return ext4_set_iomap(inode, iomap, type, first_block, &map);
>> We don't need to send first_block here. Since map->m_lblk
>> is same as first_block.
>> Also with Jan comment, we don't even need 'type' parameter.
>> Then we should be able to rename the function
>> ext4_set_iomap ==> ext4_map_to_iomap. This better reflects what it is
>> doing. Thoughts?
> 
> Depends on what we conclude in 1/8. :)
> 
> I'm for removing 'first_block', but still not convinced removing
> 'type' is heading down the right track if I were to forward think a
> little.

Only once you are convinced that map->m_lblk will not change even in
function ext4_iomap_check_delalloc(), then only you should
drop "first_block" argument from ext4_set_iomap.

Please check above comments once.

-ritesh
diff mbox series

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 03db3e71676c..d0d88f411a44 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3379,6 +3379,7 @@  static inline void ext4_clear_io_unwritten_flag(ext4_io_end_t *io_end)
 }
 
 extern const struct iomap_ops ext4_iomap_ops;
+extern const struct iomap_ops ext4_iomap_report_ops;
 
 static inline int ext4_buffer_uptodate(struct buffer_head *bh)
 {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 8d2bbcc2d813..ab75aee3e687 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -494,12 +494,14 @@  loff_t ext4_llseek(struct file *file, loff_t offset, int whence)
 						maxbytes, i_size_read(inode));
 	case SEEK_HOLE:
 		inode_lock_shared(inode);
-		offset = iomap_seek_hole(inode, offset, &ext4_iomap_ops);
+		offset = iomap_seek_hole(inode, offset,
+					 &ext4_iomap_report_ops);
 		inode_unlock_shared(inode);
 		break;
 	case SEEK_DATA:
 		inode_lock_shared(inode);
-		offset = iomap_seek_data(inode, offset, &ext4_iomap_ops);
+		offset = iomap_seek_data(inode, offset,
+					 &ext4_iomap_report_ops);
 		inode_unlock_shared(inode);
 		break;
 	}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index caeb3dec0dec..1dace576b8bd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3439,6 +3439,72 @@  static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
 	return 0;
 }
 
+static u16 ext4_iomap_check_delalloc(struct inode *inode,
+				     struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
+				  end, &es);
+
+	/* Entire range is a hole */
+	if (!es.es_len || es.es_lblk > end)
+		return IOMAP_HOLE;
+	if (es.es_lblk <= map->m_lblk) {
+		ext4_lblk_t offset = 0;
+
+		if (es.es_lblk < map->m_lblk)
+			offset = map->m_lblk - es.es_lblk;
+		map->m_lblk = es.es_lblk + offset;
+		map->m_len = es.es_len - offset;
+		return IOMAP_DELALLOC;
+	}
+
+	/* Range starts with a hole */
+	map->m_len = es.es_lblk - map->m_lblk;
+	return IOMAP_HOLE;
+}
+
+static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
+				   loff_t length, unsigned flags,
+				   struct iomap *iomap)
+{
+	int ret;
+	u16 type = 0;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+	unsigned long first_block, last_block;
+
+	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
+		return -EINVAL;
+	first_block = offset >> blkbits;
+	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
+			   EXT4_MAX_LOGICAL_BLOCK);
+
+	if (ext4_has_inline_data(inode)) {
+		ret = ext4_inline_data_iomap(inode, iomap);
+		if (ret != -EAGAIN) {
+			if (ret == 0 && offset >= iomap->length)
+				ret = -ENOENT;
+			return ret;
+		}
+	}
+
+	map.m_lblk = first_block;
+	map.m_len = last_block = first_block + 1;
+	ret = ext4_map_blocks(NULL, inode, &map, 0);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		type = ext4_iomap_check_delalloc(inode, &map);
+	return ext4_set_iomap(inode, iomap, type, first_block, &map);
+}
+
+const struct iomap_ops ext4_iomap_report_ops = {
+	.iomap_begin = ext4_iomap_begin_report,
+};
+
 static int ext4_iomap_alloc(struct inode *inode,
 			    unsigned flags,
 			    unsigned long first_block,
@@ -3498,12 +3564,10 @@  static int ext4_iomap_alloc(struct inode *inode,
 static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned flags, struct iomap *iomap)
 {
-	u16 type = 0;
-	unsigned int blkbits = inode->i_blkbits;
-	unsigned long first_block, last_block;
-	struct ext4_map_blocks map;
-	bool delalloc = false;
 	int ret;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+	unsigned long first_block, last_block;
 
 	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
 		return -EINVAL;
@@ -3511,64 +3575,21 @@  static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
 			   EXT4_MAX_LOGICAL_BLOCK);
 
-	if (flags & IOMAP_REPORT) {
-		if (ext4_has_inline_data(inode)) {
-			ret = ext4_inline_data_iomap(inode, iomap);
-			if (ret != -EAGAIN) {
-				if (ret == 0 && offset >= iomap->length)
-					ret = -ENOENT;
-				return ret;
-			}
-		}
-	} else {
-		if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
-			return -ERANGE;
-	}
+	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
+		return -ERANGE;
 
 	map.m_lblk = first_block;
 	map.m_len = last_block - first_block + 1;
 
-	if (flags & IOMAP_REPORT) {
-		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
-
-		if (ret == 0) {
-			ext4_lblk_t end = map.m_lblk + map.m_len - 1;
-			struct extent_status es;
-
-			ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
-						  map.m_lblk, end, &es);
-
-			if (!es.es_len || es.es_lblk > end) {
-				/* entire range is a hole */
-			} else if (es.es_lblk > map.m_lblk) {
-				/* range starts with a hole */
-				map.m_len = es.es_lblk - map.m_lblk;
-			} else {
-				ext4_lblk_t offs = 0;
-
-				if (es.es_lblk < map.m_lblk)
-					offs = map.m_lblk - es.es_lblk;
-				map.m_lblk = es.es_lblk + offs;
-				map.m_len = es.es_len - offs;
-				delalloc = true;
-			}
-		}
-	} else if (flags & IOMAP_WRITE) {
+	if (flags & IOMAP_WRITE)
 		ret = ext4_iomap_alloc(inode, flags, first_block, &map);
-	} else {
+	else
 		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
-	}
 
 	if (ret < 0)
 		return ret;
-
-	if (!ret)
-		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
-	return ext4_set_iomap(inode, iomap, type, first_block, &map);
+	return ext4_set_iomap(inode, iomap, ret ? 0 : IOMAP_HOLE, first_block,
+			      &map);
 }
 
 static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,