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

["yukuai (C)" <yukuai3@huawei.com>]

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


.