[v5,04/12] ext4: introduce new callback for IOMAP_REPORT
diff mbox series

Message ID f82e93ccc50014bf6c47318fd089a035d8032b28.1571647179.git.mbobrowski@mbobrowski.org
State Superseded
Headers show
Series
  • ext4: port direct I/O to iomap infrastructure
Related show

Commit Message

Matthew Bobrowski Oct. 21, 2019, 9:18 a.m. UTC
As part of the ext4_iomap_begin() cleanups that precede this patch,
here we also split up the IOMAP_REPORT branch into a completely
separate ->iomap_begin() callback named
ext4_iomap_begin_report(). Again, the raionale for this change is to
reduce the overall clutter that's starting to become apparent as we
start to port more functionality over to the iomap infrastructure.

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

Comments

Jan Kara Oct. 21, 2019, 1:37 p.m. UTC | #1
On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
> As part of the ext4_iomap_begin() cleanups that precede this patch,
> here we also split up the IOMAP_REPORT branch into a completely
> separate ->iomap_begin() callback named
> ext4_iomap_begin_report(). Again, the raionale for this change is to
> reduce the overall clutter that's starting to become apparent as we
> start to port more functionality over to the iomap infrastructure.
> 
> Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>

The patch looks good to me. You can add:

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

One nit below.

> +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
> +				  map->m_lblk, end, &es);
> +
> +	if (!es.es_len || es.es_lblk > end)
> +		return false;
> +
> +	if (es.es_lblk > map->m_lblk) {
> +		map->m_len = es.es_lblk - map->m_lblk;
> +		return false;
> +	}
> +
> +	if (es.es_lblk <= map->m_lblk)

This condition must be always true AFAICT.

> +		offset = map->m_lblk - es.es_lblk;
> +
> +	map->m_lblk = es.es_lblk + offset;
> +	map->m_len = es.es_len - offset;
> +
> +	return true;
> +}

								Honza
Matthew Bobrowski Oct. 22, 2019, 1:55 a.m. UTC | #2
On Mon, Oct 21, 2019 at 03:37:15PM +0200, Jan Kara wrote:
> On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
> One nit below.
> > +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
> > +				  map->m_lblk, end, &es);
> > +
> > +	if (!es.es_len || es.es_lblk > end)
> > +		return false;
> > +
> > +	if (es.es_lblk > map->m_lblk) {
> > +		map->m_len = es.es_lblk - map->m_lblk;
> > +		return false;
> > +	}
> > +
> > +	if (es.es_lblk <= map->m_lblk)
> 
> This condition must be always true AFAICT.

That would make sense. I will remove this condition in v6.

> > +		offset = map->m_lblk - es.es_lblk;
> > +
> > +	map->m_lblk = es.es_lblk + offset;
> > +	map->m_len = es.es_len - offset;
> > +
> > +	return true;
> > +}

--<M>--
Ritesh Harjani Oct. 23, 2019, 6:39 a.m. UTC | #3
On 10/22/19 7:25 AM, Matthew Bobrowski wrote:
> On Mon, Oct 21, 2019 at 03:37:15PM +0200, Jan Kara wrote:
>> On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
>> One nit below.
>>> +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
>>> +				  map->m_lblk, end, &es);
>>> +
>>> +	if (!es.es_len || es.es_lblk > end)
>>> +		return false;
>>> +
>>> +	if (es.es_lblk > map->m_lblk) {
>>> +		map->m_len = es.es_lblk - map->m_lblk;
>>> +		return false;
>>> +	}
>>> +
>>> +	if (es.es_lblk <= map->m_lblk)
>>
>> This condition must be always true AFAICT.
> 
> That would make sense. I will remove this condition in v6.
> 
>>> +		offset = map->m_lblk - es.es_lblk;
>>> +
>>> +	map->m_lblk = es.es_lblk + offset;

And so, this above line will also be redundant.



>>> +	map->m_len = es.es_len - offset;
>>> +
>>> +	return true;
>>> +}
> 
> --<M>--
>
Matthew Bobrowski Oct. 23, 2019, 10:35 a.m. UTC | #4
On Wed, Oct 23, 2019 at 12:09:24PM +0530, Ritesh Harjani wrote:
> On 10/22/19 7:25 AM, Matthew Bobrowski wrote:
> > On Mon, Oct 21, 2019 at 03:37:15PM +0200, Jan Kara wrote:
> > > On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
> > > One nit below.
> > > > +	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
> > > > +				  map->m_lblk, end, &es);
> > > > +
> > > > +	if (!es.es_len || es.es_lblk > end)
> > > > +		return false;
> > > > +
> > > > +	if (es.es_lblk > map->m_lblk) {
> > > > +		map->m_len = es.es_lblk - map->m_lblk;
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	if (es.es_lblk <= map->m_lblk)
> > > > +		offset = map->m_lblk - es.es_lblk;
> > > > +	map->m_lblk = es.es_lblk + offset;
> 
> And so, this above line will also be redundant.

And, you're absolutely right. Nice catch. Who knew basic math could go such a
long way? :P

Thanks Ritesh!

--<M>--

Patch
diff mbox series

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 3dc92bd8a944..ebeedbf3900f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3501,74 +3501,32 @@  static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
 static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned flags, struct iomap *iomap)
 {
-	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;
 
 	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 (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;
-	}
 
-	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 (WARN_ON_ONCE(ext4_has_inline_data(inode)))
+		return -ERANGE;
 
-			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;
+	/*
+	 * Calculate the first and last logical blocks respectively.
+	 */
+	map.m_lblk = offset >> blkbits;
+	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
+			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
 
-				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, &map, flags);
-	} else {
+	else
 		ret = ext4_map_blocks(NULL, inode, &map, 0);
-	}
 
 	if (ret < 0)
 		return ret;
 
 	ext4_set_iomap(inode, iomap, &map, offset, length);
-	if (delalloc && iomap->type == IOMAP_HOLE)
-		iomap->type = IOMAP_DELALLOC;
 
 	return 0;
 }
@@ -3630,6 +3588,77 @@  const struct iomap_ops ext4_iomap_ops = {
 	.iomap_end		= ext4_iomap_end,
 };
 
+static bool ext4_iomap_is_delalloc(struct inode *inode,
+				   struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t offset = 0, end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
+				  map->m_lblk, end, &es);
+
+	if (!es.es_len || es.es_lblk > end)
+		return false;
+
+	if (es.es_lblk > map->m_lblk) {
+		map->m_len = es.es_lblk - map->m_lblk;
+		return false;
+	}
+
+	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 true;
+}
+
+static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
+				   loff_t length, unsigned int flags,
+				   struct iomap *iomap)
+{
+	int ret;
+	bool delalloc = false;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+
+	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
+		return -EINVAL;
+
+	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;
+		}
+	}
+
+	/*
+	 * Calculate the first and last logical blocks respectively.
+	 */
+	map.m_lblk = offset >> blkbits;
+	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
+			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
+
+	ret = ext4_map_blocks(NULL, inode, &map, 0);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		delalloc = ext4_iomap_is_delalloc(inode, &map);
+
+	ext4_set_iomap(inode, iomap, &map, offset, length);
+	if (delalloc && iomap->type == IOMAP_HOLE)
+		iomap->type = IOMAP_DELALLOC;
+
+	return 0;
+}
+
+const struct iomap_ops ext4_iomap_report_ops = {
+	.iomap_begin = ext4_iomap_begin_report,
+};
+
 static int ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
 			    ssize_t size, void *private)
 {