[RFCv1 0/4] ext4: misc left over folio changes

[RFCv1 0/4] ext4: misc left over folio changes

[Ritesh Harjani (IBM)]

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/20230324180129.1220691-7-willy@infradead.org/


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

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

[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) <ritesh.list@gmail.com>
---
 fs/ext4/ext4.h   |  4 ----
 fs/ext4/inline.c | 24 ------------------------
 2 files changed, 28 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 83f0cc02250f..b00597b41c96 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -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,
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index b9fb1177fff6..062c7747bb40 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -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:
-- 
2.39.2

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

[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) <ritesh.list@gmail.com>
---
 fs/ext4/inode.c             |  4 ++--
 include/trace/events/ext4.h | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 6d628d6c0847..5bb141288b1b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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))
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index ebccf6a6aa1b..a9415f1c68ec 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -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,
-- 
2.39.2

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

[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) <ritesh.list@gmail.com>
---
 fs/ext4/inode.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5bb141288b1b..3f76b06e9aa4 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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);
-- 
2.39.2

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

[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) <ritesh.list@gmail.com>
---
 fs/ext4/ext4.h   |  6 ++----
 fs/ext4/inline.c |  3 +--
 fs/ext4/inode.c  | 23 ++++++++++++++---------
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b00597b41c96..081f04ea2be0 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -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,
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 062c7747bb40..eee78035c12d 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -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;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3f76b06e9aa4..741e4ff63992 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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);
 }
 
 /*
-- 
2.39.2

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

[Matthew Wilcox]

On Mon, Apr 17, 2023 at 12:01:50AM +0530, Ritesh Harjani (IBM) wrote:
> 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) <ritesh.list@gmail.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

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

[Matthew Wilcox]

On Mon, Apr 17, 2023 at 12:01:51AM +0530, Ritesh Harjani (IBM) wrote:
> 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) <willy@infradead.org>

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.

> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/ext4/inode.c             |  4 ++--
>  include/trace/events/ext4.h | 26 +++++++++++++-------------
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6d628d6c0847..5bb141288b1b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -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))
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index ebccf6a6aa1b..a9415f1c68ec 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -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,
> -- 
> 2.39.2
> 

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

[Matthew Wilcox]

On Mon, Apr 17, 2023 at 12:01:52AM +0530, Ritesh Harjani (IBM) wrote:
> 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 ...

> -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?

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

[Matthew Wilcox]

On Mon, Apr 17, 2023 at 12:01:53AM +0530, Ritesh Harjani (IBM) wrote:
> 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) <ritesh.list@gmail.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

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

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Mon, Apr 17, 2023 at 12:01:52AM +0530, Ritesh Harjani (IBM) wrote:
>> 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.

>> -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/20230324180129.1220691-7-willy@infradead.org/


Thanks for the quick review!

-ritesh

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

[Ritesh Harjani]

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

> Matthew Wilcox <willy@infradead.org> writes:
>
>> On Mon, Apr 17, 2023 at 12:01:52AM +0530, Ritesh Harjani (IBM) wrote:
>>> 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.
>
>>> -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/20230324180129.1220691-7-willy@infradead.org/

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

Re: [RFCv1 0/4] ext4: misc left over folio changes

[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

Re: [RFCv1 0/4] ext4: misc left over folio changes

[Ritesh Harjani]

"Theodore Ts'o" <tytso@mit.edu> writes:

> 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