Skip to contentSkip to conversation

ext4: misc left over folio changes

RFC Superseded Ritesh Harjani (IBM) 3 participants 13 messages Apr 16, 2023 – May 14, 2023

Subsystems (2)
Tracing ext4 file system

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Ritesh Harjani (IBM)

author reviewer
#0 lore raw

cover ext4: misc left over folio changes

Hello Matthew,

Could you please review this series. I found this during code review and I think these can/should go in as well along with other folio changes queued up for 6.4 (after some testing).

Also had a query w.r.t your change [1]. I couldn't understand this change diff from [1]. Given if we are making the conversion to folio, then shouldn't we do len = size - folio_pos(pos), instead of len = size & ~PAGE_MASK Could you please tell if the current change in [1] is kept deliberately? At other places you did make len as size - folio_pos(pos) which removes the PAGE_SIZE assumption.

-static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page) +static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
{
- int len;
+ size_t len;

	<...>

	size = i_size_read(mpd->inode);

- if (page->index == size >> PAGE_SHIFT &&
+ len = folio_size(folio);
+ if (folio_pos(folio) + len > size &&
!ext4_verity_in_progress(mpd->inode))
len = size & ~PAGE_MASK;
- else
- len = PAGE_SIZE;
- err = ext4_bio_write_page(&mpd->io_submit, page, len);
+ err = ext4_bio_write_page(&mpd->io_submit, &folio->page, len);
if (!err)
mpd->wbc->nr_to_write--;

[1]: https://lore.kernel.org/linux-ext4/[email protected]/

Note: I haven't yet tested this series completely. I mainly first wanted to get some initial inputs from Matthew on this one before I do any serious fstests testing of the changes. If the changes are in the right direction, I shall do some testing before sending next revision.

Ritesh Harjani (IBM) (4):
ext4: kill unused function ext4_journalled_write_inline_data
ext4: Change remaining tracepoints to use folio
ext4: Make mpage_journal_page_buffers use folio
ext4: Make ext4_write_inline_data_end() use folio

 fs/ext4/ext4.h              | 10 ++-----
 fs/ext4/inline.c            | 27 +-----------------
 fs/ext4/inode.c             | 57 +++++++++++++++++++------------------
 include/trace/events/ext4.h | 26 ++++++++---------
 4 files changed, 46 insertions(+), 74 deletions(-)

--
2.39.2

author reviewer
#1 lore raw

RFC 1/4 ext4: kill unused function ext4_journalled_write_inline_data

Commit 3f079114bf522 ("ext4: Convert data=journal writeback to use ext4_writepages()") Added support for writeback of journalled data into ext4_writepages() and killed function __ext4_journalled_writepage() which used to call ext4_journalled_write_inline_data() for inline data.
This function got left over by mistake. Hence kill it's definition as
no one uses it.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext4/ext4.h   |  4 ----
 fs/ext4/inline.c | 24 ------------------------
 2 files changed, 28 deletions(-)
fs/ext4/ext4.h +0 -4
index 83f0cc02250f..b00597b41c96 100644
@@ -3558,10 +3558,6 @@ extern int ext4_write_inline_data_end(struct inode *inode,
 				      loff_t pos, unsigned len,
 				      unsigned copied,
 				      struct page *page);
-extern struct buffer_head *
-ext4_journalled_write_inline_data(struct inode *inode,
-				  unsigned len,
-				  struct page *page);
 extern int ext4_da_write_inline_data_begin(struct address_space *mapping,
 					   struct inode *inode,
 					   loff_t pos, unsigned len,
fs/ext4/inline.c +0 -24
index b9fb1177fff6..062c7747bb40 100644
@@ -812,30 +812,6 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
 	return ret ? ret : copied;
 }
 
-struct buffer_head *
-ext4_journalled_write_inline_data(struct inode *inode,
-				  unsigned len,
-				  struct page *page)
-{
-	int ret, no_expand;
-	void *kaddr;
-	struct ext4_iloc iloc;
-
-	ret = ext4_get_inode_loc(inode, &iloc);
-	if (ret) {
-		ext4_std_error(inode->i_sb, ret);
-		return NULL;
-	}
-
-	ext4_write_lock_xattr(inode, &no_expand);
-	kaddr = kmap_atomic(page);
-	ext4_write_inline_data(inode, &iloc, kaddr, 0, len);
-	kunmap_atomic(kaddr);
-	ext4_write_unlock_xattr(inode, &no_expand);
-
-	return iloc.bh;
-}
-
 /*
  * Try to make the page cache and handle ready for the inline data case.
  * We can call this function in 2 cases:
Signature · 1 line
2.39.2
author reviewer
#3 lore raw

RFC 2/4 ext4: Change remaining tracepoints to use folio

In reply to Ritesh Harjani (IBM)

est4_readpage() is converted to ext4_read_folio() hence change the related tracepoint from trace_ext4_readpage(page) to trace_ext4_read_folio(folio).

Do the same for trace_ext4_releasepage(page) to
trace_ext4_release_folio(folio)

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext4/inode.c             |  4 ++--
 include/trace/events/ext4.h | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)
fs/ext4/inode.c +2 -2
index 6d628d6c0847..5bb141288b1b 100644
@@ -3157,7 +3157,7 @@ static int ext4_read_folio(struct file *file, struct folio *folio)
 	int ret = -EAGAIN;
 	struct inode *inode = folio->mapping->host;
 
-	trace_ext4_readpage(&folio->page);
+	trace_ext4_read_folio(folio);
 
 	if (ext4_has_inline_data(inode))
 		ret = ext4_readpage_inline(inode, folio);

@@ -3218,7 +3218,7 @@ static bool ext4_release_folio(struct folio *folio, gfp_t wait)
 {
 	journal_t *journal = EXT4_JOURNAL(folio->mapping->host);
 
-	trace_ext4_releasepage(&folio->page);
+	trace_ext4_release_folio(folio);
 
 	/* Page has dirty journalled data -> cannot release */
 	if (folio_test_checked(folio))
include/trace/events/ext4.h +13 -13
index ebccf6a6aa1b..a9415f1c68ec 100644
@@ -560,10 +560,10 @@ TRACE_EVENT(ext4_writepages_result,
 		  (unsigned long) __entry->writeback_index)
 );
 
-DECLARE_EVENT_CLASS(ext4__page_op,
-	TP_PROTO(struct page *page),
+DECLARE_EVENT_CLASS(ext4__folio_op,
+	TP_PROTO(struct folio *folio),
 
-	TP_ARGS(page),
+	TP_ARGS(folio),
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)

@@ -573,29 +573,29 @@ DECLARE_EVENT_CLASS(ext4__page_op,
 	),
 
 	TP_fast_assign(
-		__entry->dev	= page->mapping->host->i_sb->s_dev;
-		__entry->ino	= page->mapping->host->i_ino;
-		__entry->index	= page->index;
+		__entry->dev	= folio->mapping->host->i_sb->s_dev;
+		__entry->ino	= folio->mapping->host->i_ino;
+		__entry->index	= folio->index;
 	),
 
-	TP_printk("dev %d,%d ino %lu page_index %lu",
+	TP_printk("dev %d,%d ino %lu folio_index %lu",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  (unsigned long) __entry->ino,
 		  (unsigned long) __entry->index)
 );
 
-DEFINE_EVENT(ext4__page_op, ext4_readpage,
+DEFINE_EVENT(ext4__folio_op, ext4_read_folio,
 
-	TP_PROTO(struct page *page),
+	TP_PROTO(struct folio *folio),
 
-	TP_ARGS(page)
+	TP_ARGS(folio)
 );
 
-DEFINE_EVENT(ext4__page_op, ext4_releasepage,
+DEFINE_EVENT(ext4__folio_op, ext4_release_folio,
 
-	TP_PROTO(struct page *page),
+	TP_PROTO(struct folio *folio),
 
-	TP_ARGS(page)
+	TP_ARGS(folio)
 );
 
 DECLARE_EVENT_CLASS(ext4_invalidate_folio_op,
Signature · 1 line
2.39.2
author reviewer
#5 lore raw

RFC 3/4 ext4: Make mpage_journal_page_buffers use folio

In reply to Ritesh Harjani (IBM)

This patch converts mpage_journal_page_buffers() to use folio and also removes the PAGE_SIZE assumption.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext4/inode.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)
fs/ext4/inode.c +14 -16
index 5bb141288b1b..3f76b06e9aa4 100644
@@ -2349,11 +2349,11 @@ static bool ext4_folio_nomap_can_writeout(struct folio *folio)
 	return false;
 }
 
-static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
-				     int len)
+static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
+				     size_t len)
 {
-	struct buffer_head *page_bufs = page_buffers(page);
-	struct inode *inode = page->mapping->host;
+	struct buffer_head *page_bufs = folio_buffers(folio);
+	struct inode *inode = folio->mapping->host;
 	int ret, err;
 
 	ret = ext4_walk_page_buffers(handle, inode, page_bufs, 0, len,

@@ -2362,7 +2362,7 @@ static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
 				     NULL, write_end_fn);
 	if (ret == 0)
 		ret = err;
-	err = ext4_jbd2_inode_add_write(handle, inode, page_offset(page), len);
+	err = ext4_jbd2_inode_add_write(handle, inode, folio_pos(folio), len);
 	if (ret == 0)
 		ret = err;
 	EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;

@@ -2374,23 +2374,21 @@ static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
 
 static int mpage_journal_page_buffers(handle_t *handle,
 				      struct mpage_da_data *mpd,
-				      struct page *page)
+				      struct folio *folio)
 {
 	struct inode *inode = mpd->inode;
 	loff_t size = i_size_read(inode);
-	int len;
+	size_t len = folio_size(folio);
 
-	ClearPageChecked(page);
-	clear_page_dirty_for_io(page);
+	folio_clear_checked(folio);
+	folio_clear_dirty_for_io(folio);
 	mpd->wbc->nr_to_write--;
 
-	if (page->index == size >> PAGE_SHIFT &&
+	if (folio_pos(folio) + len > size &&
 	    !ext4_verity_in_progress(inode))
-		len = size & ~PAGE_MASK;
-	else
-		len = PAGE_SIZE;
+		len = size - folio_pos(folio);
 
-	return ext4_journal_page_buffers(handle, page, len);
+	return ext4_journal_page_buffers(handle, folio, len);
 }
 
 /*

@@ -2546,7 +2544,7 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 					WARN_ON_ONCE(sb->s_writers.frozen >=
 						     SB_FREEZE_FS);
 					err = mpage_journal_page_buffers(handle,
-						mpd, &folio->page);
+						mpd, folio);
 					if (err < 0)
 						goto out;
 				}

@@ -6184,7 +6182,7 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 		err = __block_write_begin(&folio->page, 0, len, ext4_get_block);
 		if (!err) {
 			ret = VM_FAULT_SIGBUS;
-			if (ext4_journal_page_buffers(handle, &folio->page, len))
+			if (ext4_journal_page_buffers(handle, folio, len))
 				goto out_error;
 		} else {
 			folio_unlock(folio);
Signature · 1 line
2.39.2
author reviewer
#9 lore raw

RFC 4/4 ext4: Make ext4_write_inline_data_end() use folio

In reply to Ritesh Harjani (IBM)

ext4_write_inline_data_end() is completely converted to work with folio. Also all callers of ext4_write_inline_data_end() already works on folio except ext4_da_write_end(). Mostly for consistency and saving few instructions maybe, this patch just converts ext4_da_write_end() to work with folio which makes the last caller of ext4_write_inline_data_end() also converted to work with folio.
We then make ext4_write_inline_data_end() take folio instead of page.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext4/ext4.h   |  6 ++----
 fs/ext4/inline.c |  3 +--
 fs/ext4/inode.c  | 23 ++++++++++++++---------
 3 files changed, 17 insertions(+), 15 deletions(-)
fs/ext4/ext4.h +2 -4
index b00597b41c96..081f04ea2be0 100644
@@ -3554,10 +3554,8 @@ extern int ext4_try_to_write_inline_data(struct address_space *mapping,
 					 struct inode *inode,
 					 loff_t pos, unsigned len,
 					 struct page **pagep);
-extern int ext4_write_inline_data_end(struct inode *inode,
-				      loff_t pos, unsigned len,
-				      unsigned copied,
-				      struct page *page);
+int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
+			       unsigned copied, struct folio *folio);
 extern int ext4_da_write_inline_data_begin(struct address_space *mapping,
 					   struct inode *inode,
 					   loff_t pos, unsigned len,
fs/ext4/inline.c +1 -2
index 062c7747bb40..eee78035c12d 100644
@@ -730,9 +730,8 @@ int ext4_try_to_write_inline_data(struct address_space *mapping,
 }
 
 int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
-			       unsigned copied, struct page *page)
+			       unsigned copied, struct folio *folio)
 {
-	struct folio *folio = page_folio(page);
 	handle_t *handle = ext4_journal_current_handle();
 	int no_expand;
 	void *kaddr;
fs/ext4/inode.c +14 -9
index 3f76b06e9aa4..741e4ff63992 100644
@@ -1302,7 +1302,8 @@ static int ext4_write_end(struct file *file,
 
 	if (ext4_has_inline_data(inode) &&
 	    ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
-		return ext4_write_inline_data_end(inode, pos, len, copied, page);
+		return ext4_write_inline_data_end(inode, pos, len, copied,
+						  folio);
 
 	copied = block_write_end(file, mapping, pos, len, copied, page, fsdata);
 	/*

@@ -1410,7 +1411,8 @@ static int ext4_journalled_write_end(struct file *file,
 	BUG_ON(!ext4_handle_valid(handle));
 
 	if (ext4_has_inline_data(inode))
-		return ext4_write_inline_data_end(inode, pos, len, copied, page);
+		return ext4_write_inline_data_end(inode, pos, len, copied,
+						  folio);
 
 	if (unlikely(copied < len) && !folio_test_uptodate(folio)) {
 		copied = 0;

@@ -2966,15 +2968,15 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
  * Check if we should update i_disksize
  * when write to the end of file but not require block allocation
  */
-static int ext4_da_should_update_i_disksize(struct page *page,
+static int ext4_da_should_update_i_disksize(struct folio *folio,
 					    unsigned long offset)
 {
 	struct buffer_head *bh;
-	struct inode *inode = page->mapping->host;
+	struct inode *inode = folio->mapping->host;
 	unsigned int idx;
 	int i;
 
-	bh = page_buffers(page);
+	bh = folio_buffers(folio);
 	idx = offset >> inode->i_blkbits;
 
 	for (i = 0; i < idx; i++)

@@ -2994,17 +2996,19 @@ static int ext4_da_write_end(struct file *file,
 	loff_t new_i_size;
 	unsigned long start, end;
 	int write_mode = (int)(unsigned long)fsdata;
+	struct folio *folio = page_folio(page);
 
 	if (write_mode == FALL_BACK_TO_NONDELALLOC)
 		return ext4_write_end(file, mapping, pos,
-				      len, copied, page, fsdata);
+				      len, copied, &folio->page, fsdata);
 
 	trace_ext4_da_write_end(inode, pos, len, copied);
 
 	if (write_mode != CONVERT_INLINE_DATA &&
 	    ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA) &&
 	    ext4_has_inline_data(inode))
-		return ext4_write_inline_data_end(inode, pos, len, copied, page);
+		return ext4_write_inline_data_end(inode, pos, len, copied,
+						  folio);
 
 	start = pos & (PAGE_SIZE - 1);
 	end = start + copied - 1;

@@ -3025,10 +3029,11 @@ static int ext4_da_write_end(struct file *file,
 	 */
 	new_i_size = pos + copied;
 	if (copied && new_i_size > inode->i_size &&
-	    ext4_da_should_update_i_disksize(page, end))
+	    ext4_da_should_update_i_disksize(folio, end))
 		ext4_update_i_disksize(inode, new_i_size);
 
-	return generic_write_end(file, mapping, pos, len, copied, page, fsdata);
+	return generic_write_end(file, mapping, pos, len, copied, &folio->page,
+				 fsdata);
 }
 
 /*
Signature · 1 line
2.39.2
Reviewed-by on v1 1/4
#2 lore raw

Re: [RFCv1 1/4] ext4: kill unused function ext4_journalled_write_inline_data

In reply to Ritesh Harjani (IBM)
Quoted 8 lines · Ritesh Harjani (IBM)

Commit 3f079114bf522 ("ext4: Convert data=journal writeback to use ext4_writepages()") Added support for writeback of journalled data into ext4_writepages() and killed function __ext4_journalled_writepage() which used to call ext4_journalled_write_inline_data() for inline data.
This function got left over by mistake. Hence kill it's definition as
no one uses it.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
Reviewed-by Matthew Wilcox (Oracle) <[email protected]>
Reviewed-by on v1 2/4
#4 lore raw

Re: [RFCv1 2/4] ext4: Change remaining tracepoints to use folio

In reply to Ritesh Harjani (IBM)
Quoted 6 lines · Ritesh Harjani (IBM)

est4_readpage() is converted to ext4_read_folio() hence change the related tracepoint from trace_ext4_readpage(page) to trace_ext4_read_folio(folio).

Do the same for trace_ext4_releasepage(page) to
trace_ext4_release_folio(folio)

Thanks for doing this!

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

One possible enhancement is that each fo these functions does folio->mapping->host already. We could change ext4__folio_op to take (inode, folio) instead of just folio and simplify/remove some dereferences.

Quoted 89 lines
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext4/inode.c             |  4 ++--
 include/trace/events/ext4.h | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)
fs/ext4/inode.c +2 -2
index 6d628d6c0847..5bb141288b1b 100644
@@ -3157,7 +3157,7 @@ static int ext4_read_folio(struct file *file, struct folio *folio)
 	int ret = -EAGAIN;
 	struct inode *inode = folio->mapping->host;
 
-	trace_ext4_readpage(&folio->page);
+	trace_ext4_read_folio(folio);
 
 	if (ext4_has_inline_data(inode))
 		ret = ext4_readpage_inline(inode, folio);

@@ -3218,7 +3218,7 @@ static bool ext4_release_folio(struct folio *folio, gfp_t wait)
 {
 	journal_t *journal = EXT4_JOURNAL(folio->mapping->host);
 
-	trace_ext4_releasepage(&folio->page);
+	trace_ext4_release_folio(folio);
 
 	/* Page has dirty journalled data -> cannot release */
 	if (folio_test_checked(folio))
include/trace/events/ext4.h +13 -13
index ebccf6a6aa1b..a9415f1c68ec 100644
@@ -560,10 +560,10 @@ TRACE_EVENT(ext4_writepages_result,
 		  (unsigned long) __entry->writeback_index)
 );
 
-DECLARE_EVENT_CLASS(ext4__page_op,
-	TP_PROTO(struct page *page),
+DECLARE_EVENT_CLASS(ext4__folio_op,
+	TP_PROTO(struct folio *folio),
 
-	TP_ARGS(page),
+	TP_ARGS(folio),
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)

@@ -573,29 +573,29 @@ DECLARE_EVENT_CLASS(ext4__page_op,
 	),
 
 	TP_fast_assign(
-		__entry->dev	= page->mapping->host->i_sb->s_dev;
-		__entry->ino	= page->mapping->host->i_ino;
-		__entry->index	= page->index;
+		__entry->dev	= folio->mapping->host->i_sb->s_dev;
+		__entry->ino	= folio->mapping->host->i_ino;
+		__entry->index	= folio->index;
 	),
 
-	TP_printk("dev %d,%d ino %lu page_index %lu",
+	TP_printk("dev %d,%d ino %lu folio_index %lu",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  (unsigned long) __entry->ino,
 		  (unsigned long) __entry->index)
 );
 
-DEFINE_EVENT(ext4__page_op, ext4_readpage,
+DEFINE_EVENT(ext4__folio_op, ext4_read_folio,
 
-	TP_PROTO(struct page *page),
+	TP_PROTO(struct folio *folio),
 
-	TP_ARGS(page)
+	TP_ARGS(folio)
 );
 
-DEFINE_EVENT(ext4__page_op, ext4_releasepage,
+DEFINE_EVENT(ext4__folio_op, ext4_release_folio,
 
-	TP_PROTO(struct page *page),
+	TP_PROTO(struct folio *folio),
 
-	TP_ARGS(page)
+	TP_ARGS(folio)
 );
 
 DECLARE_EVENT_CLASS(ext4_invalidate_folio_op,
Signature · 1 line
2.39.2
#6 lore raw

Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

In reply to Ritesh Harjani (IBM)
Quoted 2 lines · Ritesh Harjani (IBM)

This patch converts mpage_journal_page_buffers() to use folio and also removes the PAGE_SIZE assumption.

Bit of an oversight on my part. I neglected to do this after Jan added it. Perils of parallel development ...

fs/ext4/inode.c static bool ext4_folio_nomap_can_writeout(struct folio *folio) · line 2353

Quoted 4 lines
fs/ext4/inode.c +2 -2
-static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
-				     int len)
+static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
+				     size_t len)

Should this be called ext4_journal_folio_buffers?

Reviewed-by on v1 4/4
#10 lore raw

Re: [RFCv1 4/4] ext4: Make ext4_write_inline_data_end() use folio

In reply to Ritesh Harjani (IBM)
Quoted 9 lines · Ritesh Harjani (IBM)

ext4_write_inline_data_end() is completely converted to work with folio. Also all callers of ext4_write_inline_data_end() already works on folio except ext4_da_write_end(). Mostly for consistency and saving few instructions maybe, this patch just converts ext4_da_write_end() to work with folio which makes the last caller of ext4_write_inline_data_end() also converted to work with folio.
We then make ext4_write_inline_data_end() take folio instead of page.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
Reviewed-by Matthew Wilcox (Oracle) <[email protected]>
author reviewer
#7 lore raw

Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

In reply to Matthew Wilcox
Quoted 7 lines · Matthew Wilcox
Quoted 2 lines · Ritesh Harjani (IBM)

This patch converts mpage_journal_page_buffers() to use folio and also removes the PAGE_SIZE assumption.

Bit of an oversight on my part. I neglected to do this after Jan added it. Perils of parallel development ...

Yes, these got left overs because of the parallel series.

Quoted 6 lines

fs/ext4/inode.c static bool ext4_folio_nomap_can_writeout(struct folio *folio) · line 2353

Quoted 4 lines
fs/ext4/inode.c +2 -2
-static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
-				     int len)
+static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
+				     size_t len)

Should this be called ext4_journal_folio_buffers?

Sure. Will make the change. Otherwise this patch looks good to you?
I also had a query regarding setting "len = size - folio_pos(folio)" in this patch. Details of which I had pasted in the cover letter. Let me copy-paste it here from the cover letter. Could you please take a look at it?

<copy-paste>
Also had a query w.r.t your change [1]. I couldn't understand this change diff from [1]. Given if we are making the conversion to folio, then shouldn't we do len = size - folio_pos(pos), instead of len = size & ~PAGE_MASK Could you please tell if the current change in [1] is kept deliberately? At other places you did make len as size - folio_pos(pos) which removes the PAGE_SIZE assumption.

-static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page) +static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
{
- int len;
+ size_t len;

	<...>

	size = i_size_read(mpd->inode);

- if (page->index == size >> PAGE_SHIFT &&
+ len = folio_size(folio);
+ if (folio_pos(folio) + len > size &&
!ext4_verity_in_progress(mpd->inode))
len = size & ~PAGE_MASK;
- else
- len = PAGE_SIZE;
- err = ext4_bio_write_page(&mpd->io_submit, page, len);
+ err = ext4_bio_write_page(&mpd->io_submit, &folio->page, len);
if (!err)
mpd->wbc->nr_to_write--;

[1]: https://lore.kernel.org/linux-ext4/[email protected]/

Thanks for the quick review!

-ritesh

author reviewer
#8 lore raw

Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

Quoted 54 lines · Ritesh Harjani (IBM)
Quoted 7 lines · Matthew Wilcox
Quoted 2 lines · Ritesh Harjani (IBM)

This patch converts mpage_journal_page_buffers() to use folio and also removes the PAGE_SIZE assumption.

Bit of an oversight on my part. I neglected to do this after Jan added it. Perils of parallel development ...

Yes, these got left overs because of the parallel series.

Quoted 6 lines

fs/ext4/inode.c static bool ext4_folio_nomap_can_writeout(struct folio *folio) · line 2353

Quoted 4 lines
fs/ext4/inode.c +2 -2
-static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
-				     int len)
+static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
+				     size_t len)

Should this be called ext4_journal_folio_buffers?

Sure. Will make the change. Otherwise this patch looks good to you?
I also had a query regarding setting "len = size - folio_pos(folio)" in this patch. Details of which I had pasted in the cover letter. Let me copy-paste it here from the cover letter. Could you please take a look at it?

<copy-paste>
Also had a query w.r.t your change [1]. I couldn't understand this change diff from [1]. Given if we are making the conversion to folio, then shouldn't we do len = size - folio_pos(pos), instead of len = size & ~PAGE_MASK Could you please tell if the current change in [1] is kept deliberately? At other places you did make len as size - folio_pos(pos) which removes the PAGE_SIZE assumption.

fs/ext4/inode.c +2 -2
-static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
+static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
 {
-	int len;
+	size_t len;
 
<...>

	size = i_size_read(mpd->inode);
(quoted diff) +3 -4
-	if (page->index == size >> PAGE_SHIFT &&
+	len = folio_size(folio);
+	if (folio_pos(folio) + len > size &&
 	    !ext4_verity_in_progress(mpd->inode))
 		len = size & ~PAGE_MASK;
-	else
-		len = PAGE_SIZE;
-	err = ext4_bio_write_page(&mpd->io_submit, page, len);
+	err = ext4_bio_write_page(&mpd->io_submit, &folio->page, len);
 	if (!err)
 		mpd->wbc->nr_to_write--;
Quoted 1 line

[1]: https://lore.kernel.org/linux-ext4/[email protected]/

Here is the complete function. Looking at it again, I think we should make len = size - folio_pos(folio) (at linenumber 26, like how it is done at other places in ext4-folio patches), because we now call ext4_bio_write_folio() instead of ext4_bio_write_page().

Although I know it doesn't make a difference in the functionality today since folio_size(folio) today in case of ext4 is still PAGE_SIZE.

Please let me know if this understanding is correct. If yes, then I can write a patch to make len = size - folio_pos(folio) at line 26.

If not I will be happy to know more about what am I missing.

1 static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio) 2 {

 3         size_t len;
 4         loff_t size;
 5         int err;
 6
 7         BUG_ON(folio->index != mpd->first_page);
 8         folio_clear_dirty_for_io(folio);
 9         /*
10          * We have to be very careful here!  Nothing protects writeback path
11          * against i_size changes and the page can be writeably mapped into
12          * page tables. So an application can be growing i_size and writing
13          * data through mmap while writeback runs. folio_clear_dirty_for_io()
14          * write-protects our page in page tables and the page cannot get
15          * written to again until we release folio lock. So only after
16          * folio_clear_dirty_for_io() we are safe to sample i_size for
17          * ext4_bio_write_folio() to zero-out tail of the written page. We rely
18          * on the barrier provided by folio_test_clear_dirty() in
19          * folio_clear_dirty_for_io() to make sure i_size is really sampled only
20          * after page tables are updated.
21          */
22         size = i_size_read(mpd->inode);
23         len = folio_size(folio);
24         if (folio_pos(folio) + len > size &&

25 !ext4_verity_in_progress(mpd->inode))
26 len = size & ~PAGE_MASK;
27 err = ext4_bio_write_folio(&mpd->io_submit, folio, len);
28 if (!err)
29 mpd->wbc->nr_to_write--;
30
31 return err;
32 }

Thanks a lot!!
-ritesh

author reviewer
#12 lore raw
Quoted 6 lines · Theodore Ts'o

Hey Ritesh,

My understanding is that you are intending on sending a revised version of this patch set; is that correct? Thanks!!

- Ted

Yes, Ted. I had some open questions to willy mainly in Patch-3. Let me rebase this and will put the queries in the rfcv2.

-ritesh