[PATCHv9 0/6] iomap: Add support for per-block dirty state to improve write performance

[PATCHv9 0/6] iomap: Add support for per-block dirty state to improve write performance

[Ritesh Harjani (IBM)]

Hello All,

Please find PATCHv9 which adds per-block dirty tracking to iomap.
As discussed earlier this is required to improve write performance and reduce
write amplification for cases where either blocksize is less than pagesize (such
as Power platform with 64k pagesize) or when we have a large folio (such as xfs
which currently supports large folio).

v7/v8 -> v9
============
1. Splitted the renaming & refactoring changes into different patchsets.
   (Patch-1 & Patch-2)
2. Addressed review comments from everyone in v9.
3. Fixed a punch-out bug pointed out by Darrick in Patch-6.
4. Included iomap_ifs_calc_range() function suggested by Christoph in Patch-6.

Testing
=========
I have tested v9 on:-
   - Power with 4k blocksize -g auto
   - x86 with 1k and 1k_adv with -g auto
   - arm64 with 4k blocksize and 64k pagesize with 4k quick
   - also tested gfs2 with minimal local config (-O -b 1024 -p lock_nolock)
   - unit tested failed punch-out operation with "-f" support to pwrite in
     xfs_io.
I haven't observed any new testcase failures in any of my testing so far.

Thanks everyone for helping with reviews and suggestions.
Please do let me know if there are any further review comments on this one.

<Perf data copy paste from previous version>
=============================================
Performance testing of below fio workload reveals ~16x performance
improvement using nvme with XFS (4k blocksize) on Power (64K pagesize)
FIO reported write bw scores improved from around ~28 MBps to ~452 MBps.

1. <test_randwrite.fio>
[global]
	ioengine=psync
	rw=randwrite
	overwrite=1
	pre_read=1
	direct=0
	bs=4k
	size=1G
	dir=./
	numjobs=8
	fdatasync=1
	runtime=60
	iodepth=64
	group_reporting=1

[fio-run]

2. Also our internal performance team reported that this patch improves
   their database workload performance by around ~83% (with XFS on Power)

Ritesh Harjani (IBM) (6):
  iomap: Rename iomap_page to iomap_folio_state and others
  iomap: Drop ifs argument from iomap_set_range_uptodate()
  iomap: Add some uptodate state handling helpers for ifs state bitmap
  iomap: Refactor iomap_write_delalloc_punch() function out
  iomap: Allocate ifs in ->write_begin() early
  iomap: Add per-block dirty state tracking to improve performance

 fs/gfs2/aops.c         |   2 +-
 fs/iomap/buffered-io.c | 394 +++++++++++++++++++++++++++++------------
 fs/xfs/xfs_aops.c      |   2 +-
 fs/zonefs/file.c       |   2 +-
 include/linux/iomap.h  |   1 +
 5 files changed, 282 insertions(+), 119 deletions(-)

--
2.40.1

[PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Ritesh Harjani (IBM)]

struct iomap_page actually tracks per-block state of a folio.
Hence it make sense to rename some of these function names and data
structures for e.g.
1. struct iomap_page (iop) -> struct iomap_folio_state (ifs)
2. iomap_page_create() -> iomap_ifs_alloc()
3. iomap_page_release() -> iomap_ifs_free()
4. to_iomap_page() -> iomap_get_ifs()

Since in later patches we are also going to add per-block dirty state
tracking to iomap_folio_state. Hence this patch also renames "uptodate"
& "uptodate_lock" members of iomap_folio_state to "state" and"state_lock".

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/buffered-io.c | 146 ++++++++++++++++++++---------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 063133ec77f4..779205fe228f 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -24,17 +24,17 @@
 #define IOEND_BATCH_SIZE	4096
 
 /*
- * Structure allocated for each folio when block size < folio size
- * to track sub-folio uptodate status and I/O completions.
+ * Structure allocated for each folio to track per-block uptodate state
+ * and I/O completions.
  */
-struct iomap_page {
+struct iomap_folio_state {
 	atomic_t		read_bytes_pending;
 	atomic_t		write_bytes_pending;
-	spinlock_t		uptodate_lock;
-	unsigned long		uptodate[];
+	spinlock_t		state_lock;
+	unsigned long		state[];
 };
 
-static inline struct iomap_page *to_iomap_page(struct folio *folio)
+static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
 {
 	if (folio_test_private(folio))
 		return folio_get_private(folio);
@@ -43,45 +43,45 @@ static inline struct iomap_page *to_iomap_page(struct folio *folio)
 
 static struct bio_set iomap_ioend_bioset;
 
-static struct iomap_page *
-iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags)
+static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
+		struct folio *folio, unsigned int flags)
 {
-	struct iomap_page *iop = to_iomap_page(folio);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
 	gfp_t gfp;
 
-	if (iop || nr_blocks <= 1)
-		return iop;
+	if (ifs || nr_blocks <= 1)
+		return ifs;
 
 	if (flags & IOMAP_NOWAIT)
 		gfp = GFP_NOWAIT;
 	else
 		gfp = GFP_NOFS | __GFP_NOFAIL;
 
-	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
+	ifs = kzalloc(struct_size(ifs, state, BITS_TO_LONGS(nr_blocks)),
 		      gfp);
-	if (iop) {
-		spin_lock_init(&iop->uptodate_lock);
+	if (ifs) {
+		spin_lock_init(&ifs->state_lock);
 		if (folio_test_uptodate(folio))
-			bitmap_fill(iop->uptodate, nr_blocks);
-		folio_attach_private(folio, iop);
+			bitmap_fill(ifs->state, nr_blocks);
+		folio_attach_private(folio, ifs);
 	}
-	return iop;
+	return ifs;
 }
 
-static void iomap_page_release(struct folio *folio)
+static void iomap_ifs_free(struct folio *folio)
 {
-	struct iomap_page *iop = folio_detach_private(folio);
+	struct iomap_folio_state *ifs = folio_detach_private(folio);
 	struct inode *inode = folio->mapping->host;
 	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
 
-	if (!iop)
+	if (!ifs)
 		return;
-	WARN_ON_ONCE(atomic_read(&iop->read_bytes_pending));
-	WARN_ON_ONCE(atomic_read(&iop->write_bytes_pending));
-	WARN_ON_ONCE(bitmap_full(iop->uptodate, nr_blocks) !=
+	WARN_ON_ONCE(atomic_read(&ifs->read_bytes_pending));
+	WARN_ON_ONCE(atomic_read(&ifs->write_bytes_pending));
+	WARN_ON_ONCE(bitmap_full(ifs->state, nr_blocks) !=
 			folio_test_uptodate(folio));
-	kfree(iop);
+	kfree(ifs);
 }
 
 /*
@@ -90,7 +90,7 @@ static void iomap_page_release(struct folio *folio)
 static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 		loff_t *pos, loff_t length, size_t *offp, size_t *lenp)
 {
-	struct iomap_page *iop = to_iomap_page(folio);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 	loff_t orig_pos = *pos;
 	loff_t isize = i_size_read(inode);
 	unsigned block_bits = inode->i_blkbits;
@@ -105,12 +105,12 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 	 * per-block uptodate status and adjust the offset and length if needed
 	 * to avoid reading in already uptodate ranges.
 	 */
-	if (iop) {
+	if (ifs) {
 		unsigned int i;
 
 		/* move forward for each leading block marked uptodate */
 		for (i = first; i <= last; i++) {
-			if (!test_bit(i, iop->uptodate))
+			if (!test_bit(i, ifs->state))
 				break;
 			*pos += block_size;
 			poff += block_size;
@@ -120,7 +120,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 
 		/* truncate len if we find any trailing uptodate block(s) */
 		for ( ; i <= last; i++) {
-			if (test_bit(i, iop->uptodate)) {
+			if (test_bit(i, ifs->state)) {
 				plen -= (last - i + 1) * block_size;
 				last = i - 1;
 				break;
@@ -144,26 +144,26 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 	*lenp = plen;
 }
 
-static void iomap_iop_set_range_uptodate(struct folio *folio,
-		struct iomap_page *iop, size_t off, size_t len)
+static void iomap_ifs_set_range_uptodate(struct folio *folio,
+		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
 	struct inode *inode = folio->mapping->host;
 	unsigned first = off >> inode->i_blkbits;
 	unsigned last = (off + len - 1) >> inode->i_blkbits;
 	unsigned long flags;
 
-	spin_lock_irqsave(&iop->uptodate_lock, flags);
-	bitmap_set(iop->uptodate, first, last - first + 1);
-	if (bitmap_full(iop->uptodate, i_blocks_per_folio(inode, folio)))
+	spin_lock_irqsave(&ifs->state_lock, flags);
+	bitmap_set(ifs->state, first, last - first + 1);
+	if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
 		folio_mark_uptodate(folio);
-	spin_unlock_irqrestore(&iop->uptodate_lock, flags);
+	spin_unlock_irqrestore(&ifs->state_lock, flags);
 }
 
 static void iomap_set_range_uptodate(struct folio *folio,
-		struct iomap_page *iop, size_t off, size_t len)
+		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
-	if (iop)
-		iomap_iop_set_range_uptodate(folio, iop, off, len);
+	if (ifs)
+		iomap_ifs_set_range_uptodate(folio, ifs, off, len);
 	else
 		folio_mark_uptodate(folio);
 }
@@ -171,16 +171,16 @@ static void iomap_set_range_uptodate(struct folio *folio,
 static void iomap_finish_folio_read(struct folio *folio, size_t offset,
 		size_t len, int error)
 {
-	struct iomap_page *iop = to_iomap_page(folio);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 
 	if (unlikely(error)) {
 		folio_clear_uptodate(folio);
 		folio_set_error(folio);
 	} else {
-		iomap_set_range_uptodate(folio, iop, offset, len);
+		iomap_set_range_uptodate(folio, ifs, offset, len);
 	}
 
-	if (!iop || atomic_sub_and_test(len, &iop->read_bytes_pending))
+	if (!ifs || atomic_sub_and_test(len, &ifs->read_bytes_pending))
 		folio_unlock(folio);
 }
 
@@ -213,7 +213,7 @@ struct iomap_readpage_ctx {
 static int iomap_read_inline_data(const struct iomap_iter *iter,
 		struct folio *folio)
 {
-	struct iomap_page *iop;
+	struct iomap_folio_state *ifs;
 	const struct iomap *iomap = iomap_iter_srcmap(iter);
 	size_t size = i_size_read(iter->inode) - iomap->offset;
 	size_t poff = offset_in_page(iomap->offset);
@@ -231,15 +231,15 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
 	if (offset > 0)
-		iop = iomap_page_create(iter->inode, folio, iter->flags);
+		ifs = iomap_ifs_alloc(iter->inode, folio, iter->flags);
 	else
-		iop = to_iomap_page(folio);
+		ifs = iomap_get_ifs(folio);
 
 	addr = kmap_local_folio(folio, offset);
 	memcpy(addr, iomap->inline_data, size);
 	memset(addr + size, 0, PAGE_SIZE - poff - size);
 	kunmap_local(addr);
-	iomap_set_range_uptodate(folio, iop, offset, PAGE_SIZE - poff);
+	iomap_set_range_uptodate(folio, ifs, offset, PAGE_SIZE - poff);
 	return 0;
 }
 
@@ -260,7 +260,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
 	loff_t pos = iter->pos + offset;
 	loff_t length = iomap_length(iter) - offset;
 	struct folio *folio = ctx->cur_folio;
-	struct iomap_page *iop;
+	struct iomap_folio_state *ifs;
 	loff_t orig_pos = pos;
 	size_t poff, plen;
 	sector_t sector;
@@ -269,20 +269,20 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
 		return iomap_read_inline_data(iter, folio);
 
 	/* zero post-eof blocks as the page may be mapped */
-	iop = iomap_page_create(iter->inode, folio, iter->flags);
+	ifs = iomap_ifs_alloc(iter->inode, folio, iter->flags);
 	iomap_adjust_read_range(iter->inode, folio, &pos, length, &poff, &plen);
 	if (plen == 0)
 		goto done;
 
 	if (iomap_block_needs_zeroing(iter, pos)) {
 		folio_zero_range(folio, poff, plen);
-		iomap_set_range_uptodate(folio, iop, poff, plen);
+		iomap_set_range_uptodate(folio, ifs, poff, plen);
 		goto done;
 	}
 
 	ctx->cur_folio_in_bio = true;
-	if (iop)
-		atomic_add(plen, &iop->read_bytes_pending);
+	if (ifs)
+		atomic_add(plen, &ifs->read_bytes_pending);
 
 	sector = iomap_sector(iomap, pos);
 	if (!ctx->bio ||
@@ -436,11 +436,11 @@ EXPORT_SYMBOL_GPL(iomap_readahead);
  */
 bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
 {
-	struct iomap_page *iop = to_iomap_page(folio);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 	struct inode *inode = folio->mapping->host;
 	unsigned first, last, i;
 
-	if (!iop)
+	if (!ifs)
 		return false;
 
 	/* Caller's range may extend past the end of this folio */
@@ -451,7 +451,7 @@ bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
 	last = (from + count - 1) >> inode->i_blkbits;
 
 	for (i = first; i <= last; i++)
-		if (!test_bit(i, iop->uptodate))
+		if (!test_bit(i, ifs->state))
 			return false;
 	return true;
 }
@@ -490,7 +490,7 @@ bool iomap_release_folio(struct folio *folio, gfp_t gfp_flags)
 	 */
 	if (folio_test_dirty(folio) || folio_test_writeback(folio))
 		return false;
-	iomap_page_release(folio);
+	iomap_ifs_free(folio);
 	return true;
 }
 EXPORT_SYMBOL_GPL(iomap_release_folio);
@@ -507,12 +507,12 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len)
 	if (offset == 0 && len == folio_size(folio)) {
 		WARN_ON_ONCE(folio_test_writeback(folio));
 		folio_cancel_dirty(folio);
-		iomap_page_release(folio);
+		iomap_ifs_free(folio);
 	} else if (folio_test_large(folio)) {
-		/* Must release the iop so the page can be split */
+		/* Must release the ifs so the page can be split */
 		WARN_ON_ONCE(!folio_test_uptodate(folio) &&
 			     folio_test_dirty(folio));
-		iomap_page_release(folio);
+		iomap_ifs_free(folio);
 	}
 }
 EXPORT_SYMBOL_GPL(iomap_invalidate_folio);
@@ -547,7 +547,7 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 		size_t len, struct folio *folio)
 {
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
-	struct iomap_page *iop;
+	struct iomap_folio_state *ifs;
 	loff_t block_size = i_blocksize(iter->inode);
 	loff_t block_start = round_down(pos, block_size);
 	loff_t block_end = round_up(pos + len, block_size);
@@ -559,8 +559,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 		return 0;
 	folio_clear_error(folio);
 
-	iop = iomap_page_create(iter->inode, folio, iter->flags);
-	if ((iter->flags & IOMAP_NOWAIT) && !iop && nr_blocks > 1)
+	ifs = iomap_ifs_alloc(iter->inode, folio, iter->flags);
+	if ((iter->flags & IOMAP_NOWAIT) && !ifs && nr_blocks > 1)
 		return -EAGAIN;
 
 	do {
@@ -589,7 +589,7 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 			if (status)
 				return status;
 		}
-		iomap_set_range_uptodate(folio, iop, poff, plen);
+		iomap_set_range_uptodate(folio, ifs, poff, plen);
 	} while ((block_start += plen) < block_end);
 
 	return 0;
@@ -696,7 +696,7 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
 static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
 		size_t copied, struct folio *folio)
 {
-	struct iomap_page *iop = to_iomap_page(folio);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 	flush_dcache_folio(folio);
 
 	/*
@@ -712,7 +712,7 @@ static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
 	 */
 	if (unlikely(copied < len && !folio_test_uptodate(folio)))
 		return 0;
-	iomap_set_range_uptodate(folio, iop, offset_in_folio(folio, pos), len);
+	iomap_set_range_uptodate(folio, ifs, offset_in_folio(folio, pos), len);
 	filemap_dirty_folio(inode->i_mapping, folio);
 	return copied;
 }
@@ -1290,17 +1290,17 @@ EXPORT_SYMBOL_GPL(iomap_page_mkwrite);
 static void iomap_finish_folio_write(struct inode *inode, struct folio *folio,
 		size_t len, int error)
 {
-	struct iomap_page *iop = to_iomap_page(folio);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 
 	if (error) {
 		folio_set_error(folio);
 		mapping_set_error(inode->i_mapping, error);
 	}
 
-	WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !iop);
-	WARN_ON_ONCE(iop && atomic_read(&iop->write_bytes_pending) <= 0);
+	WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !ifs);
+	WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) <= 0);
 
-	if (!iop || atomic_sub_and_test(len, &iop->write_bytes_pending))
+	if (!ifs || atomic_sub_and_test(len, &ifs->write_bytes_pending))
 		folio_end_writeback(folio);
 }
 
@@ -1567,7 +1567,7 @@ iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t offset,
  */
 static void
 iomap_add_to_ioend(struct inode *inode, loff_t pos, struct folio *folio,
-		struct iomap_page *iop, struct iomap_writepage_ctx *wpc,
+		struct iomap_folio_state *ifs, struct iomap_writepage_ctx *wpc,
 		struct writeback_control *wbc, struct list_head *iolist)
 {
 	sector_t sector = iomap_sector(&wpc->iomap, pos);
@@ -1585,8 +1585,8 @@ iomap_add_to_ioend(struct inode *inode, loff_t pos, struct folio *folio,
 		bio_add_folio(wpc->ioend->io_bio, folio, len, poff);
 	}
 
-	if (iop)
-		atomic_add(len, &iop->write_bytes_pending);
+	if (ifs)
+		atomic_add(len, &ifs->write_bytes_pending);
 	wpc->ioend->io_size += len;
 	wbc_account_cgroup_owner(wbc, &folio->page, len);
 }
@@ -1612,7 +1612,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		struct writeback_control *wbc, struct inode *inode,
 		struct folio *folio, u64 end_pos)
 {
-	struct iomap_page *iop = iomap_page_create(inode, folio, 0);
+	struct iomap_folio_state *ifs = iomap_ifs_alloc(inode, folio, 0);
 	struct iomap_ioend *ioend, *next;
 	unsigned len = i_blocksize(inode);
 	unsigned nblocks = i_blocks_per_folio(inode, folio);
@@ -1620,7 +1620,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	int error = 0, count = 0, i;
 	LIST_HEAD(submit_list);
 
-	WARN_ON_ONCE(iop && atomic_read(&iop->write_bytes_pending) != 0);
+	WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) != 0);
 
 	/*
 	 * Walk through the folio to find areas to write back. If we
@@ -1628,7 +1628,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	 * invalid, grab a new one.
 	 */
 	for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
-		if (iop && !test_bit(i, iop->uptodate))
+		if (ifs && !test_bit(i, ifs->state))
 			continue;
 
 		error = wpc->ops->map_blocks(wpc, inode, pos);
@@ -1639,7 +1639,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 			continue;
 		if (wpc->iomap.type == IOMAP_HOLE)
 			continue;
-		iomap_add_to_ioend(inode, pos, folio, iop, wpc, wbc,
+		iomap_add_to_ioend(inode, pos, folio, ifs, wpc, wbc,
 				 &submit_list);
 		count++;
 	}
-- 
2.40.1

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Christoph Hellwig]

On Sun, Jun 11, 2023 at 11:21:48PM -0700, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Actually, coming back to this:

-to_iomap_page(struct folio *folio)
+static inline struct iomap_folio_state *iomap_get_ifs

I find the to_* naming much more descriptive for derving the
private data.  get tends to imply grabbing a refcount.

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Ritesh Harjani]

Christoph Hellwig <hch@infradead.org> writes:

> On Sun, Jun 11, 2023 at 11:21:48PM -0700, Christoph Hellwig wrote:
>> Looks good:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

>
> Actually, coming back to this:
>
> -to_iomap_page(struct folio *folio)
> +static inline struct iomap_folio_state *iomap_get_ifs
>
> I find the to_* naming much more descriptive for derving the
> private data.  get tends to imply grabbing a refcount.

Not always. That would be iomap_ifs_get(ifs)/iomap_ifs_put(ifs).

See this -

static inline void *folio_get_private(struct folio *folio)
{
	return folio->private;
}

So, is it ok if we hear from others too on iomap_get_ifs() function
name?

-ritesh

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Darrick J. Wong]

On Mon, Jun 12, 2023 at 02:49:50PM +0530, Ritesh Harjani wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Sun, Jun 11, 2023 at 11:21:48PM -0700, Christoph Hellwig wrote:
> >> Looks good:
> >>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Thanks!
> 
> >
> > Actually, coming back to this:
> >
> > -to_iomap_page(struct folio *folio)
> > +static inline struct iomap_folio_state *iomap_get_ifs
> >
> > I find the to_* naming much more descriptive for derving the
> > private data.  get tends to imply grabbing a refcount.
> 
> Not always. That would be iomap_ifs_get(ifs)/iomap_ifs_put(ifs).
> 
> See this -
> 
> static inline void *folio_get_private(struct folio *folio)
> {
> 	return folio->private;
> }
> 
> So, is it ok if we hear from others too on iomap_get_ifs() function
> name?

It's a static inline, no need for namespacing in the name.  And hch is
right, _get/_put often imply receiving and returning some active
refcount.  That (IMO) makes folio_get_private the odd one out, since
pages don't refcount the private pointer.

I think of this more as a C(rap)-style type conversion function for a
generic object that can get touched by many subsystems (and hence we
cannot do the embed-parent-in-child-object thing).

So.

static inline struct iomap_folio_state *
to_folio_state(struct folio *folio)
{
	return folio->private;
}

is fine with me.  Can we go with this, and not make Ritesh run around
renaming and rebasing beyond v10?

[08:02] <willy> honestly, I think even having the abstraction was a
mistake.  just use folio->private

--D

> -ritesh

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Matthew Wilcox]

On Mon, Jun 12, 2023 at 08:05:20AM -0700, Darrick J. Wong wrote:
> static inline struct iomap_folio_state *
> to_folio_state(struct folio *folio)
> {
> 	return folio->private;
> }

I'd reformat it to not-XFS-style:

static inline struct iomap_folio_state *to_folio_state(struct folio *folio)
{
	return folio->private;
}

But other than that, I approve.  Unless you just want to do without it
entirely and do

	struct iomap_folio_state *state = folio->private;

instead of

	struct iomap_folio_state *state = to_folio_state(folio);

I'm having a hard time caring between the two.

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Darrick J. Wong]

On Mon, Jun 12, 2023 at 04:08:51PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 12, 2023 at 08:05:20AM -0700, Darrick J. Wong wrote:
> > static inline struct iomap_folio_state *
> > to_folio_state(struct folio *folio)
> > {
> > 	return folio->private;
> > }
> 
> I'd reformat it to not-XFS-style:
> 
> static inline struct iomap_folio_state *to_folio_state(struct folio *folio)
> {
> 	return folio->private;
> }
> 
> But other than that, I approve.  Unless you just want to do without it
> entirely and do
> 
> 	struct iomap_folio_state *state = folio->private;
> 
> instead of
> 
> 	struct iomap_folio_state *state = to_folio_state(folio);
> 
> I'm having a hard time caring between the two.

Either's fine with me too.  I'm getting tired of reading this series
over and over again.

Ritesh: Please pick whichever variant you like, and that's it, no more
discussion.

--D

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Ritesh Harjani]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Mon, Jun 12, 2023 at 04:08:51PM +0100, Matthew Wilcox wrote:
>> On Mon, Jun 12, 2023 at 08:05:20AM -0700, Darrick J. Wong wrote:
>> > static inline struct iomap_folio_state *
>> > to_folio_state(struct folio *folio)
>> > {
>> > 	return folio->private;
>> > }
>> 
>> I'd reformat it to not-XFS-style:
>> 
>> static inline struct iomap_folio_state *to_folio_state(struct folio *folio)
>> {
>> 	return folio->private;
>> }
>> 
>> But other than that, I approve.  Unless you just want to do without it
>> entirely and do
>> 
>> 	struct iomap_folio_state *state = folio->private;
>> 
>> instead of
>> 
>> 	struct iomap_folio_state *state = to_folio_state(folio);
>> 
>> I'm having a hard time caring between the two.
>
> Either's fine with me too.  I'm getting tired of reading this series
> over and over again.
>
> Ritesh: Please pick whichever variant you like, and that's it, no more
> discussion.

static inline struct iomap_folio_state *to_folio_state(struct folio *folio)
{
    return folio->private;
}

Sure this looks fine to me. So, I am hoping that there is no need to check
folio_test_private(folio) PG_private flag here before returning
folio->private (which was the case in original code to_iomap_page())

I did take a cursory look and didn't find any reason to test for doing
folio_test_private(folio) here. It should always remain set between
iomap_ifs_alloc() and iomap_ifs_free().

- IIUC, it is mostly for MM subsystem to see whether there is a
private FS data attached to a folio for which they think we might have
to call FS callback. for e.g. .is_dirty_writeback callback.
- Or like FS can use it within it's own subsystem to say whether a
folio is being associated with an in-progress read or write request. (e.g. NFS?)


-ritesh

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Matthew Wilcox]

On Mon, Jun 12, 2023 at 11:13:42PM +0530, Ritesh Harjani wrote:
> "Darrick J. Wong" <djwong@kernel.org> writes:
> > Ritesh: Please pick whichever variant you like, and that's it, no more
> > discussion.
> 
> static inline struct iomap_folio_state *to_folio_state(struct folio *folio)
> {
>     return folio->private;
> }
> 
> Sure this looks fine to me. So, I am hoping that there is no need to check
> folio_test_private(folio) PG_private flag here before returning
> folio->private (which was the case in original code to_iomap_page())
> 
> I did take a cursory look and didn't find any reason to test for doing
> folio_test_private(folio) here. It should always remain set between
> iomap_ifs_alloc() and iomap_ifs_free().
> 
> - IIUC, it is mostly for MM subsystem to see whether there is a
> private FS data attached to a folio for which they think we might have
> to call FS callback. for e.g. .is_dirty_writeback callback.
> - Or like FS can use it within it's own subsystem to say whether a
> folio is being associated with an in-progress read or write request. (e.g. NFS?)

The PG_private flag is being obsoleted in favour of just testing
folio->private for being NULL.  It's still in widespread use, but I'm
removing it as I change code.  I think we'll probably get rid of PG_error
first, but the more page flags we free up the better.  PG_hwpoison is
also scheduled for removal, but that requires folios to be dynamically
allocated first, so freeing that bit probably comes third.

Re: [PATCHv9 1/6] iomap: Rename iomap_page to iomap_folio_state and others

[Christoph Hellwig]

On Mon, Jun 12, 2023 at 04:08:51PM +0100, Matthew Wilcox wrote:
> But other than that, I approve.  Unless you just want to do without it
> entirely and do
> 
> 	struct iomap_folio_state *state = folio->private;
> 
> instead of
> 
> 	struct iomap_folio_state *state = to_folio_state(folio);
> 
> I'm having a hard time caring between the two.

The direct dereference is fine to me as well and bypasses the naming
discussion entirely, which is always a plus.

I didn't notice the folio had switched to a void pointer for the private
data vs the always somewhat odd unsigned long in the page, making this
possibly and preferred by me.

[PATCHv9 2/6] iomap: Drop ifs argument from iomap_set_range_uptodate()

[Ritesh Harjani (IBM)]

iomap_folio_state (ifs) can be derived directly from the folio, making it
unnecessary to pass "ifs" as an argument to iomap_set_range_uptodate().
This patch eliminates "ifs" argument from iomap_set_range_uptodate() function.

Also, the definition of iomap_set_range_uptodate() and
iomap_ifs_set_range_uptodate() functions are moved above iomap_ifs_alloc().
In upcoming patches, we plan to introduce additional helper routines for
handling dirty state, with the intention of consolidating all of "ifs" state
handling routines at one place.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/buffered-io.c | 67 +++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 779205fe228f..e237f2b786bc 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -43,6 +43,33 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
 
 static struct bio_set iomap_ioend_bioset;
 
+static void iomap_ifs_set_range_uptodate(struct folio *folio,
+		struct iomap_folio_state *ifs, size_t off, size_t len)
+{
+	struct inode *inode = folio->mapping->host;
+	unsigned int first_blk = off >> inode->i_blkbits;
+	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
+	unsigned int nr_blks = last_blk - first_blk + 1;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ifs->state_lock, flags);
+	bitmap_set(ifs->state, first_blk, nr_blks);
+	if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
+		folio_mark_uptodate(folio);
+	spin_unlock_irqrestore(&ifs->state_lock, flags);
+}
+
+static void iomap_set_range_uptodate(struct folio *folio, size_t off,
+		size_t len)
+{
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
+
+	if (ifs)
+		iomap_ifs_set_range_uptodate(folio, ifs, off, len);
+	else
+		folio_mark_uptodate(folio);
+}
+
 static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
 		struct folio *folio, unsigned int flags)
 {
@@ -144,30 +171,6 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 	*lenp = plen;
 }
 
-static void iomap_ifs_set_range_uptodate(struct folio *folio,
-		struct iomap_folio_state *ifs, size_t off, size_t len)
-{
-	struct inode *inode = folio->mapping->host;
-	unsigned first = off >> inode->i_blkbits;
-	unsigned last = (off + len - 1) >> inode->i_blkbits;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ifs->state_lock, flags);
-	bitmap_set(ifs->state, first, last - first + 1);
-	if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
-		folio_mark_uptodate(folio);
-	spin_unlock_irqrestore(&ifs->state_lock, flags);
-}
-
-static void iomap_set_range_uptodate(struct folio *folio,
-		struct iomap_folio_state *ifs, size_t off, size_t len)
-{
-	if (ifs)
-		iomap_ifs_set_range_uptodate(folio, ifs, off, len);
-	else
-		folio_mark_uptodate(folio);
-}
-
 static void iomap_finish_folio_read(struct folio *folio, size_t offset,
 		size_t len, int error)
 {
@@ -177,7 +180,7 @@ static void iomap_finish_folio_read(struct folio *folio, size_t offset,
 		folio_clear_uptodate(folio);
 		folio_set_error(folio);
 	} else {
-		iomap_set_range_uptodate(folio, ifs, offset, len);
+		iomap_set_range_uptodate(folio, offset, len);
 	}
 
 	if (!ifs || atomic_sub_and_test(len, &ifs->read_bytes_pending))
@@ -213,7 +216,6 @@ struct iomap_readpage_ctx {
 static int iomap_read_inline_data(const struct iomap_iter *iter,
 		struct folio *folio)
 {
-	struct iomap_folio_state *ifs;
 	const struct iomap *iomap = iomap_iter_srcmap(iter);
 	size_t size = i_size_read(iter->inode) - iomap->offset;
 	size_t poff = offset_in_page(iomap->offset);
@@ -231,15 +233,13 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
 	if (offset > 0)
-		ifs = iomap_ifs_alloc(iter->inode, folio, iter->flags);
-	else
-		ifs = iomap_get_ifs(folio);
+		iomap_ifs_alloc(iter->inode, folio, iter->flags);
 
 	addr = kmap_local_folio(folio, offset);
 	memcpy(addr, iomap->inline_data, size);
 	memset(addr + size, 0, PAGE_SIZE - poff - size);
 	kunmap_local(addr);
-	iomap_set_range_uptodate(folio, ifs, offset, PAGE_SIZE - poff);
+	iomap_set_range_uptodate(folio, offset, PAGE_SIZE - poff);
 	return 0;
 }
 
@@ -276,7 +276,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
 
 	if (iomap_block_needs_zeroing(iter, pos)) {
 		folio_zero_range(folio, poff, plen);
-		iomap_set_range_uptodate(folio, ifs, poff, plen);
+		iomap_set_range_uptodate(folio, poff, plen);
 		goto done;
 	}
 
@@ -589,7 +589,7 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 			if (status)
 				return status;
 		}
-		iomap_set_range_uptodate(folio, ifs, poff, plen);
+		iomap_set_range_uptodate(folio, poff, plen);
 	} while ((block_start += plen) < block_end);
 
 	return 0;
@@ -696,7 +696,6 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
 static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
 		size_t copied, struct folio *folio)
 {
-	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 	flush_dcache_folio(folio);
 
 	/*
@@ -712,7 +711,7 @@ static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
 	 */
 	if (unlikely(copied < len && !folio_test_uptodate(folio)))
 		return 0;
-	iomap_set_range_uptodate(folio, ifs, offset_in_folio(folio, pos), len);
+	iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len);
 	filemap_dirty_folio(inode->i_mapping, folio);
 	return copied;
 }
-- 
2.40.1

Re: [PATCHv9 2/6] iomap: Drop ifs argument from iomap_set_range_uptodate()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani (IBM)]

This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
and iomap_ifs_is_block_uptodate() for managing uptodate state of
ifs state bitmap.

In later patches ifs state bitmap array will also handle dirty state of all
blocks of a folio. Hence this patch adds some helper routines for handling
uptodate state of the ifs state bitmap.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index e237f2b786bc..206808f6e818 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
 
 static struct bio_set iomap_ioend_bioset;
 
+static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
+					       struct iomap_folio_state *ifs)
+{
+	struct inode *inode = folio->mapping->host;
+
+	return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
+}
+
+static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
+					       unsigned int block)
+{
+	return test_bit(block, ifs->state);
+}
+
 static void iomap_ifs_set_range_uptodate(struct folio *folio,
 		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
@@ -54,7 +68,7 @@ static void iomap_ifs_set_range_uptodate(struct folio *folio,
 
 	spin_lock_irqsave(&ifs->state_lock, flags);
 	bitmap_set(ifs->state, first_blk, nr_blks);
-	if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
+	if (iomap_ifs_is_fully_uptodate(folio, ifs))
 		folio_mark_uptodate(folio);
 	spin_unlock_irqrestore(&ifs->state_lock, flags);
 }
@@ -99,14 +113,12 @@ static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
 static void iomap_ifs_free(struct folio *folio)
 {
 	struct iomap_folio_state *ifs = folio_detach_private(folio);
-	struct inode *inode = folio->mapping->host;
-	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
 
 	if (!ifs)
 		return;
 	WARN_ON_ONCE(atomic_read(&ifs->read_bytes_pending));
 	WARN_ON_ONCE(atomic_read(&ifs->write_bytes_pending));
-	WARN_ON_ONCE(bitmap_full(ifs->state, nr_blocks) !=
+	WARN_ON_ONCE(iomap_ifs_is_fully_uptodate(folio, ifs) !=
 			folio_test_uptodate(folio));
 	kfree(ifs);
 }
@@ -137,7 +149,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 
 		/* move forward for each leading block marked uptodate */
 		for (i = first; i <= last; i++) {
-			if (!test_bit(i, ifs->state))
+			if (!iomap_ifs_is_block_uptodate(ifs, i))
 				break;
 			*pos += block_size;
 			poff += block_size;
@@ -147,7 +159,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 
 		/* truncate len if we find any trailing uptodate block(s) */
 		for ( ; i <= last; i++) {
-			if (test_bit(i, ifs->state)) {
+			if (iomap_ifs_is_block_uptodate(ifs, i)) {
 				plen -= (last - i + 1) * block_size;
 				last = i - 1;
 				break;
@@ -451,7 +463,7 @@ bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
 	last = (from + count - 1) >> inode->i_blkbits;
 
 	for (i = first; i <= last; i++)
-		if (!test_bit(i, ifs->state))
+		if (!iomap_ifs_is_block_uptodate(ifs, i))
 			return false;
 	return true;
 }
@@ -1627,7 +1639,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	 * invalid, grab a new one.
 	 */
 	for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
-		if (ifs && !test_bit(i, ifs->state))
+		if (ifs && !iomap_ifs_is_block_uptodate(ifs, i))
 			continue;
 
 		error = wpc->ops->map_blocks(wpc, inode, pos);
-- 
2.40.1

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Christoph Hellwig]

On Sat, Jun 10, 2023 at 05:09:04PM +0530, Ritesh Harjani (IBM) wrote:
> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
> and iomap_ifs_is_block_uptodate() for managing uptodate state of
> ifs state bitmap.
> 
> In later patches ifs state bitmap array will also handle dirty state of all
> blocks of a folio. Hence this patch adds some helper routines for handling
> uptodate state of the ifs state bitmap.
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index e237f2b786bc..206808f6e818 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>  
>  static struct bio_set iomap_ioend_bioset;
>  
> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
> +					       struct iomap_folio_state *ifs)
> +{
> +	struct inode *inode = folio->mapping->host;
> +
> +	return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
> +}
> +
> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> +					       unsigned int block)
> +{
> +	return test_bit(block, ifs->state);
> +}

A little nitpicky, but do the _ifs_ name compenents here really add
value?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani]

Christoph Hellwig <hch@infradead.org> writes:

> On Sat, Jun 10, 2023 at 05:09:04PM +0530, Ritesh Harjani (IBM) wrote:
>> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
>> and iomap_ifs_is_block_uptodate() for managing uptodate state of
>> ifs state bitmap.
>>
>> In later patches ifs state bitmap array will also handle dirty state of all
>> blocks of a folio. Hence this patch adds some helper routines for handling
>> uptodate state of the ifs state bitmap.
>>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> index e237f2b786bc..206808f6e818 100644
>> --- a/fs/iomap/buffered-io.c
>> +++ b/fs/iomap/buffered-io.c
>> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>>
>>  static struct bio_set iomap_ioend_bioset;
>>
>> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
>> +					       struct iomap_folio_state *ifs)
>> +{
>> +	struct inode *inode = folio->mapping->host;
>> +
>> +	return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
>> +}
>> +
>> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
>> +					       unsigned int block)
>> +{
>> +	return test_bit(block, ifs->state);
>> +}

static inline bool iomap_ifs_is_block_dirty(struct folio *folio,
		struct iomap_folio_state *ifs, int block)
{
	struct inode *inode = folio->mapping->host;
	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);

	return test_bit(block + blks_per_folio, ifs->state);
}

>
> A little nitpicky, but do the _ifs_ name compenents here really add
> value?
>

Maybe if you look at both of above functions together to see the value?

> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Andreas Gruenbacher]

On Mon, Jun 12, 2023 at 8:25 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Sat, Jun 10, 2023 at 05:09:04PM +0530, Ritesh Harjani (IBM) wrote:
> > This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
> > and iomap_ifs_is_block_uptodate() for managing uptodate state of
> > ifs state bitmap.
> >
> > In later patches ifs state bitmap array will also handle dirty state of all
> > blocks of a folio. Hence this patch adds some helper routines for handling
> > uptodate state of the ifs state bitmap.
> >
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > ---
> >  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index e237f2b786bc..206808f6e818 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
> >
> >  static struct bio_set iomap_ioend_bioset;
> >
> > +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
> > +                                            struct iomap_folio_state *ifs)
> > +{
> > +     struct inode *inode = folio->mapping->host;
> > +
> > +     return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
> > +}
> > +
> > +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> > +                                            unsigned int block)
> > +{
> > +     return test_bit(block, ifs->state);

"block_is_uptodate" instead of "is_block_uptodate" here as well, please.

Also see by previous mail about iomap_ifs_is_block_uptodate().

> > +}
>
> A little nitpicky, but do the _ifs_ name compenents here really add
> value?

Since we're at the nitpicking, I don't find those names very useful,
either. How about the following instead?

iomap_ifs_alloc -> iomap_folio_state_alloc
iomap_ifs_free -> iomap_folio_state_free
iomap_ifs_calc_range -> iomap_folio_state_calc_range

iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
iomap_ifs_is_block_dirty -> iomap_block_is_dirty

iomap_ifs_set_range_uptodate -> __iomap_set_range_uptodate
iomap_ifs_clear_range_dirty -> __iomap_clear_range_dirty
iomap_ifs_set_range_dirty -> __iomap_set_range_dirty

Thanks,
Andreas

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani]

Andreas Gruenbacher <agruenba@redhat.com> writes:

> On Mon, Jun 12, 2023 at 8:25 AM Christoph Hellwig <hch@infradead.org> wrote:
>> On Sat, Jun 10, 2023 at 05:09:04PM +0530, Ritesh Harjani (IBM) wrote:
>> > This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
>> > and iomap_ifs_is_block_uptodate() for managing uptodate state of
>> > ifs state bitmap.
>> >
>> > In later patches ifs state bitmap array will also handle dirty state of all
>> > blocks of a folio. Hence this patch adds some helper routines for handling
>> > uptodate state of the ifs state bitmap.
>> >
>> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> > ---
>> >  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>> >  1 file changed, 20 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> > index e237f2b786bc..206808f6e818 100644
>> > --- a/fs/iomap/buffered-io.c
>> > +++ b/fs/iomap/buffered-io.c
>> > @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>> >
>> >  static struct bio_set iomap_ioend_bioset;
>> >
>> > +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
>> > +                                            struct iomap_folio_state *ifs)
>> > +{
>> > +     struct inode *inode = folio->mapping->host;
>> > +
>> > +     return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
>> > +}
>> > +
>> > +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
>> > +                                            unsigned int block)
>> > +{
>> > +     return test_bit(block, ifs->state);
>
> "block_is_uptodate" instead of "is_block_uptodate" here as well, please.
>
> Also see by previous mail about iomap_ifs_is_block_uptodate().
>

"is_block_uptodate" is what we decided in our previous discussion here [1]
[1]: https://lore.kernel.org/linux-xfs/20230605225434.GF1325469@frogsfrogsfrogs/


Unless any strong objections, for now I will keep Maintainer's suggested name
;) and wait for his feedback on this.

>> > +}
>>
>> A little nitpicky, but do the _ifs_ name compenents here really add
>> value?
>
> Since we're at the nitpicking, I don't find those names very useful,
> either. How about the following instead?
>
> iomap_ifs_alloc -> iomap_folio_state_alloc
> iomap_ifs_free -> iomap_folio_state_free
> iomap_ifs_calc_range -> iomap_folio_state_calc_range

First of all I think we need to get used to the name "ifs" like how we
were using "iop" earlier. ifs == iomap_folio_state...

>
> iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
> iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
> iomap_ifs_is_block_dirty -> iomap_block_is_dirty
>

...if you then look above functions with _ifs_ == _iomap_folio_state_
naming. It will make more sense. 


> iomap_ifs_set_range_uptodate -> __iomap_set_range_uptodate
> iomap_ifs_clear_range_dirty -> __iomap_clear_range_dirty
> iomap_ifs_set_range_dirty -> __iomap_set_range_dirty

Same as above.

>
> Thanks,
> Andreas

Thanks for the review! 

I am not saying I am not open to changing the name. But AFAIR, Darrick
and Matthew were ok with "ifs" naming. In fact they first suggested it
as well. So if others also dislike "ifs" naming, then we could consider
other options.

-ritesh

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Matthew Wilcox]

On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
> > Since we're at the nitpicking, I don't find those names very useful,
> > either. How about the following instead?
> >
> > iomap_ifs_alloc -> iomap_folio_state_alloc
> > iomap_ifs_free -> iomap_folio_state_free
> > iomap_ifs_calc_range -> iomap_folio_state_calc_range
> 
> First of all I think we need to get used to the name "ifs" like how we
> were using "iop" earlier. ifs == iomap_folio_state...
> 
> >
> > iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
> > iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
> > iomap_ifs_is_block_dirty -> iomap_block_is_dirty
> >
> 
> ...if you then look above functions with _ifs_ == _iomap_folio_state_
> naming. It will make more sense. 

Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.
I don't think there's any need to namespace this so fully.
ifs_is_fully_uptodate() is just fine for a static function, IMO.

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
>> > Since we're at the nitpicking, I don't find those names very useful,
>> > either. How about the following instead?
>> >
>> > iomap_ifs_alloc -> iomap_folio_state_alloc
>> > iomap_ifs_free -> iomap_folio_state_free
>> > iomap_ifs_calc_range -> iomap_folio_state_calc_range
>>
>> First of all I think we need to get used to the name "ifs" like how we
>> were using "iop" earlier. ifs == iomap_folio_state...
>>
>> >
>> > iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
>> > iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
>> > iomap_ifs_is_block_dirty -> iomap_block_is_dirty
>> >
>>
>> ...if you then look above functions with _ifs_ == _iomap_folio_state_
>> naming. It will make more sense.
>
> Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.

:P 

> I don't think there's any need to namespace this so fully.
> ifs_is_fully_uptodate() is just fine for a static function, IMO.

Ohh, we went that road but were shot down. That time the naming was
iop_is_fully_uptodate(). :(

-ritesh

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Andreas Gruenbacher]

On Mon, Jun 12, 2023 at 5:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
> > > Since we're at the nitpicking, I don't find those names very useful,
> > > either. How about the following instead?
> > >
> > > iomap_ifs_alloc -> iomap_folio_state_alloc
> > > iomap_ifs_free -> iomap_folio_state_free
> > > iomap_ifs_calc_range -> iomap_folio_state_calc_range
> >
> > First of all I think we need to get used to the name "ifs" like how we
> > were using "iop" earlier. ifs == iomap_folio_state...
> >
> > >
> > > iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
> > > iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
> > > iomap_ifs_is_block_dirty -> iomap_block_is_dirty
> > >
> >
> > ...if you then look above functions with _ifs_ == _iomap_folio_state_
> > naming. It will make more sense.
>
> Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.

Exactly.

> I don't think there's any need to namespace this so fully.
> ifs_is_fully_uptodate() is just fine for a static function, IMO.

I'd be perfectly happy with that kind of naming scheme as well.

Thanks,
Andreas

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Darrick J. Wong]

On Mon, Jun 12, 2023 at 05:57:48PM +0200, Andreas Gruenbacher wrote:
> On Mon, Jun 12, 2023 at 5:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> > On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
> > > > Since we're at the nitpicking, I don't find those names very useful,
> > > > either. How about the following instead?
> > > >
> > > > iomap_ifs_alloc -> iomap_folio_state_alloc
> > > > iomap_ifs_free -> iomap_folio_state_free
> > > > iomap_ifs_calc_range -> iomap_folio_state_calc_range
> > >
> > > First of all I think we need to get used to the name "ifs" like how we
> > > were using "iop" earlier. ifs == iomap_folio_state...
> > >
> > > >
> > > > iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
> > > > iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
> > > > iomap_ifs_is_block_dirty -> iomap_block_is_dirty
> > > >
> > >
> > > ...if you then look above functions with _ifs_ == _iomap_folio_state_
> > > naming. It will make more sense.
> >
> > Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.
> 
> Exactly.
> 
> > I don't think there's any need to namespace this so fully.
> > ifs_is_fully_uptodate() is just fine for a static function, IMO.
> 
> I'd be perfectly happy with that kind of naming scheme as well.

Ugh, /another/ round of renaming.

to_folio_state (or just folio->private)

ifs_alloc
ifs_free
ifs_calc_range

ifs_set_range_uptodate
ifs_is_fully_uptodate
ifs_block_is_uptodate

ifs_block_is_dirty
ifs_clear_range_dirty
ifs_set_range_dirty

No more renaming suggestions.  I've reached the point where my eyes and
brain have both glazed over from repeated re-reads of this series such
that I don't see the *bugs* anymore.

Anyone else wanting new naming gets to *send in their own patch*.
Please focus only on finding code defects or friction between iomap and
some other subsystem.

Flame away about my aggressive tone,

~Your burned out and pissed off maintainer~

> Thanks,
> Andreas
> 

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Mon, Jun 12, 2023 at 05:57:48PM +0200, Andreas Gruenbacher wrote:
>> On Mon, Jun 12, 2023 at 5:24 PM Matthew Wilcox <willy@infradead.org> wrote:
>> > On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
>> > > > Since we're at the nitpicking, I don't find those names very useful,
>> > > > either. How about the following instead?
>> > > >
>> > > > iomap_ifs_alloc -> iomap_folio_state_alloc
>> > > > iomap_ifs_free -> iomap_folio_state_free
>> > > > iomap_ifs_calc_range -> iomap_folio_state_calc_range
>> > >
>> > > First of all I think we need to get used to the name "ifs" like how we
>> > > were using "iop" earlier. ifs == iomap_folio_state...
>> > >
>> > > >
>> > > > iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
>> > > > iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
>> > > > iomap_ifs_is_block_dirty -> iomap_block_is_dirty
>> > > >
>> > >
>> > > ...if you then look above functions with _ifs_ == _iomap_folio_state_
>> > > naming. It will make more sense.
>> >
>> > Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.
>> 
>> Exactly.
>> 
>> > I don't think there's any need to namespace this so fully.
>> > ifs_is_fully_uptodate() is just fine for a static function, IMO.
>> 
>> I'd be perfectly happy with that kind of naming scheme as well.
>
> Ugh, /another/ round of renaming.
>
> to_folio_state (or just folio->private)
>
> ifs_alloc
> ifs_free
> ifs_calc_range
>
> ifs_set_range_uptodate
> ifs_is_fully_uptodate
> ifs_block_is_uptodate
>
> ifs_block_is_dirty
> ifs_clear_range_dirty
> ifs_set_range_dirty
>

Oops you have put me into a tough spot here. 
We came back from iop_** functions naming to iomap_iop_** to
iomap_ifs_**.

Christoph? Is it ok if we go back to ifs_** functions here then? 

Or do others prefer 
iomap_folio_state_** namings. instead of ifs_**  or iomap_ifs_**? 


> No more renaming suggestions.  I've reached the point where my eyes and
> brain have both glazed over from repeated re-reads of this series such
> that I don't see the *bugs* anymore.
>
> Anyone else wanting new naming gets to *send in their own patch*.
> Please focus only on finding code defects or friction between iomap and
> some other subsystem.

Yes, it would be helpful if we uncover any bugs/ or even suggstions for
how can we better test this (adding/improving any given test in xfstests).

I have been using xfstests mainly on x86 with 1k and Power with 4k "-g auto".
I will make sure I run some more configs before sending the next
revision. 

>
> Flame away about my aggressive tone,

Thanks Darrick. No issues at all. 

>
> ~Your burned out and pissed off maintainer~
>
>> Thanks,
>> Andreas
>> 

-ritesh

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Andreas Gruenbacher]

On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
<ritesh.list@gmail.com> wrote:
> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
> and iomap_ifs_is_block_uptodate() for managing uptodate state of
> ifs state bitmap.
>
> In later patches ifs state bitmap array will also handle dirty state of all
> blocks of a folio. Hence this patch adds some helper routines for handling
> uptodate state of the ifs state bitmap.
>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index e237f2b786bc..206808f6e818 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>
>  static struct bio_set iomap_ioend_bioset;
>
> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
> +                                              struct iomap_folio_state *ifs)
> +{
> +       struct inode *inode = folio->mapping->host;
> +
> +       return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));

This should be written as something like:

unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
blks_per_folio);

> +}
> +
> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> +                                              unsigned int block)
> +{
> +       return test_bit(block, ifs->state);

This function should be called iomap_ifs_block_is_uptodate(), and
probably be written as follows, passing in the folio as well (this
will optimize out, anyway):

struct inode *inode = folio->mapping->host;
unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);

> +}
> +
>  static void iomap_ifs_set_range_uptodate(struct folio *folio,
>                 struct iomap_folio_state *ifs, size_t off, size_t len)
>  {
> @@ -54,7 +68,7 @@ static void iomap_ifs_set_range_uptodate(struct folio *folio,
>
>         spin_lock_irqsave(&ifs->state_lock, flags);
>         bitmap_set(ifs->state, first_blk, nr_blks);
> -       if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
> +       if (iomap_ifs_is_fully_uptodate(folio, ifs))
>                 folio_mark_uptodate(folio);
>         spin_unlock_irqrestore(&ifs->state_lock, flags);
>  }
> @@ -99,14 +113,12 @@ static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
>  static void iomap_ifs_free(struct folio *folio)
>  {
>         struct iomap_folio_state *ifs = folio_detach_private(folio);
> -       struct inode *inode = folio->mapping->host;
> -       unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
>
>         if (!ifs)
>                 return;
>         WARN_ON_ONCE(atomic_read(&ifs->read_bytes_pending));
>         WARN_ON_ONCE(atomic_read(&ifs->write_bytes_pending));
> -       WARN_ON_ONCE(bitmap_full(ifs->state, nr_blocks) !=
> +       WARN_ON_ONCE(iomap_ifs_is_fully_uptodate(folio, ifs) !=
>                         folio_test_uptodate(folio));
>         kfree(ifs);
>  }
> @@ -137,7 +149,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
>
>                 /* move forward for each leading block marked uptodate */
>                 for (i = first; i <= last; i++) {
> -                       if (!test_bit(i, ifs->state))
> +                       if (!iomap_ifs_is_block_uptodate(ifs, i))
>                                 break;
>                         *pos += block_size;
>                         poff += block_size;
> @@ -147,7 +159,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
>
>                 /* truncate len if we find any trailing uptodate block(s) */
>                 for ( ; i <= last; i++) {
> -                       if (test_bit(i, ifs->state)) {
> +                       if (iomap_ifs_is_block_uptodate(ifs, i)) {
>                                 plen -= (last - i + 1) * block_size;
>                                 last = i - 1;
>                                 break;
> @@ -451,7 +463,7 @@ bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
>         last = (from + count - 1) >> inode->i_blkbits;
>
>         for (i = first; i <= last; i++)
> -               if (!test_bit(i, ifs->state))
> +               if (!iomap_ifs_is_block_uptodate(ifs, i))
>                         return false;
>         return true;
>  }
> @@ -1627,7 +1639,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>          * invalid, grab a new one.
>          */
>         for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
> -               if (ifs && !test_bit(i, ifs->state))
> +               if (ifs && !iomap_ifs_is_block_uptodate(ifs, i))
>                         continue;
>
>                 error = wpc->ops->map_blocks(wpc, inode, pos);
> --
> 2.40.1
>

Thanks,
Andreas

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani]

Andreas Gruenbacher <agruenba@redhat.com> writes:

> On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> <ritesh.list@gmail.com> wrote:
>> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
>> and iomap_ifs_is_block_uptodate() for managing uptodate state of
>> ifs state bitmap.
>>
>> In later patches ifs state bitmap array will also handle dirty state of all
>> blocks of a folio. Hence this patch adds some helper routines for handling
>> uptodate state of the ifs state bitmap.
>>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> index e237f2b786bc..206808f6e818 100644
>> --- a/fs/iomap/buffered-io.c
>> +++ b/fs/iomap/buffered-io.c
>> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>>
>>  static struct bio_set iomap_ioend_bioset;
>>
>> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
>> +                                              struct iomap_folio_state *ifs)
>> +{
>> +       struct inode *inode = folio->mapping->host;
>> +
>> +       return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
>
> This should be written as something like:
>
> unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> blks_per_folio);
>

Nah, I feel it is not required... It make sense when we have the same
function getting use for both "uptodate" and "dirty" state.
Here the function anyways operates on uptodate state.
Hence I feel it is not required.

>> +}
>> +
>> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
>> +                                              unsigned int block)
>> +{
>> +       return test_bit(block, ifs->state);
>
> This function should be called iomap_ifs_block_is_uptodate(), and
> probably be written as follows, passing in the folio as well (this
> will optimize out, anyway):
>
> struct inode *inode = folio->mapping->host;
> unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
>

Same here.

-ritesh

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Andreas Grünbacher]

Am Mo., 12. Juni 2023 um 17:43 Uhr schrieb Ritesh Harjani
<ritesh.list@gmail.com>:
> Andreas Gruenbacher <agruenba@redhat.com> writes:
> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > <ritesh.list@gmail.com> wrote:
> >> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
> >> and iomap_ifs_is_block_uptodate() for managing uptodate state of
> >> ifs state bitmap.
> >>
> >> In later patches ifs state bitmap array will also handle dirty state of all
> >> blocks of a folio. Hence this patch adds some helper routines for handling
> >> uptodate state of the ifs state bitmap.
> >>
> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> >> ---
> >>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
> >>  1 file changed, 20 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> >> index e237f2b786bc..206808f6e818 100644
> >> --- a/fs/iomap/buffered-io.c
> >> +++ b/fs/iomap/buffered-io.c
> >> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
> >>
> >>  static struct bio_set iomap_ioend_bioset;
> >>
> >> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
> >> +                                              struct iomap_folio_state *ifs)
> >> +{
> >> +       struct inode *inode = folio->mapping->host;
> >> +
> >> +       return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
> >
> > This should be written as something like:
> >
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> > blks_per_folio);
> >
>
> Nah, I feel it is not required... It make sense when we have the same
> function getting use for both "uptodate" and "dirty" state.
> Here the function anyways operates on uptodate state.
> Hence I feel it is not required.

So we have this iomap_block_state enum now, but IOMAP_ST_UPTODATE must
be 0 or else the code will break. That's worse than not having this
abstraction in the first place because.

Andreas

> >> +}
> >> +
> >> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> >> +                                              unsigned int block)
> >> +{
> >> +       return test_bit(block, ifs->state);
> >
> > This function should be called iomap_ifs_block_is_uptodate(), and
> > probably be written as follows, passing in the folio as well (this
> > will optimize out, anyway):
> >
> > struct inode *inode = folio->mapping->host;
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
> >
>
> Same here.
>
> -ritesh

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Darrick J. Wong]

On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
> Andreas Gruenbacher <agruenba@redhat.com> writes:
> 
> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > <ritesh.list@gmail.com> wrote:
> >> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
> >> and iomap_ifs_is_block_uptodate() for managing uptodate state of
> >> ifs state bitmap.
> >>
> >> In later patches ifs state bitmap array will also handle dirty state of all
> >> blocks of a folio. Hence this patch adds some helper routines for handling
> >> uptodate state of the ifs state bitmap.
> >>
> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> >> ---
> >>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
> >>  1 file changed, 20 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> >> index e237f2b786bc..206808f6e818 100644
> >> --- a/fs/iomap/buffered-io.c
> >> +++ b/fs/iomap/buffered-io.c
> >> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
> >>
> >>  static struct bio_set iomap_ioend_bioset;
> >>
> >> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
> >> +                                              struct iomap_folio_state *ifs)
> >> +{
> >> +       struct inode *inode = folio->mapping->host;
> >> +
> >> +       return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
> >
> > This should be written as something like:
> >
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> > blks_per_folio);
> >
> 
> Nah, I feel it is not required... It make sense when we have the same
> function getting use for both "uptodate" and "dirty" state.
> Here the function anyways operates on uptodate state.
> Hence I feel it is not required.

Honestly I thought that enum-for-bits thing was excessive considering
that ifs has only two state bits.  But, since you included it, it
doesn't make much sense /not/ to use it here.

OTOH, if you disassemble the object code and discover that the compiler
*isn't* using constant propagation to simplify the object code, then
yes, that would be a good reason to get rid of it.

--D

> >> +}
> >> +
> >> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> >> +                                              unsigned int block)
> >> +{
> >> +       return test_bit(block, ifs->state);
> >
> > This function should be called iomap_ifs_block_is_uptodate(), and
> > probably be written as follows, passing in the folio as well (this
> > will optimize out, anyway):
> >
> > struct inode *inode = folio->mapping->host;
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
> >
> 
> Same here.
> 
> -ritesh

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Andreas Gruenbacher]

On Mon, Jun 12, 2023 at 6:16 PM Darrick J. Wong <djwong@kernel.org> wrote:
> On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
> > Andreas Gruenbacher <agruenba@redhat.com> writes:
> >
> > > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > > <ritesh.list@gmail.com> wrote:
> > >> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
> > >> and iomap_ifs_is_block_uptodate() for managing uptodate state of
> > >> ifs state bitmap.
> > >>
> > >> In later patches ifs state bitmap array will also handle dirty state of all
> > >> blocks of a folio. Hence this patch adds some helper routines for handling
> > >> uptodate state of the ifs state bitmap.
> > >>
> > >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > >> ---
> > >>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
> > >>  1 file changed, 20 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > >> index e237f2b786bc..206808f6e818 100644
> > >> --- a/fs/iomap/buffered-io.c
> > >> +++ b/fs/iomap/buffered-io.c
> > >> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
> > >>
> > >>  static struct bio_set iomap_ioend_bioset;
> > >>
> > >> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
> > >> +                                              struct iomap_folio_state *ifs)
> > >> +{
> > >> +       struct inode *inode = folio->mapping->host;
> > >> +
> > >> +       return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
> > >
> > > This should be written as something like:
> > >
> > > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> > > blks_per_folio);
> > >
> >
> > Nah, I feel it is not required... It make sense when we have the same
> > function getting use for both "uptodate" and "dirty" state.
> > Here the function anyways operates on uptodate state.
> > Hence I feel it is not required.
>
> Honestly I thought that enum-for-bits thing was excessive considering
> that ifs has only two state bits.  But, since you included it, it
> doesn't make much sense /not/ to use it here.
>
> OTOH, if you disassemble the object code and discover that the compiler
> *isn't* using constant propagation to simplify the object code, then
> yes, that would be a good reason to get rid of it.

I've checked on x86_64 earlier and there at least, the enum didn't
affect the produced code at all.

Andreas

> --D
>
> > >> +}
> > >> +
> > >> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> > >> +                                              unsigned int block)
> > >> +{
> > >> +       return test_bit(block, ifs->state);
> > >
> > > This function should be called iomap_ifs_block_is_uptodate(), and
> > > probably be written as follows, passing in the folio as well (this
> > > will optimize out, anyway):
> > >
> > > struct inode *inode = folio->mapping->host;
> > > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > > return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
> > >
> >
> > Same here.
> >
> > -ritesh
>

Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

[Ritesh Harjani]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
>> Andreas Gruenbacher <agruenba@redhat.com> writes:
>> 
>> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
>> > <ritesh.list@gmail.com> wrote:
>> >> This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
>> >> and iomap_ifs_is_block_uptodate() for managing uptodate state of
>> >> ifs state bitmap.
>> >>
>> >> In later patches ifs state bitmap array will also handle dirty state of all
>> >> blocks of a folio. Hence this patch adds some helper routines for handling
>> >> uptodate state of the ifs state bitmap.
>> >>
>> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> >> ---
>> >>  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>> >>  1 file changed, 20 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> >> index e237f2b786bc..206808f6e818 100644
>> >> --- a/fs/iomap/buffered-io.c
>> >> +++ b/fs/iomap/buffered-io.c
>> >> @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>> >>
>> >>  static struct bio_set iomap_ioend_bioset;
>> >>
>> >> +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
>> >> +                                              struct iomap_folio_state *ifs)
>> >> +{
>> >> +       struct inode *inode = folio->mapping->host;
>> >> +
>> >> +       return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
>> >
>> > This should be written as something like:
>> >
>> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
>> > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
>> > blks_per_folio);
>> >
>> 
>> Nah, I feel it is not required... It make sense when we have the same
>> function getting use for both "uptodate" and "dirty" state.
>> Here the function anyways operates on uptodate state.
>> Hence I feel it is not required.
>
> Honestly I thought that enum-for-bits thing was excessive considering
> that ifs has only two state bits.  But, since you included it, it
> doesn't make much sense /not/ to use it here.

Ok. Will make the changes.

>
> OTOH, if you disassemble the object code and discover that the compiler
> *isn't* using constant propagation to simplify the object code, then
> yes, that would be a good reason to get rid of it.

Sure, I will check that once too.


-ritesh

[PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Ritesh Harjani (IBM)]

This patch factors iomap_write_delalloc_punch() function out. This function
is resposible for actual punch out operation.
The reason for doing this is, to avoid deep indentation when we bring punch-out
of individual non-dirty blocks within a dirty folio in a later patch
(which adds per-block dirty status handling to iomap) to avoid delalloc block
leak.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/buffered-io.c | 54 ++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 206808f6e818..1261f26479af 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -888,6 +888,33 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
 }
 EXPORT_SYMBOL_GPL(iomap_file_buffered_write);
 
+static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
+		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
+		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
+{
+	int ret = 0;
+
+	if (!folio_test_dirty(folio))
+		return ret;
+
+	/* if dirty, punch up to offset */
+	if (start_byte > *punch_start_byte) {
+		ret = punch(inode, *punch_start_byte,
+				start_byte - *punch_start_byte);
+		if (ret)
+			goto out;
+	}
+	/*
+	 * Make sure the next punch start is correctly bound to
+	 * the end of this data range, not the end of the folio.
+	 */
+	*punch_start_byte = min_t(loff_t, end_byte,
+				  folio_next_index(folio) << PAGE_SHIFT);
+
+out:
+	return ret;
+}
+
 /*
  * Scan the data range passed to us for dirty page cache folios. If we find a
  * dirty folio, punch out the preceeding range and update the offset from which
@@ -911,6 +938,7 @@ static int iomap_write_delalloc_scan(struct inode *inode,
 {
 	while (start_byte < end_byte) {
 		struct folio	*folio;
+		int ret;
 
 		/* grab locked page */
 		folio = filemap_lock_folio(inode->i_mapping,
@@ -921,26 +949,12 @@ static int iomap_write_delalloc_scan(struct inode *inode,
 			continue;
 		}
 
-		/* if dirty, punch up to offset */
-		if (folio_test_dirty(folio)) {
-			if (start_byte > *punch_start_byte) {
-				int	error;
-
-				error = punch(inode, *punch_start_byte,
-						start_byte - *punch_start_byte);
-				if (error) {
-					folio_unlock(folio);
-					folio_put(folio);
-					return error;
-				}
-			}
-
-			/*
-			 * Make sure the next punch start is correctly bound to
-			 * the end of this data range, not the end of the folio.
-			 */
-			*punch_start_byte = min_t(loff_t, end_byte,
-					folio_next_index(folio) << PAGE_SHIFT);
+		ret = iomap_write_delalloc_punch(inode, folio, punch_start_byte,
+						 start_byte, end_byte, punch);
+		if (ret) {
+			folio_unlock(folio);
+			folio_put(folio);
+			return ret;
 		}
 
 		/* move offset to start of next folio in range */
-- 
2.40.1

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Christoph Hellwig]

On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
> This patch factors iomap_write_delalloc_punch() function out. This function
> is resposible for actual punch out operation.
> The reason for doing this is, to avoid deep indentation when we bring punch-out
> of individual non-dirty blocks within a dirty folio in a later patch
> (which adds per-block dirty status handling to iomap) to avoid delalloc block
> leak.

A bunch of overly long lines in the commit message here,
but otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Ritesh Harjani]

Christoph Hellwig <hch@infradead.org> writes:

> On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
>> This patch factors iomap_write_delalloc_punch() function out. This function
>> is resposible for actual punch out operation.
>> The reason for doing this is, to avoid deep indentation when we bring punch-out
>> of individual non-dirty blocks within a dirty folio in a later patch
>> (which adds per-block dirty status handling to iomap) to avoid delalloc block
>> leak.
>
> A bunch of overly long lines in the commit message here,
> but otherwise this looks good:

Sure. Will shorten those in next revision (considering we might need it for
some minor changes in patch-6)

>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

-ritesh

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Matthew Wilcox]

On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))

I can't help but feel that a

typedef iomap_punch_t(struct inode *, loff_t offset, loff_t length);

would make all of this easier to read.

> +	/*
> +	 * Make sure the next punch start is correctly bound to
> +	 * the end of this data range, not the end of the folio.
> +	 */
> +	*punch_start_byte = min_t(loff_t, end_byte,
> +				  folio_next_index(folio) << PAGE_SHIFT);

	*punch_start_byte = min(end_byte, folio_pos(folio) + folio_size(folio));

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Sat, Jun 10, 2023 at 05:09:05PM +0530, Ritesh Harjani (IBM) wrote:
>> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
>> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
>> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
>
> I can't help but feel that a
>
> typedef iomap_punch_t(struct inode *, loff_t offset, loff_t length);
>
> would make all of this easier to read.
>

Sure. Make sense.

>> +	/*
>> +	 * Make sure the next punch start is correctly bound to
>> +	 * the end of this data range, not the end of the folio.
>> +	 */
>> +	*punch_start_byte = min_t(loff_t, end_byte,
>> +				  folio_next_index(folio) << PAGE_SHIFT);
>
> 	*punch_start_byte = min(end_byte, folio_pos(folio) + folio_size(folio));

Current code was also correct only. But I guess this just avoids
min_t/loff_t thing. No other reason right?

-ritesh

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Matthew Wilcox]

On Mon, Jun 12, 2023 at 07:33:29PM +0530, Ritesh Harjani wrote:
> >> +	*punch_start_byte = min_t(loff_t, end_byte,
> >> +				  folio_next_index(folio) << PAGE_SHIFT);
> >
> > 	*punch_start_byte = min(end_byte, folio_pos(folio) + folio_size(folio));
> 
> Current code was also correct only. But I guess this just avoids
> min_t/loff_t thing. No other reason right?

Actually, it's buggy on 32-bit platforms.  folio_next_index returns
an unsigned long (32 bit), which is shifted by PAGE_SHIFT, losing the
top bits, then cast to an loff_t.  folio_pos() does this correctly.

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Pankaj Raghav]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

Minor nit:

> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
> +{
> +	int ret = 0;
> +
> +	if (!folio_test_dirty(folio))
> +		return ret;
Either this could be changed to `goto out`

OR

> +
> +	/* if dirty, punch up to offset */
> +	if (start_byte > *punch_start_byte) {
> +		ret = punch(inode, *punch_start_byte,
> +				start_byte - *punch_start_byte);
> +		if (ret)
> +			goto out;

This could be changed to `return ret` and we could get rid of the `out`
label.
> +	}
> +	/*
> +	 * Make sure the next punch start is correctly bound to
> +	 * the end of this data range, not the end of the folio.
> +	 */
> +	*punch_start_byte = min_t(loff_t, end_byte,
> +				  folio_next_index(folio) << PAGE_SHIFT);
> +
> +out:
> +	return ret;
> +}
> +

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv9 4/6] iomap: Refactor iomap_write_delalloc_punch() function out

[Ritesh Harjani]

Pankaj Raghav <p.raghav@samsung.com> writes:

> Minor nit:
>
>> +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
>> +		loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
>> +		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
>> +{
>> +	int ret = 0;
>> +
>> +	if (!folio_test_dirty(folio))
>> +		return ret;
> Either this could be changed to `goto out`
>
> OR
>
>> +
>> +	/* if dirty, punch up to offset */
>> +	if (start_byte > *punch_start_byte) {
>> +		ret = punch(inode, *punch_start_byte,
>> +				start_byte - *punch_start_byte);
>> +		if (ret)
>> +			goto out;
>
> This could be changed to `return ret` and we could get rid of the `out`
> label.

Sure, thanks Pankaj. I noted that too.
Since there is nothing in the out label. So mostly will simply return
ret. Will fix it in the next rev.

-ritesh

[PATCHv9 5/6] iomap: Allocate ifs in ->write_begin() early

[Ritesh Harjani (IBM)]

We dont need to allocate an ifs in ->write_begin() for writes where the
position and length completely overlap with the given folio.
Therefore, such cases are skipped.

Currently when the folio is uptodate, we only allocate ifs at writeback
time (in iomap_writepage_map()). This is ok until now, but when we are
going to add support for per-block dirty state bitmap in ifs, this
could cause some performance degradation. The reason is that if we don't
allocate ifs during ->write_begin(), then we will never mark the
necessary dirty bits in ->write_end() call. And we will have to mark all
the bits as dirty at the writeback time, that could cause the same write
amplification and performance problems as it is now.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/iomap/buffered-io.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 1261f26479af..c6dcb0f0d22f 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -567,14 +567,23 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	size_t from = offset_in_folio(folio, pos), to = from + len;
 	size_t poff, plen;
 
-	if (folio_test_uptodate(folio))
+	/*
+	 * If the write completely overlaps the current folio, then
+	 * entire folio will be dirtied so there is no need for
+	 * per-block state tracking structures to be attached to this folio.
+	 */
+	if (pos <= folio_pos(folio) &&
+	    pos + len >= folio_pos(folio) + folio_size(folio))
 		return 0;
-	folio_clear_error(folio);
 
 	ifs = iomap_ifs_alloc(iter->inode, folio, iter->flags);
 	if ((iter->flags & IOMAP_NOWAIT) && !ifs && nr_blocks > 1)
 		return -EAGAIN;
 
+	if (folio_test_uptodate(folio))
+		return 0;
+	folio_clear_error(folio);
+
 	do {
 		iomap_adjust_read_range(iter->inode, folio, &block_start,
 				block_end - block_start, &poff, &plen);
-- 
2.40.1

[PATCHv9 6/6] iomap: Add per-block dirty state tracking to improve performance

[Ritesh Harjani (IBM)]

When filesystem blocksize is less than folio size (either with
mapping_large_folio_support() or with blocksize < pagesize) and when the
folio is uptodate in pagecache, then even a byte write can cause
an entire folio to be written to disk during writeback. This happens
because we currently don't have a mechanism to track per-block dirty
state within struct iomap_folio_state. We currently only track uptodate state.

This patch implements support for tracking per-block dirty state in
iomap_folio_state->state bitmap. This should help improve the filesystem write
performance and help reduce write amplification.

Performance testing of below fio workload reveals ~16x performance
improvement using nvme with XFS (4k blocksize) on Power (64K pagesize)
FIO reported write bw scores improved from around ~28 MBps to ~452 MBps.

1. <test_randwrite.fio>
[global]
	ioengine=psync
	rw=randwrite
	overwrite=1
	pre_read=1
	direct=0
	bs=4k
	size=1G
	dir=./
	numjobs=8
	fdatasync=1
	runtime=60
	iodepth=64
	group_reporting=1

[fio-run]

2. Also our internal performance team reported that this patch improves
   their database workload performance by around ~83% (with XFS on Power)

Reported-by: Aravinda Herle <araherle@in.ibm.com>
Reported-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/gfs2/aops.c         |   2 +-
 fs/iomap/buffered-io.c | 158 +++++++++++++++++++++++++++++++++++++----
 fs/xfs/xfs_aops.c      |   2 +-
 fs/zonefs/file.c       |   2 +-
 include/linux/iomap.h  |   1 +
 5 files changed, 147 insertions(+), 18 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index a5f4be6b9213..75efec3c3b71 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -746,7 +746,7 @@ static const struct address_space_operations gfs2_aops = {
 	.writepages = gfs2_writepages,
 	.read_folio = gfs2_read_folio,
 	.readahead = gfs2_readahead,
-	.dirty_folio = filemap_dirty_folio,
+	.dirty_folio = iomap_dirty_folio,
 	.release_folio = iomap_release_folio,
 	.invalidate_folio = iomap_invalidate_folio,
 	.bmap = gfs2_bmap,
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index c6dcb0f0d22f..d5b8d134921c 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -24,7 +24,7 @@
 #define IOEND_BATCH_SIZE	4096

 /*
- * Structure allocated for each folio to track per-block uptodate state
+ * Structure allocated for each folio to track per-block uptodate, dirty state
  * and I/O completions.
  */
 struct iomap_folio_state {
@@ -34,6 +34,26 @@ struct iomap_folio_state {
 	unsigned long		state[];
 };

+enum iomap_block_state {
+	IOMAP_ST_UPTODATE,
+	IOMAP_ST_DIRTY,
+
+	IOMAP_ST_MAX,
+};
+
+static void iomap_ifs_calc_range(struct folio *folio, size_t off, size_t len,
+		enum iomap_block_state state, unsigned int *first_blkp,
+		unsigned int *nr_blksp)
+{
+	struct inode *inode = folio->mapping->host;
+	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
+	unsigned int first_blk = off >> inode->i_blkbits;
+	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
+
+	*first_blkp = first_blk + (state * blks_per_folio);
+	*nr_blksp = last_blk - first_blk + 1;
+}
+
 static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
 {
 	if (folio_test_private(folio))
@@ -60,12 +80,11 @@ static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
 static void iomap_ifs_set_range_uptodate(struct folio *folio,
 		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
-	struct inode *inode = folio->mapping->host;
-	unsigned int first_blk = off >> inode->i_blkbits;
-	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
-	unsigned int nr_blks = last_blk - first_blk + 1;
+	unsigned int first_blk, nr_blks;
 	unsigned long flags;

+	iomap_ifs_calc_range(folio, off, len, IOMAP_ST_UPTODATE, &first_blk,
+			     &nr_blks);
 	spin_lock_irqsave(&ifs->state_lock, flags);
 	bitmap_set(ifs->state, first_blk, nr_blks);
 	if (iomap_ifs_is_fully_uptodate(folio, ifs))
@@ -84,6 +103,59 @@ static void iomap_set_range_uptodate(struct folio *folio, size_t off,
 		folio_mark_uptodate(folio);
 }

+static inline bool iomap_ifs_is_block_dirty(struct folio *folio,
+		struct iomap_folio_state *ifs, int block)
+{
+	struct inode *inode = folio->mapping->host;
+	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
+
+	return test_bit(block + blks_per_folio, ifs->state);
+}
+
+static void iomap_ifs_clear_range_dirty(struct folio *folio,
+		struct iomap_folio_state *ifs, size_t off, size_t len)
+{
+	unsigned int first_blk, nr_blks;
+	unsigned long flags;
+
+	iomap_ifs_calc_range(folio, off, len, IOMAP_ST_DIRTY, &first_blk,
+			     &nr_blks);
+	spin_lock_irqsave(&ifs->state_lock, flags);
+	bitmap_clear(ifs->state, first_blk, nr_blks);
+	spin_unlock_irqrestore(&ifs->state_lock, flags);
+}
+
+static void iomap_clear_range_dirty(struct folio *folio, size_t off, size_t len)
+{
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
+
+	if (!ifs)
+		return;
+	iomap_ifs_clear_range_dirty(folio, ifs, off, len);
+}
+
+static void iomap_ifs_set_range_dirty(struct folio *folio,
+		struct iomap_folio_state *ifs, size_t off, size_t len)
+{
+	unsigned int first_blk, nr_blks;
+	unsigned long flags;
+
+	iomap_ifs_calc_range(folio, off, len, IOMAP_ST_DIRTY, &first_blk,
+			     &nr_blks);
+	spin_lock_irqsave(&ifs->state_lock, flags);
+	bitmap_set(ifs->state, first_blk, nr_blks);
+	spin_unlock_irqrestore(&ifs->state_lock, flags);
+}
+
+static void iomap_set_range_dirty(struct folio *folio, size_t off, size_t len)
+{
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
+
+	if (!ifs)
+		return;
+	iomap_ifs_set_range_dirty(folio, ifs, off, len);
+}
+
 static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
 		struct folio *folio, unsigned int flags)
 {
@@ -99,14 +171,24 @@ static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
 	else
 		gfp = GFP_NOFS | __GFP_NOFAIL;

-	ifs = kzalloc(struct_size(ifs, state, BITS_TO_LONGS(nr_blocks)),
-		      gfp);
-	if (ifs) {
-		spin_lock_init(&ifs->state_lock);
-		if (folio_test_uptodate(folio))
-			bitmap_fill(ifs->state, nr_blocks);
-		folio_attach_private(folio, ifs);
-	}
+	/*
+	 * ifs->state tracks two sets of state flags when the
+	 * filesystem block size is smaller than the folio size.
+	 * The first state tracks per-block uptodate and the
+	 * second tracks per-block dirty state.
+	 */
+	ifs = kzalloc(struct_size(ifs, state,
+		      BITS_TO_LONGS(IOMAP_ST_MAX * nr_blocks)), gfp);
+	if (!ifs)
+		return ifs;
+
+	spin_lock_init(&ifs->state_lock);
+	if (folio_test_uptodate(folio))
+		bitmap_set(ifs->state, 0, nr_blocks);
+	if (folio_test_dirty(folio))
+		bitmap_set(ifs->state, nr_blocks, nr_blocks);
+	folio_attach_private(folio, ifs);
+
 	return ifs;
 }

@@ -529,6 +611,17 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len)
 }
 EXPORT_SYMBOL_GPL(iomap_invalidate_folio);

+bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio)
+{
+	struct inode *inode = mapping->host;
+	size_t len = folio_size(folio);
+
+	iomap_ifs_alloc(inode, folio, 0);
+	iomap_set_range_dirty(folio, 0, len);
+	return filemap_dirty_folio(mapping, folio);
+}
+EXPORT_SYMBOL_GPL(iomap_dirty_folio);
+
 static void
 iomap_write_failed(struct inode *inode, loff_t pos, unsigned len)
 {
@@ -733,6 +826,7 @@ static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
 	if (unlikely(copied < len && !folio_test_uptodate(folio)))
 		return 0;
 	iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len);
+	iomap_set_range_dirty(folio, offset_in_folio(folio, pos), copied);
 	filemap_dirty_folio(inode->i_mapping, folio);
 	return copied;
 }
@@ -902,6 +996,10 @@ static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
 		int (*punch)(struct inode *inode, loff_t offset, loff_t length))
 {
 	int ret = 0;
+	struct iomap_folio_state *ifs;
+	unsigned int first_blk, last_blk, i;
+	loff_t last_byte;
+	u8 blkbits = inode->i_blkbits;

 	if (!folio_test_dirty(folio))
 		return ret;
@@ -913,6 +1011,30 @@ static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
 		if (ret)
 			goto out;
 	}
+	/*
+	 * When we have per-block dirty tracking, there can be
+	 * blocks within a folio which are marked uptodate
+	 * but not dirty. In that case it is necessary to punch
+	 * out such blocks to avoid leaking any delalloc blocks.
+	 */
+	ifs = iomap_get_ifs(folio);
+	if (!ifs)
+		goto skip_ifs_punch;
+
+	last_byte = min_t(loff_t, end_byte - 1,
+		(folio_next_index(folio) << PAGE_SHIFT) - 1);
+	first_blk = offset_in_folio(folio, start_byte) >> blkbits;
+	last_blk = offset_in_folio(folio, last_byte) >> blkbits;
+	for (i = first_blk; i <= last_blk; i++) {
+		if (!iomap_ifs_is_block_dirty(folio, ifs, i)) {
+			ret = punch(inode, folio_pos(folio) + (i << blkbits),
+				    1 << blkbits);
+			if (ret)
+				goto out;
+		}
+	}
+
+skip_ifs_punch:
 	/*
 	 * Make sure the next punch start is correctly bound to
 	 * the end of this data range, not the end of the folio.
@@ -1646,7 +1768,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		struct writeback_control *wbc, struct inode *inode,
 		struct folio *folio, u64 end_pos)
 {
-	struct iomap_folio_state *ifs = iomap_ifs_alloc(inode, folio, 0);
+	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
 	struct iomap_ioend *ioend, *next;
 	unsigned len = i_blocksize(inode);
 	unsigned nblocks = i_blocks_per_folio(inode, folio);
@@ -1654,6 +1776,11 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	int error = 0, count = 0, i;
 	LIST_HEAD(submit_list);

+	if (!ifs && nblocks > 1) {
+		ifs = iomap_ifs_alloc(inode, folio, 0);
+		iomap_set_range_dirty(folio, 0, folio_size(folio));
+	}
+
 	WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) != 0);

 	/*
@@ -1662,7 +1789,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	 * invalid, grab a new one.
 	 */
 	for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
-		if (ifs && !iomap_ifs_is_block_uptodate(ifs, i))
+		if (ifs && !iomap_ifs_is_block_dirty(folio, ifs, i))
 			continue;

 		error = wpc->ops->map_blocks(wpc, inode, pos);
@@ -1706,6 +1833,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		}
 	}

+	iomap_clear_range_dirty(folio, 0, end_pos - folio_pos(folio));
 	folio_start_writeback(folio);
 	folio_unlock(folio);

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 2ef78aa1d3f6..77c7332ae197 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -578,7 +578,7 @@ const struct address_space_operations xfs_address_space_operations = {
 	.read_folio		= xfs_vm_read_folio,
 	.readahead		= xfs_vm_readahead,
 	.writepages		= xfs_vm_writepages,
-	.dirty_folio		= filemap_dirty_folio,
+	.dirty_folio		= iomap_dirty_folio,
 	.release_folio		= iomap_release_folio,
 	.invalidate_folio	= iomap_invalidate_folio,
 	.bmap			= xfs_vm_bmap,
diff --git a/fs/zonefs/file.c b/fs/zonefs/file.c
index 132f01d3461f..e508c8e97372 100644
--- a/fs/zonefs/file.c
+++ b/fs/zonefs/file.c
@@ -175,7 +175,7 @@ const struct address_space_operations zonefs_file_aops = {
 	.read_folio		= zonefs_read_folio,
 	.readahead		= zonefs_readahead,
 	.writepages		= zonefs_writepages,
-	.dirty_folio		= filemap_dirty_folio,
+	.dirty_folio		= iomap_dirty_folio,
 	.release_folio		= iomap_release_folio,
 	.invalidate_folio	= iomap_invalidate_folio,
 	.migrate_folio		= filemap_migrate_folio,
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index e2b836c2e119..eb9335c46bf3 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -264,6 +264,7 @@ bool iomap_is_partially_uptodate(struct folio *, size_t from, size_t count);
 struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos);
 bool iomap_release_folio(struct folio *folio, gfp_t gfp_flags);
 void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len);
+bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio);
 int iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len,
 		const struct iomap_ops *ops);
 int iomap_zero_range(struct inode *inode, loff_t pos, loff_t len,
--
2.40.1

Re: [PATCHv9 6/6] iomap: Add per-block dirty state tracking to improve performance

[Christoph Hellwig]

Just some nitpicks, this otherwise looks fine.

First during the last patches ifs as a variable name has started
to really annoy me and I'm not sure why.  I'd like to hear from the
others, bu maybe just state might be a better name that flows easier?

> +static void iomap_clear_range_dirty(struct folio *folio, size_t off, size_t len)
> +{
> +	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
> +
> +	if (!ifs)
> +		return;
> +	iomap_ifs_clear_range_dirty(folio, ifs, off, len);

Maybe just do

	if (ifs)
		iomap_ifs_clear_range_dirty(folio, ifs, off, len);

?

But also do we even need the ifs argument to iomap_ifs_clear_range_dirty
after we've removed it everywhere else earlier?

> +	/*
> +	 * When we have per-block dirty tracking, there can be
> +	 * blocks within a folio which are marked uptodate
> +	 * but not dirty. In that case it is necessary to punch
> +	 * out such blocks to avoid leaking any delalloc blocks.
> +	 */
> +	ifs = iomap_get_ifs(folio);
> +	if (!ifs)
> +		goto skip_ifs_punch;
> +
> +	last_byte = min_t(loff_t, end_byte - 1,
> +		(folio_next_index(folio) << PAGE_SHIFT) - 1);
> +	first_blk = offset_in_folio(folio, start_byte) >> blkbits;
> +	last_blk = offset_in_folio(folio, last_byte) >> blkbits;
> +	for (i = first_blk; i <= last_blk; i++) {
> +		if (!iomap_ifs_is_block_dirty(folio, ifs, i)) {
> +			ret = punch(inode, folio_pos(folio) + (i << blkbits),
> +				    1 << blkbits);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +
> +skip_ifs_punch:

And happy to hear from the others, but to me having a helper for
all the iomap_folio_state manipulation rather than having it in
the middle of the function and jumped over if not needed would
improve the code structure.

Re: [PATCHv9 6/6] iomap: Add per-block dirty state tracking to improve performance

[Ritesh Harjani]

Christoph Hellwig <hch@infradead.org> writes:

> Just some nitpicks,

Sure.

> this otherwise looks fine.

Thanks for the review.

>
> First during the last patches ifs as a variable name has started
> to really annoy me and I'm not sure why.  I'd like to hear from the
> others, bu maybe just state might be a better name that flows easier?
>

Ok. Let's hear from others too.

>> +static void iomap_clear_range_dirty(struct folio *folio, size_t off, size_t len)
>> +{
>> +	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
>> +
>> +	if (!ifs)
>> +		return;
>> +	iomap_ifs_clear_range_dirty(folio, ifs, off, len);
>
> Maybe just do
>
> 	if (ifs)
> 		iomap_ifs_clear_range_dirty(folio, ifs, off, len);
>
> ?

Sure.

>
> But also do we even need the ifs argument to iomap_ifs_clear_range_dirty
> after we've removed it everywhere else earlier?
>

Some of the previous discussions / reasoning behind it -

- In one of the previous discussions we discussed that functions which
has _ifs_ in their naming, then it generally should imply that we will
be working on iomap_folio_state struct. So we should pass that as a
argument.

- Also in most of these *_ifs_* functions we have "ifs" as a non-null
  function argument.

- At some places where we are calling these _ifs_ functions, we
already have derived ifs, so why not just pass it.

FYI - We dropped "ifs" argument in one of the function which is
iomap_set_range_uptodate(), because we would like this function
to work in both cases.
    1. When we have non-null folio->private (ifs)
    2. When it is null.

So API wise it looks good in my humble opinion. But sure, in
case if someone has better ideas, I can look into that.

>> +	/*
>> +	 * When we have per-block dirty tracking, there can be
>> +	 * blocks within a folio which are marked uptodate
>> +	 * but not dirty. In that case it is necessary to punch
>> +	 * out such blocks to avoid leaking any delalloc blocks.
>> +	 */
>> +	ifs = iomap_get_ifs(folio);
>> +	if (!ifs)
>> +		goto skip_ifs_punch;
>> +
>> +	last_byte = min_t(loff_t, end_byte - 1,
>> +		(folio_next_index(folio) << PAGE_SHIFT) - 1);
>> +	first_blk = offset_in_folio(folio, start_byte) >> blkbits;
>> +	last_blk = offset_in_folio(folio, last_byte) >> blkbits;
>> +	for (i = first_blk; i <= last_blk; i++) {
>> +		if (!iomap_ifs_is_block_dirty(folio, ifs, i)) {
>> +			ret = punch(inode, folio_pos(folio) + (i << blkbits),
>> +				    1 << blkbits);
>> +			if (ret)
>> +				goto out;
>> +		}
>> +	}
>> +
>> +skip_ifs_punch:
>
> And happy to hear from the others, but to me having a helper for
> all the iomap_folio_state manipulation rather than having it in
> the middle of the function and jumped over if not needed would
> improve the code structure.

I think Darrick was also pointing towards having a separate funciton.
But let's hear from him & others too. I can consider adding a separate
function for above.

Does this look good?

static int iomap_write_delalloc_ifs_punch(struct inode *inode, struct folio *folio,
		struct iomap_folio_state *ifs, loff_t start_byte, loff_t end_byte,
		int (*punch)(struct inode *inode, loff_t offset, loff_t length))

The function argument are kept similar to what we have for
iomap_write_delalloc_punch(), except maybe *punch_start_byte (which is
not required).

-ritesh

Re: [PATCHv9 6/6] iomap: Add per-block dirty state tracking to improve performance

[Matthew Wilcox]

On Sat, Jun 10, 2023 at 05:09:07PM +0530, Ritesh Harjani (IBM) wrote:
> +static void iomap_ifs_calc_range(struct folio *folio, size_t off, size_t len,
> +		enum iomap_block_state state, unsigned int *first_blkp,
> +		unsigned int *nr_blksp)
> +{
> +	struct inode *inode = folio->mapping->host;
> +	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> +	unsigned int first_blk = off >> inode->i_blkbits;
> +	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
> +
> +	*first_blkp = first_blk + (state * blks_per_folio);

This _isn't_ first_blk.  It's first_bit.  You could just rename it to
'first', but misnaming it as first_blkp is going to confuse.

Re: [PATCHv9 0/6] iomap: Add support for per-block dirty state to improve write performance

[Ritesh Harjani]

"Ritesh Harjani (IBM)" <ritesh.list@gmail.com> writes:

Hello All,

So I gave some thoughts about function naming and I guess the reason we
are ping ponging between the different namings is that I am not able to
properly articulate the reasoning behind, why we chose iomap_ifs_**.

Here is my attempt to convince everyone....

In one of the previous versions of the patchsets, Christoph opposed the
idea of naming these functions with iop_** because he wanted iomap_ as a
prefix in all of these function names. Now that I gave more thought to it,
I too agree that we should have iomap_ as prefix in these APIs. Because
- fs/iomap/buffered-io.c follows that style for all other functions.
- It then also becomes easy in finding function names using ctags and
  in doing grep or fuzzy searches.

Now why "ifs" in the naming because we are abbrevating iomap_folio_state
as "ifs". And since we are passing ifs as an argument in these functions
and operating upon it, hence the naming of all of these functions should
go as iomap_ifs_**.

Now if I am reading all of the emails correctly, none of the reviewers have
any strong objections with iomap_ifs_** naming style. Some of us just
started with nitpicking, but there are no strong objections, I feel.
Also I do think iomap_ifs_** naming is completely apt for these
functional changes.

So if no one has any strong objections, could we please continue with
iomap_ifs_** naming itself.

In case if someone does oppose strongly, I would humbly request to please
also convince the rest of the reviewers on why your function naming
should be chosen by giving proper reasoning (like above).
I can definitely help with making the required changes and testing them.

Does this look good and sound fair for the function naming part?

If everyone is convinced with iomap_ifs_** naming, then I will go ahead
and work on the rest of the review comments.

Thanks a lot for all the great review!
-ritesh

Re: [PATCHv9 0/6] iomap: Add support for per-block dirty state to improve write performance

[Ritesh Harjani]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> "Ritesh Harjani (IBM)" <ritesh.list@gmail.com> writes:
>
> Hello All,
>
> So I gave some thoughts about function naming and I guess the reason we
> are ping ponging between the different namings is that I am not able to
> properly articulate the reasoning behind, why we chose iomap_ifs_**.
>
> Here is my attempt to convince everyone....
>
> In one of the previous versions of the patchsets, Christoph opposed the
> idea of naming these functions with iop_** because he wanted iomap_ as a
> prefix in all of these function names. Now that I gave more thought to it,
> I too agree that we should have iomap_ as prefix in these APIs. Because
> - fs/iomap/buffered-io.c follows that style for all other functions.
> - It then also becomes easy in finding function names using ctags and
>   in doing grep or fuzzy searches.
>
> Now why "ifs" in the naming because we are abbrevating iomap_folio_state
> as "ifs". And since we are passing ifs as an argument in these functions
> and operating upon it, hence the naming of all of these functions should
> go as iomap_ifs_**.
>
> Now if I am reading all of the emails correctly, none of the reviewers have
> any strong objections with iomap_ifs_** naming style. Some of us just
> started with nitpicking, but there are no strong objections, I feel.
> Also I do think iomap_ifs_** naming is completely apt for these
> functional changes.
>
> So if no one has any strong objections, could we please continue with
> iomap_ifs_** naming itself.
>
> In case if someone does oppose strongly, I would humbly request to please
> also convince the rest of the reviewers on why your function naming
> should be chosen by giving proper reasoning (like above).
> I can definitely help with making the required changes and testing them.
>
> Does this look good and sound fair for the function naming part?

Hello All,

Hope I didn't take too long to respond to my previous email.
I had a discussion with Darrick and he convinced me with -

- ifs_ naming style will be much shorter and hence preferred.
- ifs_ already means iomap_folio_state_** v/s iomap_fs_** means
  iomap_iomap_folio_state... makes iomap_ifs_ naming awkward.
- All of these functions are anyway local and static to
  fs/iomap/buffered-io.c file so it is ok even if we have a shorter
  names like ifs_alloc()/ifs_free() etc.

Hence I have decided to go with ifs_ options for v10 which Darrick (including few others) agreed to here [1]

[1]: https://lore.kernel.org/linux-xfs/87h6r8wyxa.fsf@doe.com/T/#m7c061e634243f835ecddfb642bbfb091a9227ec7

-ritesh


>
> If everyone is convinced with iomap_ifs_** naming, then I will go ahead
> and work on the rest of the review comments.
>
> Thanks a lot for all the great review!
> -ritesh