Re: [RFC PATCH V4] iomap: add support to track dirty state of sub pages

[yukuai (C)]

On 2020/08/21 20:44, Matthew Wilcox wrote:
> On Fri, Aug 21, 2020 at 08:33:06PM +0800, Yu Kuai wrote:
> > changes from v3: - add IOMAP_STATE_ARRAY_SIZE - replace set_bit / 
> > clear_bit with bitmap_set / bitmap_clear - move 
> > iomap_set_page_dirty() out of 'iop->state_lock' - merge 
> > iomap_set/clear_range_dirty() and 
> > iomap_iop_clear/clear_range_dirty()
>
> I'm still working on the iomap parts of the THP series (fixing up 
> invalidatepage right now), but here are some of the relevant bits 
> (patch series to follow)

Hi, Matthew

Since your THP iomap patches were reviewed, I made some modifications
based on these patches.

Best regards,
Yu Kuai

---
 fs/iomap/buffered-io.c | 92 +++++++++++++++++++++++++++++++++---------
 1 file changed, 74 insertions(+), 18 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index edf5eea56cf5..bc7f57748be8 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -23,13 +23,17 @@

 /*
  * Structure allocated for each page or THP when block size < page size
- * to track sub-page uptodate status and I/O completions.
+ * to track sub-page status and I/O completions.
  */
 struct iomap_page {
 	atomic_t		read_bytes_pending;
 	atomic_t		write_bytes_pending;
-	spinlock_t		uptodate_lock;
-	unsigned long		uptodate[];
+	spinlock_t		state_lock;
+	/*
+	 * The first half bits are used to track sub-page uptodate status,
+	 * the second half bits are for dirty status.
+	 */
+	unsigned long		state[];
 };

 static inline struct iomap_page *to_iomap_page(struct page *page)
@@ -57,9 +61,9 @@ iomap_page_create(struct inode *inode, struct page *page)
 	if (iop || nr_blocks <= 1)
 		return iop;

-	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
+	iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(2 * nr_blocks)),
 			GFP_NOFS | __GFP_NOFAIL);
-	spin_lock_init(&iop->uptodate_lock);
+	spin_lock_init(&iop->state_lock);
 	attach_page_private(page, iop);
 	return iop;
 }
@@ -74,7 +78,7 @@ iomap_page_release(struct page *page)
 		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(bitmap_full(iop->state, nr_blocks) !=
 			PageUptodate(page));
 	kfree(iop);
 }
@@ -105,7 +109,7 @@ iomap_adjust_read_range(struct inode *inode, struct 
iomap_page *iop,

 		/* move forward for each leading block marked uptodate */
 		for (i = first; i <= last; i++) {
-			if (!test_bit(i, iop->uptodate))
+			if (!test_bit(i, iop->state))
 				break;
 			*pos += block_size;
 			poff += block_size;
@@ -115,7 +119,7 @@ iomap_adjust_read_range(struct inode *inode, struct 
iomap_page *iop,

 		/* truncate len if we find any trailing uptodate block(s) */
 		for ( ; i <= last; i++) {
-			if (test_bit(i, iop->uptodate)) {
+			if (test_bit(i, iop->state)) {
 				plen -= (last - i + 1) * block_size;
 				last = i - 1;
 				break;
@@ -139,6 +143,55 @@ iomap_adjust_read_range(struct inode *inode, struct 
iomap_page *iop,
 	*lenp = plen;
 }

+static void
+iomap_set_range_dirty(struct page *page, unsigned int off,
+		unsigned int len)
+{
+	struct inode *inode = page->mapping->host;
+	unsigned int blocks_per_page = i_blocks_per_page(inode, page);
+	unsigned int first = (off >> inode->i_blkbits) + blocks_per_page;
+	unsigned int last = ((off + len - 1) >> inode->i_blkbits) + 
blocks_per_page;
+	unsigned long flags;
+	struct iomap_page *iop;
+
+	if (PageError(page))
+		return;
+
+	if (len)
+		iomap_set_page_dirty(page);
+
+	if (!page_has_private(page))
+		return;
+
+	iop = to_iomap_page(page);
+	spin_lock_irqsave(&iop->state_lock, flags);
+	bitmap_set(iop->state, first, last - first + 1);
+	spin_unlock_irqrestore(&iop->state_lock, flags);
+}
+
+static void
+iomap_clear_range_dirty(struct page *page, unsigned int off,
+		unsigned int len)
+{
+	struct inode *inode = page->mapping->host;
+	unsigned int blocks_per_page = i_blocks_per_page(inode, page);
+	unsigned int first = (off >> inode->i_blkbits) + blocks_per_page;
+	unsigned int last = ((off + len - 1) >> inode->i_blkbits) + 
blocks_per_page;
+	unsigned long flags;
+	struct iomap_page *iop;
+
+	if (PageError(page))
+		return;
+
+	if (!page_has_private(page))
+		return;
+
+	iop = to_iomap_page(page);
+	spin_lock_irqsave(&iop->state_lock, flags);
+	bitmap_clear(iop->state, first, last - first + 1);
+	spin_unlock_irqrestore(&iop->state_lock, flags);
+}
+
 static void
 iomap_iop_set_range_uptodate(struct page *page, unsigned off, unsigned 
len)
 {
@@ -148,11 +201,11 @@ iomap_iop_set_range_uptodate(struct page *page, 
unsigned off, unsigned len)
 	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_page(inode, page)))
+	spin_lock_irqsave(&iop->state_lock, flags);
+	bitmap_set(iop->state, first, last - first + 1);
+	if (bitmap_full(iop->state, i_blocks_per_page(inode, page)))
 		SetPageUptodate(page);
-	spin_unlock_irqrestore(&iop->uptodate_lock, flags);
+	spin_unlock_irqrestore(&iop->state_lock, flags);
 }

 static void
@@ -445,7 +498,7 @@ iomap_is_partially_uptodate(struct page *page, 
unsigned long from,

 	if (iop) {
 		for (i = first; i <= last; i++)
-			if (!test_bit(i, iop->uptodate))
+			if (!test_bit(i, iop->state))
 				return 0;
 		return 1;
 	}
@@ -683,7 +736,7 @@ static size_t __iomap_write_end(struct inode *inode, 
loff_t pos, size_t len,
 	if (unlikely(copied < len && !PageUptodate(page)))
 		return 0;
 	iomap_set_range_uptodate(page, offset_in_page(pos), len);
-	iomap_set_page_dirty(page);
+	iomap_set_range_dirty(page, offset_in_page(pos), len);
 	return copied;
 }

@@ -997,7 +1050,6 @@ iomap_page_mkwrite_actor(struct inode *inode, 
loff_t pos, loff_t length,
 	} else {
 		WARN_ON_ONCE(!PageUptodate(page));
 		iomap_page_create(inode, page);
-		set_page_dirty(page);
 	}

 	return length;
@@ -1007,7 +1059,7 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault 
*vmf, const struct iomap_ops *ops)
 {
 	struct page *page = vmf->page;
 	struct inode *inode = file_inode(vmf->vma->vm_file);
-	unsigned long length;
+	unsigned int length, dirty_bits;
 	loff_t offset;
 	ssize_t ret;

@@ -1016,6 +1068,7 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault 
*vmf, const struct iomap_ops *ops)
 	if (ret < 0)
 		goto out_unlock;
 	length = ret;
+	dirty_bits = ret;

 	offset = page_offset(page);
 	while (length > 0) {
@@ -1028,6 +1081,7 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault 
*vmf, const struct iomap_ops *ops)
 		length -= ret;
 	}

+	iomap_set_range_dirty(page, 0, dirty_bits);
 	wait_for_stable_page(page);
 	return VM_FAULT_LOCKED;
 out_unlock:
@@ -1340,11 +1394,12 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	struct iomap_page *iop = to_iomap_page(page);
 	struct iomap_ioend *ioend, *next;
 	unsigned len = i_blocksize(inode);
+	unsigned int blocks_per_page = i_blocks_per_page(inode, page);
 	u64 file_offset; /* file offset of page */
 	int error = 0, count = 0, i;
 	LIST_HEAD(submit_list);

-	WARN_ON_ONCE(i_blocks_per_page(inode, page) > 1 && !iop);
+	WARN_ON_ONCE(blocks_per_page > 1 && !iop);
 	WARN_ON_ONCE(iop && atomic_read(&iop->write_bytes_pending) != 0);

 	/*
@@ -1355,7 +1410,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	for (i = 0, file_offset = page_offset(page);
 	     i < (PAGE_SIZE >> inode->i_blkbits) && file_offset < end_offset;
 	     i++, file_offset += len) {
-		if (iop && !test_bit(i, iop->uptodate))
+		if (iop && !test_bit(i, iop->state + blocks_per_page))
 			continue;

 		error = wpc->ops->map_blocks(wpc, inode, file_offset);
@@ -1404,6 +1459,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		 */
 		set_page_writeback_keepwrite(page);
 	} else {
+		iomap_clear_range_dirty(page, 0, PAGE_SIZE);
 		clear_page_dirty_for_io(page);
 		set_page_writeback(page);
 	}
--
2.25.4


.