[PATCH 0/9] ext4: Stop using ext4_writepage() for writeout of ordered data

[PATCH 0/9] ext4: Stop using ext4_writepage() for writeout of ordered data

[Jan Kara]

Hello,

this patch series modifies ext4 so that we stop using ext4_writepage() for
writeout of ordered data during transaction commit (through
generic_writepages() from jbd2_journal_submit_inode_data_buffers()). Instead we
directly call ext4_writepages() from the
ext4_journal_submit_inode_data_buffers(). This is part of Christoph's effort
to get rid of the .writepage() callback in all filesystems.

Note that we still keep ext4_writepage() function for writeout of journalled
data. That can be also dealt with but let's untangle things one by one.  Also
note that jbd2 still uses generic_writepages() in its
jbd2_journal_submit_inode_data_buffers() helper because it is still used from
OCFS2. Again, something to be dealt with in a separate patchset.

Patches pass for me tests on both 1k and 4k filesystems with dioreadlock and
without it.

								Honza

[PATCH 1/9] ext4: Handle redirtying in ext4_bio_write_page()

[Jan Kara]

Since we want to transition transaction commits to use ext4_writepages()
for writing back ordered, add handling of page redirtying into
ext4_bio_write_page(). Also move buffer dirty bit clearing into the same
place other buffer state handling.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/page-io.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 97fa7b4c645f..4e68ace86f11 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -482,6 +482,13 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 			/* A hole? We can safely clear the dirty bit */
 			if (!buffer_mapped(bh))
 				clear_buffer_dirty(bh);
+			/*
+			 * Keeping dirty some buffer we cannot write? Make
+			 * sure to redirty the page. This happens e.g. when
+			 * doing writeout for transaction commit.
+			 */
+			if (buffer_dirty(bh) && !PageDirty(page))
+				redirty_page_for_writepage(wbc, page);
 			if (io->io_bio)
 				ext4_io_submit(io);
 			continue;
@@ -489,6 +496,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		if (buffer_new(bh))
 			clear_buffer_new(bh);
 		set_buffer_async_write(bh);
+		clear_buffer_dirty(bh);
 		nr_to_submit++;
 	} while ((bh = bh->b_this_page) != head);
 
@@ -532,7 +540,10 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 			printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
 			redirty_page_for_writepage(wbc, page);
 			do {
-				clear_buffer_async_write(bh);
+				if (buffer_async_write(bh)) {
+					clear_buffer_async_write(bh);
+					set_buffer_dirty(bh);
+				}
 				bh = bh->b_this_page;
 			} while (bh != head);
 			goto unlock;
@@ -546,7 +557,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		io_submit_add_bh(io, inode,
 				 bounce_page ? bounce_page : page, bh);
 		nr_submitted++;
-		clear_buffer_dirty(bh);
 	} while ((bh = bh->b_this_page) != head);
 
 unlock:
-- 
2.35.3

Re: [PATCH 1/9] ext4: Handle redirtying in ext4_bio_write_page()

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> Since we want to transition transaction commits to use ext4_writepages()
> for writing back ordered, add handling of page redirtying into
> ext4_bio_write_page(). Also move buffer dirty bit clearing into the same
> place other buffer state handling.
>

So when we will move away from ext4_writepage() and will instead call
ext4_writepages() (for transaction commits requiring ordered write handling),
this patch should help with redirtying the page, if any of the page buffer
cannot be written back which is when either the buffer is either marked delayed
(which will require block allocation) or is unwritten (which may also
require some block allocation during unwritten to written conversion).

Also, one other good thing about this patch is, we don't have to loop over all
the buffers seperately to identify whether a page needs to be set redirty or not.
With this change we will redirty the page in the same loop (if required)
and identify all the mapped buffers for writeback.


Moving the clearing of buffer dirty state also looks right to me.

Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/page-io.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 97fa7b4c645f..4e68ace86f11 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -482,6 +482,13 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  			/* A hole? We can safely clear the dirty bit */
>  			if (!buffer_mapped(bh))
>  				clear_buffer_dirty(bh);
> +			/*
> +			 * Keeping dirty some buffer we cannot write? Make
> +			 * sure to redirty the page. This happens e.g. when
> +			 * doing writeout for transaction commit.
> +			 */
> +			if (buffer_dirty(bh) && !PageDirty(page))
> +				redirty_page_for_writepage(wbc, page);
>  			if (io->io_bio)
>  				ext4_io_submit(io);
>  			continue;
> @@ -489,6 +496,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  		if (buffer_new(bh))
>  			clear_buffer_new(bh);
>  		set_buffer_async_write(bh);
> +		clear_buffer_dirty(bh);
>  		nr_to_submit++;
>  	} while ((bh = bh->b_this_page) != head);
>
> @@ -532,7 +540,10 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  			printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
>  			redirty_page_for_writepage(wbc, page);
>  			do {
> -				clear_buffer_async_write(bh);
> +				if (buffer_async_write(bh)) {
> +					clear_buffer_async_write(bh);
> +					set_buffer_dirty(bh);
> +				}
>  				bh = bh->b_this_page;
>  			} while (bh != head);
>  			goto unlock;
> @@ -546,7 +557,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  		io_submit_add_bh(io, inode,
>  				 bounce_page ? bounce_page : page, bh);
>  		nr_submitted++;
> -		clear_buffer_dirty(bh);
>  	} while ((bh = bh->b_this_page) != head);
>
>  unlock:
> --
> 2.35.3
>

[PATCH 2/9] ext4: Move keep_towrite handling to ext4_bio_write_page()

[Jan Kara]

When we are writing back page but we cannot for some reason write all
its buffers (e.g. because we cannot allocate blocks in current context) we
have to keep TOWRITE tag set in the mapping as otherwise racing
WB_SYNC_ALL writeback that could write these buffers can skip the page
and result in data loss. We will need this logic for writeback during
transaction commit so move the logic from ext4_writepage() to
ext4_bio_write_page().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h    |  3 +--
 fs/ext4/inode.c   |  6 ++----
 fs/ext4/page-io.c | 36 +++++++++++++++++++++---------------
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8d5453852f98..1b3bffc04fd0 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3756,8 +3756,7 @@ extern void ext4_end_io_rsv_work(struct work_struct *work);
 extern void ext4_io_submit(struct ext4_io_submit *io);
 extern int ext4_bio_write_page(struct ext4_io_submit *io,
 			       struct page *page,
-			       int len,
-			       bool keep_towrite);
+			       int len);
 extern struct ext4_io_end_vec *ext4_alloc_io_end_vec(ext4_io_end_t *io_end);
 extern struct ext4_io_end_vec *ext4_last_io_end_vec(ext4_io_end_t *io_end);
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2b5ef1b64249..43eb175d0c1c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2009,7 +2009,6 @@ static int ext4_writepage(struct page *page,
 	struct buffer_head *page_bufs = NULL;
 	struct inode *inode = page->mapping->host;
 	struct ext4_io_submit io_submit;
-	bool keep_towrite = false;
 
 	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
 		folio_invalidate(folio, 0, folio_size(folio));
@@ -2067,7 +2066,6 @@ static int ext4_writepage(struct page *page,
 			unlock_page(page);
 			return 0;
 		}
-		keep_towrite = true;
 	}
 
 	if (PageChecked(page) && ext4_should_journal_data(inode))
@@ -2084,7 +2082,7 @@ static int ext4_writepage(struct page *page,
 		unlock_page(page);
 		return -ENOMEM;
 	}
-	ret = ext4_bio_write_page(&io_submit, page, len, keep_towrite);
+	ret = ext4_bio_write_page(&io_submit, page, len);
 	ext4_io_submit(&io_submit);
 	/* Drop io_end reference we got from init */
 	ext4_put_io_end_defer(io_submit.io_end);
@@ -2118,7 +2116,7 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
 		len = size & ~PAGE_MASK;
 	else
 		len = PAGE_SIZE;
-	err = ext4_bio_write_page(&mpd->io_submit, page, len, false);
+	err = ext4_bio_write_page(&mpd->io_submit, page, len);
 	if (!err)
 		mpd->wbc->nr_to_write--;
 	mpd->first_page++;
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 4e68ace86f11..4f9ecacd10aa 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -430,8 +430,7 @@ static void io_submit_add_bh(struct ext4_io_submit *io,
 
 int ext4_bio_write_page(struct ext4_io_submit *io,
 			struct page *page,
-			int len,
-			bool keep_towrite)
+			int len)
 {
 	struct page *bounce_page = NULL;
 	struct inode *inode = page->mapping->host;
@@ -441,14 +440,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	int nr_submitted = 0;
 	int nr_to_submit = 0;
 	struct writeback_control *wbc = io->io_wbc;
+	bool keep_towrite = false;
 
 	BUG_ON(!PageLocked(page));
 	BUG_ON(PageWriteback(page));
 
-	if (keep_towrite)
-		set_page_writeback_keepwrite(page);
-	else
-		set_page_writeback(page);
 	ClearPageError(page);
 
 	/*
@@ -483,12 +479,17 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 			if (!buffer_mapped(bh))
 				clear_buffer_dirty(bh);
 			/*
-			 * Keeping dirty some buffer we cannot write? Make
-			 * sure to redirty the page. This happens e.g. when
-			 * doing writeout for transaction commit.
+			 * Keeping dirty some buffer we cannot write? Make sure
+			 * to redirty the page and keep TOWRITE tag so that
+			 * racing WB_SYNC_ALL writeback does not skip the page.
+			 * This happens e.g. when doing writeout for
+			 * transaction commit.
 			 */
-			if (buffer_dirty(bh) && !PageDirty(page))
-				redirty_page_for_writepage(wbc, page);
+			if (buffer_dirty(bh)) {
+				if (!PageDirty(page))
+					redirty_page_for_writepage(wbc, page);
+				keep_towrite = true;
+			}
 			if (io->io_bio)
 				ext4_io_submit(io);
 			continue;
@@ -500,6 +501,10 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		nr_to_submit++;
 	} while ((bh = bh->b_this_page) != head);
 
+	/* Nothing to submit? Just unlock the page... */
+	if (!nr_to_submit)
+		goto unlock;
+
 	bh = head = page_buffers(page);
 
 	/*
@@ -550,6 +555,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		}
 	}
 
+	if (keep_towrite)
+		set_page_writeback_keepwrite(page);
+	else
+		set_page_writeback(page);
+
 	/* Now submit buffers to write */
 	do {
 		if (!buffer_async_write(bh))
@@ -558,11 +568,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 				 bounce_page ? bounce_page : page, bh);
 		nr_submitted++;
 	} while ((bh = bh->b_this_page) != head);
-
 unlock:
 	unlock_page(page);
-	/* Nothing submitted - we have to end page writeback */
-	if (!nr_submitted)
-		end_page_writeback(page);
 	return ret;
 }
-- 
2.35.3

Re: [PATCH 2/9] ext4: Move keep_towrite handling to ext4_bio_write_page()

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> When we are writing back page but we cannot for some reason write all
> its buffers (e.g. because we cannot allocate blocks in current context) we
> have to keep TOWRITE tag set in the mapping as otherwise racing
> WB_SYNC_ALL writeback that could write these buffers can skip the page
> and result in data loss. We will need this logic for writeback during
> transaction commit so move the logic from ext4_writepage() to
> ext4_bio_write_page().


Nice explaination.
Moving set_page_writeback() and set_page_writeback_keepwrite() to after
identifying any buffers needs to be written back or not is also sweet.
This avoid unnecessary calling of end_page_writeback().

The patch looks good to me. Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>



>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/ext4.h    |  3 +--
>  fs/ext4/inode.c   |  6 ++----
>  fs/ext4/page-io.c | 36 +++++++++++++++++++++---------------
>  3 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8d5453852f98..1b3bffc04fd0 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3756,8 +3756,7 @@ extern void ext4_end_io_rsv_work(struct work_struct *work);
>  extern void ext4_io_submit(struct ext4_io_submit *io);
>  extern int ext4_bio_write_page(struct ext4_io_submit *io,
>  			       struct page *page,
> -			       int len,
> -			       bool keep_towrite);
> +			       int len);
>  extern struct ext4_io_end_vec *ext4_alloc_io_end_vec(ext4_io_end_t *io_end);
>  extern struct ext4_io_end_vec *ext4_last_io_end_vec(ext4_io_end_t *io_end);
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 2b5ef1b64249..43eb175d0c1c 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2009,7 +2009,6 @@ static int ext4_writepage(struct page *page,
>  	struct buffer_head *page_bufs = NULL;
>  	struct inode *inode = page->mapping->host;
>  	struct ext4_io_submit io_submit;
> -	bool keep_towrite = false;
>
>  	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
>  		folio_invalidate(folio, 0, folio_size(folio));
> @@ -2067,7 +2066,6 @@ static int ext4_writepage(struct page *page,
>  			unlock_page(page);
>  			return 0;
>  		}
> -		keep_towrite = true;
>  	}
>
>  	if (PageChecked(page) && ext4_should_journal_data(inode))
> @@ -2084,7 +2082,7 @@ static int ext4_writepage(struct page *page,
>  		unlock_page(page);
>  		return -ENOMEM;
>  	}
> -	ret = ext4_bio_write_page(&io_submit, page, len, keep_towrite);
> +	ret = ext4_bio_write_page(&io_submit, page, len);
>  	ext4_io_submit(&io_submit);
>  	/* Drop io_end reference we got from init */
>  	ext4_put_io_end_defer(io_submit.io_end);
> @@ -2118,7 +2116,7 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
>  		len = size & ~PAGE_MASK;
>  	else
>  		len = PAGE_SIZE;
> -	err = ext4_bio_write_page(&mpd->io_submit, page, len, false);
> +	err = ext4_bio_write_page(&mpd->io_submit, page, len);
>  	if (!err)
>  		mpd->wbc->nr_to_write--;
>  	mpd->first_page++;
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 4e68ace86f11..4f9ecacd10aa 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -430,8 +430,7 @@ static void io_submit_add_bh(struct ext4_io_submit *io,
>
>  int ext4_bio_write_page(struct ext4_io_submit *io,
>  			struct page *page,
> -			int len,
> -			bool keep_towrite)
> +			int len)
>  {
>  	struct page *bounce_page = NULL;
>  	struct inode *inode = page->mapping->host;
> @@ -441,14 +440,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  	int nr_submitted = 0;
>  	int nr_to_submit = 0;
>  	struct writeback_control *wbc = io->io_wbc;
> +	bool keep_towrite = false;
>
>  	BUG_ON(!PageLocked(page));
>  	BUG_ON(PageWriteback(page));
>
> -	if (keep_towrite)
> -		set_page_writeback_keepwrite(page);
> -	else
> -		set_page_writeback(page);
>  	ClearPageError(page);
>
>  	/*
> @@ -483,12 +479,17 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  			if (!buffer_mapped(bh))
>  				clear_buffer_dirty(bh);
>  			/*
> -			 * Keeping dirty some buffer we cannot write? Make
> -			 * sure to redirty the page. This happens e.g. when
> -			 * doing writeout for transaction commit.
> +			 * Keeping dirty some buffer we cannot write? Make sure
> +			 * to redirty the page and keep TOWRITE tag so that
> +			 * racing WB_SYNC_ALL writeback does not skip the page.
> +			 * This happens e.g. when doing writeout for
> +			 * transaction commit.
>  			 */
> -			if (buffer_dirty(bh) && !PageDirty(page))
> -				redirty_page_for_writepage(wbc, page);
> +			if (buffer_dirty(bh)) {
> +				if (!PageDirty(page))
> +					redirty_page_for_writepage(wbc, page);
> +				keep_towrite = true;
> +			}
>  			if (io->io_bio)
>  				ext4_io_submit(io);
>  			continue;
> @@ -500,6 +501,10 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  		nr_to_submit++;
>  	} while ((bh = bh->b_this_page) != head);
>
> +	/* Nothing to submit? Just unlock the page... */
> +	if (!nr_to_submit)
> +		goto unlock;
> +
>  	bh = head = page_buffers(page);
>
>  	/*
> @@ -550,6 +555,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  		}
>  	}
>
> +	if (keep_towrite)
> +		set_page_writeback_keepwrite(page);
> +	else
> +		set_page_writeback(page);
> +
>  	/* Now submit buffers to write */
>  	do {
>  		if (!buffer_async_write(bh))
> @@ -558,11 +568,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  				 bounce_page ? bounce_page : page, bh);
>  		nr_submitted++;
>  	} while ((bh = bh->b_this_page) != head);
> -
>  unlock:
>  	unlock_page(page);
> -	/* Nothing submitted - we have to end page writeback */
> -	if (!nr_submitted)
> -		end_page_writeback(page);
>  	return ret;
>  }
> --
> 2.35.3
>

[PATCH 3/9] ext4: Remove nr_submitted from ext4_bio_write_page()

[Jan Kara]

nr_submitted is the same as nr_to_submit. Drop one of them.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/page-io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 4f9ecacd10aa..2bdfb8a046d9 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -437,7 +437,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	unsigned block_start;
 	struct buffer_head *bh, *head;
 	int ret = 0;
-	int nr_submitted = 0;
 	int nr_to_submit = 0;
 	struct writeback_control *wbc = io->io_wbc;
 	bool keep_towrite = false;
@@ -566,7 +565,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 			continue;
 		io_submit_add_bh(io, inode,
 				 bounce_page ? bounce_page : page, bh);
-		nr_submitted++;
 	} while ((bh = bh->b_this_page) != head);
 unlock:
 	unlock_page(page);
-- 
2.35.3

Re: [PATCH 3/9] ext4: Remove nr_submitted from ext4_bio_write_page()

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> nr_submitted is the same as nr_to_submit. Drop one of them.

Yup. Since we are not calling end_page_writeback() anymore.
So we don't require nr_submitted too.

Looks good to me. Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>



>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/page-io.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 4f9ecacd10aa..2bdfb8a046d9 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -437,7 +437,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  	unsigned block_start;
>  	struct buffer_head *bh, *head;
>  	int ret = 0;
> -	int nr_submitted = 0;
>  	int nr_to_submit = 0;
>  	struct writeback_control *wbc = io->io_wbc;
>  	bool keep_towrite = false;
> @@ -566,7 +565,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  			continue;
>  		io_submit_add_bh(io, inode,
>  				 bounce_page ? bounce_page : page, bh);
> -		nr_submitted++;
>  	} while ((bh = bh->b_this_page) != head);
>  unlock:
>  	unlock_page(page);
> --
> 2.35.3
>

[PATCH 4/9] ext4: Drop pointless IO submission from ext4_bio_write_page()

[Jan Kara]

We submit outstanding IO in ext4_bio_write_page() if we find a buffer we
are not going to write. This is however pointless because we already
handle submission of previous IO in case we detect newly added buffer
head is discontiguous. So just delete the pointless IO submission call.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/page-io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 2bdfb8a046d9..beaec6d81074 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -489,8 +489,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 					redirty_page_for_writepage(wbc, page);
 				keep_towrite = true;
 			}
-			if (io->io_bio)
-				ext4_io_submit(io);
 			continue;
 		}
 		if (buffer_new(bh))
-- 
2.35.3

Re: [PATCH 4/9] ext4: Drop pointless IO submission from ext4_bio_write_page()

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> We submit outstanding IO in ext4_bio_write_page() if we find a buffer we
> are not going to write. This is however pointless because we already
> handle submission of previous IO in case we detect newly added buffer
> head is discontiguous. So just delete the pointless IO submission call.

Agreed. io_submit_add_bh() is anyway called at the end for submitting buffers.
And io_submit_add_bh() also has the logic to:
1. submit a discontiguous bio
2. Also submit a bio if the bio gets full (submit_and_retry label).

Hence calling ext4_io_submit() early is not required.

I guess the same will also hold true for at this place.
https://elixir.bootlin.com/linux/v6.1-rc7/source/fs/ext4/page-io.c#L524


But this patch looks good to me. Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>



>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/page-io.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 2bdfb8a046d9..beaec6d81074 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -489,8 +489,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  					redirty_page_for_writepage(wbc, page);
>  				keep_towrite = true;
>  			}
> -			if (io->io_bio)
> -				ext4_io_submit(io);
>  			continue;
>  		}
>  		if (buffer_new(bh))
> --
> 2.35.3
>

Re: [PATCH 4/9] ext4: Drop pointless IO submission from ext4_bio_write_page()

[Jan Kara]

On Thu 01-12-22 12:36:55, Ritesh Harjani (IBM) wrote:
> On 22/11/30 05:35PM, Jan Kara wrote:
> > We submit outstanding IO in ext4_bio_write_page() if we find a buffer we
> > are not going to write. This is however pointless because we already
> > handle submission of previous IO in case we detect newly added buffer
> > head is discontiguous. So just delete the pointless IO submission call.
> 
> Agreed. io_submit_add_bh() is anyway called at the end for submitting buffers.
> And io_submit_add_bh() also has the logic to:
> 1. submit a discontiguous bio
> 2. Also submit a bio if the bio gets full (submit_and_retry label).
> 
> Hence calling ext4_io_submit() early is not required.
> 
> I guess the same will also hold true for at this place.
> https://elixir.bootlin.com/linux/v6.1-rc7/source/fs/ext4/page-io.c#L524

So there the submission is needed because we are OOM and are going to wait
for some memory to free. If we have some bio accumulated, it is pinning
pages in writeback state and memory reclaim can be waiting on them. So if
we don't submit, it is a deadlock possibility or at least asking for
trouble.

> But this patch looks good to me. Feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thanks for review!

								Honza
> 
> 
> 
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext4/page-io.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> > index 2bdfb8a046d9..beaec6d81074 100644
> > --- a/fs/ext4/page-io.c
> > +++ b/fs/ext4/page-io.c
> > @@ -489,8 +489,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
> >  					redirty_page_for_writepage(wbc, page);
> >  				keep_towrite = true;
> >  			}
> > -			if (io->io_bio)
> > -				ext4_io_submit(io);
> >  			continue;
> >  		}
> >  		if (buffer_new(bh))
> > --
> > 2.35.3
> >
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/9] ext4: Drop pointless IO submission from ext4_bio_write_page()

[Ritesh Harjani (IBM)]

On 22/12/01 11:35AM, Jan Kara wrote:
> On Thu 01-12-22 12:36:55, Ritesh Harjani (IBM) wrote:
> > On 22/11/30 05:35PM, Jan Kara wrote:
> > > We submit outstanding IO in ext4_bio_write_page() if we find a buffer we
> > > are not going to write. This is however pointless because we already
> > > handle submission of previous IO in case we detect newly added buffer
> > > head is discontiguous. So just delete the pointless IO submission call.
> >
> > Agreed. io_submit_add_bh() is anyway called at the end for submitting buffers.
> > And io_submit_add_bh() also has the logic to:
> > 1. submit a discontiguous bio
> > 2. Also submit a bio if the bio gets full (submit_and_retry label).
> >
> > Hence calling ext4_io_submit() early is not required.
> >
> > I guess the same will also hold true for at this place.
> > https://elixir.bootlin.com/linux/v6.1-rc7/source/fs/ext4/page-io.c#L524
>
> So there the submission is needed because we are OOM and are going to wait
> for some memory to free. If we have some bio accumulated, it is pinning
> pages in writeback state and memory reclaim can be waiting on them. So if
> we don't submit, it is a deadlock possibility or at least asking for
> trouble.

Aah! right. I didn't see the ret == -ENOMEM compare there.

Thanks!
-ritesh

[PATCH 5/9] ext4: Add support for writepages calls that cannot map blocks

[Jan Kara]

Add support for calls to ext4_writepages() than cannot map blocks. These
will be issued from jbd2 transaction commit code.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 43eb175d0c1c..1cde20eb6500 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1557,6 +1557,7 @@ struct mpage_da_data {
 	struct ext4_map_blocks map;
 	struct ext4_io_submit io_submit;	/* IO submission data */
 	unsigned int do_map:1;
+	unsigned int can_map:1;	/* Can writepages call map blocks? */
 	unsigned int scanned_until_end:1;
 };
 
@@ -2549,6 +2550,19 @@ static int ext4_da_writepages_trans_blocks(struct inode *inode)
 				MAX_WRITEPAGES_EXTENT_LEN + bpp - 1, bpp);
 }
 
+/* Return true if the page needs to be written as part of transaction commit */
+static bool ext4_page_nomap_can_writeout(struct page *page)
+{
+	struct buffer_head *bh, *head;
+
+	bh = head = page_buffers(page);
+	do {
+		if (buffer_dirty(bh) && buffer_mapped(bh) && !buffer_delay(bh))
+			return true;
+	} while ((bh = bh->b_this_page) != head);
+	return false;
+}
+
 /*
  * mpage_prepare_extent_to_map - find & lock contiguous range of dirty pages
  * 				 and underlying extent to map
@@ -2651,14 +2665,30 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 			if (mpd->map.m_len == 0)
 				mpd->first_page = page->index;
 			mpd->next_page = page->index + 1;
-			/* Add all dirty buffers to mpd */
-			lblk = ((ext4_lblk_t)page->index) <<
-				(PAGE_SHIFT - blkbits);
-			head = page_buffers(page);
-			err = mpage_process_page_bufs(mpd, head, head, lblk);
-			if (err <= 0)
-				goto out;
-			err = 0;
+			/*
+			 * Writeout for transaction commit where we cannot
+			 * modify metadata is simple. Just submit the page.
+			 */
+			if (!mpd->can_map) {
+				if (ext4_page_nomap_can_writeout(page)) {
+					err = mpage_submit_page(mpd, page);
+					if (err < 0)
+						goto out;
+				} else {
+					unlock_page(page);
+					mpd->first_page++;
+				}
+			} else {
+				/* Add all dirty buffers to mpd */
+				lblk = ((ext4_lblk_t)page->index) <<
+					(PAGE_SHIFT - blkbits);
+				head = page_buffers(page);
+				err = mpage_process_page_bufs(mpd, head, head,
+							      lblk);
+				if (err <= 0)
+					goto out;
+				err = 0;
+			}
 			left--;
 		}
 		pagevec_release(&pvec);
@@ -2778,6 +2808,7 @@ static int ext4_writepages(struct address_space *mapping,
 	 */
 	mpd.do_map = 0;
 	mpd.scanned_until_end = 0;
+	mpd.can_map = 1;
 	mpd.io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
 	if (!mpd.io_submit.io_end) {
 		ret = -ENOMEM;
-- 
2.35.3

Re: [PATCH 5/9] ext4: Add support for writepages calls that cannot map blocks

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> Add support for calls to ext4_writepages() than cannot map blocks. These
> will be issued from jbd2 transaction commit code.

I guess we should expand the description of mpage_prepare_extent_to_map()
function now. Other than that the patch looks good to me.

Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c | 47 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 43eb175d0c1c..1cde20eb6500 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1557,6 +1557,7 @@ struct mpage_da_data {
>  	struct ext4_map_blocks map;
>  	struct ext4_io_submit io_submit;	/* IO submission data */
>  	unsigned int do_map:1;
> +	unsigned int can_map:1;	/* Can writepages call map blocks? */
>  	unsigned int scanned_until_end:1;
>  };
>
> @@ -2549,6 +2550,19 @@ static int ext4_da_writepages_trans_blocks(struct inode *inode)
>  				MAX_WRITEPAGES_EXTENT_LEN + bpp - 1, bpp);
>  }
>
> +/* Return true if the page needs to be written as part of transaction commit */
> +static bool ext4_page_nomap_can_writeout(struct page *page)
> +{
> +	struct buffer_head *bh, *head;
> +
> +	bh = head = page_buffers(page);
> +	do {
> +		if (buffer_dirty(bh) && buffer_mapped(bh) && !buffer_delay(bh))
> +			return true;
> +	} while ((bh = bh->b_this_page) != head);
> +	return false;
> +}
> +
>  /*
>   * mpage_prepare_extent_to_map - find & lock contiguous range of dirty pages
>   * 				 and underlying extent to map

Since we are overloading this function. this can be also called with can_map
as 0. Maybe good to add some description around that?

> @@ -2651,14 +2665,30 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)


adding context of code so that it doesn't get missed in the discussion.

<...>
			/* If we can't merge this page, we are done. */
			if (mpd->map.m_len > 0 && mpd->next_page != page->index)
				goto out;

I guess this also will not hold for us given we will always have m_len to be 0.
<...>
>  			if (mpd->map.m_len == 0)
>  				mpd->first_page = page->index;
>  			mpd->next_page = page->index + 1;
> -			/* Add all dirty buffers to mpd */
> -			lblk = ((ext4_lblk_t)page->index) <<
> -				(PAGE_SHIFT - blkbits);
> -			head = page_buffers(page);
> -			err = mpage_process_page_bufs(mpd, head, head, lblk);
> -			if (err <= 0)
> -				goto out;
> -			err = 0;
> +			/*
> +			 * Writeout for transaction commit where we cannot
> +			 * modify metadata is simple. Just submit the page.
> +			 */
> +			if (!mpd->can_map) {
> +				if (ext4_page_nomap_can_writeout(page)) {
> +					err = mpage_submit_page(mpd, page);
> +					if (err < 0)
> +						goto out;
> +				} else {
> +					unlock_page(page);
> +					mpd->first_page++;

We anyway should always have mpd->map.m_len = 0.
That means, we always set mpd->first_page = page->index above.
So this might not be useful. But I guess for consistency of the code,
or to avoid any future bugs, this isn't harmful to keep.


> +				}
> +			} else {
> +				/* Add all dirty buffers to mpd */
> +				lblk = ((ext4_lblk_t)page->index) <<
> +					(PAGE_SHIFT - blkbits);
> +				head = page_buffers(page);
> +				err = mpage_process_page_bufs(mpd, head, head,
> +							      lblk);
> +				if (err <= 0)
> +					goto out;
> +				err = 0;
> +			}
>  			left--;
>  		}
>  		pagevec_release(&pvec);
> @@ -2778,6 +2808,7 @@ static int ext4_writepages(struct address_space *mapping,
>  	 */
>  	mpd.do_map = 0;
>  	mpd.scanned_until_end = 0;
> +	mpd.can_map = 1;
>  	mpd.io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
>  	if (!mpd.io_submit.io_end) {
>  		ret = -ENOMEM;
> --
> 2.35.3
>

Re: [PATCH 5/9] ext4: Add support for writepages calls that cannot map blocks

[Jan Kara]

On Thu 01-12-22 16:43:59, Ritesh Harjani (IBM) wrote:
> On 22/11/30 05:35PM, Jan Kara wrote:
> > Add support for calls to ext4_writepages() than cannot map blocks. These
> > will be issued from jbd2 transaction commit code.
> 
> I guess we should expand the description of mpage_prepare_extent_to_map()
> function now. Other than that the patch looks good to me.
> 
> Please feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thanks for review!

> >  /*
> >   * mpage_prepare_extent_to_map - find & lock contiguous range of dirty pages
> >   * 				 and underlying extent to map
> 
> Since we are overloading this function. this can be also called with can_map
> as 0. Maybe good to add some description around that?

Well, it was somewhat overloaded already before but you're right some
documentation update is in order :) I'll do that.

> > @@ -2651,14 +2665,30 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
> 
> 
> adding context of code so that it doesn't get missed in the discussion.
> 
> <...>
> 			/* If we can't merge this page, we are done. */
> 			if (mpd->map.m_len > 0 && mpd->next_page != page->index)
> 				goto out;
> 
> I guess this also will not hold for us given we will always have m_len to be 0.
> <...>

Correct.

> > +			/*
> > +			 * Writeout for transaction commit where we cannot
> > +			 * modify metadata is simple. Just submit the page.
> > +			 */
> > +			if (!mpd->can_map) {
> > +				if (ext4_page_nomap_can_writeout(page)) {
> > +					err = mpage_submit_page(mpd, page);
> > +					if (err < 0)
> > +						goto out;
> > +				} else {
> > +					unlock_page(page);
> > +					mpd->first_page++;
> 
> We anyway should always have mpd->map.m_len = 0.
> That means, we always set mpd->first_page = page->index above.
> So this might not be useful. But I guess for consistency of the code,
> or to avoid any future bugs, this isn't harmful to keep.

Yes, it is mostly for consistency but it is also needed so that once we
exit the loop, mpage_release_unused_pages() starts working from a correct
page.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 5/9] ext4: Add support for writepages calls that cannot map blocks

[Ritesh Harjani (IBM)]

On 22/12/01 12:50PM, Jan Kara wrote:
> > > +			/*
> > > +			 * Writeout for transaction commit where we cannot
> > > +			 * modify metadata is simple. Just submit the page.
> > > +			 */
> > > +			if (!mpd->can_map) {
> > > +				if (ext4_page_nomap_can_writeout(page)) {
> > > +					err = mpage_submit_page(mpd, page);
> > > +					if (err < 0)
> > > +						goto out;
> > > +				} else {
> > > +					unlock_page(page);
> > > +					mpd->first_page++;
> >
> > We anyway should always have mpd->map.m_len = 0.
> > That means, we always set mpd->first_page = page->index above.
> > So this might not be useful. But I guess for consistency of the code,
> > or to avoid any future bugs, this isn't harmful to keep.
>
> Yes, it is mostly for consistency but it is also needed so that once we
> exit the loop, mpage_release_unused_pages() starts working from a correct
> page.

Oh yes! right.

-ritesh

[PATCH 6/9] ext4: Provide ext4_do_writepages()

[Jan Kara]

Provide ext4_do_writepages() function that takes mpage_da_data as an
argument and make ext4_writepages() just a simple wrapper around it. No
functional changes.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 96 +++++++++++++++++++++++++++----------------------
 1 file changed, 54 insertions(+), 42 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1cde20eb6500..fbea77ab470f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1543,9 +1543,12 @@ void ext4_da_release_space(struct inode *inode, int to_free)
  */
 
 struct mpage_da_data {
+	/* These are input fields for ext4_do_writepages() */
 	struct inode *inode;
 	struct writeback_control *wbc;
+	unsigned int can_map:1;	/* Can writepages call map blocks? */
 
+	/* These are internal state of ext4_do_writepages() */
 	pgoff_t first_page;	/* The first page to write */
 	pgoff_t next_page;	/* Current page to examine */
 	pgoff_t last_page;	/* Last page to examine */
@@ -1557,7 +1560,6 @@ struct mpage_da_data {
 	struct ext4_map_blocks map;
 	struct ext4_io_submit io_submit;	/* IO submission data */
 	unsigned int do_map:1;
-	unsigned int can_map:1;	/* Can writepages call map blocks? */
 	unsigned int scanned_until_end:1;
 };
 
@@ -2701,16 +2703,16 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 	return err;
 }
 
-static int ext4_writepages(struct address_space *mapping,
-			   struct writeback_control *wbc)
+static int ext4_do_writepages(struct mpage_da_data *mpd)
 {
+	struct writeback_control *wbc = mpd->wbc;
 	pgoff_t	writeback_index = 0;
 	long nr_to_write = wbc->nr_to_write;
 	int range_whole = 0;
 	int cycled = 1;
 	handle_t *handle = NULL;
-	struct mpage_da_data mpd;
-	struct inode *inode = mapping->host;
+	struct inode *inode = mpd->inode;
+	struct address_space *mapping = inode->i_mapping;
 	int needed_blocks, rsv_blocks = 0, ret = 0;
 	struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
 	struct blk_plug plug;
@@ -2785,19 +2787,18 @@ static int ext4_writepages(struct address_space *mapping,
 		writeback_index = mapping->writeback_index;
 		if (writeback_index)
 			cycled = 0;
-		mpd.first_page = writeback_index;
-		mpd.last_page = -1;
+		mpd->first_page = writeback_index;
+		mpd->last_page = -1;
 	} else {
-		mpd.first_page = wbc->range_start >> PAGE_SHIFT;
-		mpd.last_page = wbc->range_end >> PAGE_SHIFT;
+		mpd->first_page = wbc->range_start >> PAGE_SHIFT;
+		mpd->last_page = wbc->range_end >> PAGE_SHIFT;
 	}
 
-	mpd.inode = inode;
-	mpd.wbc = wbc;
-	ext4_io_submit_init(&mpd.io_submit, wbc);
+	ext4_io_submit_init(&mpd->io_submit, wbc);
 retry:
 	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
-		tag_pages_for_writeback(mapping, mpd.first_page, mpd.last_page);
+		tag_pages_for_writeback(mapping, mpd->first_page,
+					mpd->last_page);
 	blk_start_plug(&plug);
 
 	/*
@@ -2806,28 +2807,27 @@ static int ext4_writepages(struct address_space *mapping,
 	 * in the block layer on device congestion while having transaction
 	 * started.
 	 */
-	mpd.do_map = 0;
-	mpd.scanned_until_end = 0;
-	mpd.can_map = 1;
-	mpd.io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
-	if (!mpd.io_submit.io_end) {
+	mpd->do_map = 0;
+	mpd->scanned_until_end = 0;
+	mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
+	if (!mpd->io_submit.io_end) {
 		ret = -ENOMEM;
 		goto unplug;
 	}
-	ret = mpage_prepare_extent_to_map(&mpd);
+	ret = mpage_prepare_extent_to_map(mpd);
 	/* Unlock pages we didn't use */
-	mpage_release_unused_pages(&mpd, false);
+	mpage_release_unused_pages(mpd, false);
 	/* Submit prepared bio */
-	ext4_io_submit(&mpd.io_submit);
-	ext4_put_io_end_defer(mpd.io_submit.io_end);
-	mpd.io_submit.io_end = NULL;
+	ext4_io_submit(&mpd->io_submit);
+	ext4_put_io_end_defer(mpd->io_submit.io_end);
+	mpd->io_submit.io_end = NULL;
 	if (ret < 0)
 		goto unplug;
 
-	while (!mpd.scanned_until_end && wbc->nr_to_write > 0) {
+	while (!mpd->scanned_until_end && wbc->nr_to_write > 0) {
 		/* For each extent of pages we use new io_end */
-		mpd.io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
-		if (!mpd.io_submit.io_end) {
+		mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
+		if (!mpd->io_submit.io_end) {
 			ret = -ENOMEM;
 			break;
 		}
@@ -2851,16 +2851,16 @@ static int ext4_writepages(struct address_space *mapping,
 			       "%ld pages, ino %lu; err %d", __func__,
 				wbc->nr_to_write, inode->i_ino, ret);
 			/* Release allocated io_end */
-			ext4_put_io_end(mpd.io_submit.io_end);
-			mpd.io_submit.io_end = NULL;
+			ext4_put_io_end(mpd->io_submit.io_end);
+			mpd->io_submit.io_end = NULL;
 			break;
 		}
-		mpd.do_map = 1;
+		mpd->do_map = 1;
 
-		trace_ext4_da_write_pages(inode, mpd.first_page, mpd.wbc);
-		ret = mpage_prepare_extent_to_map(&mpd);
-		if (!ret && mpd.map.m_len)
-			ret = mpage_map_and_submit_extent(handle, &mpd,
+		trace_ext4_da_write_pages(inode, mpd->first_page, wbc);
+		ret = mpage_prepare_extent_to_map(mpd);
+		if (!ret && mpd->map.m_len)
+			ret = mpage_map_and_submit_extent(handle, mpd,
 					&give_up_on_write);
 		/*
 		 * Caution: If the handle is synchronous,
@@ -2875,12 +2875,12 @@ static int ext4_writepages(struct address_space *mapping,
 		if (!ext4_handle_valid(handle) || handle->h_sync == 0) {
 			ext4_journal_stop(handle);
 			handle = NULL;
-			mpd.do_map = 0;
+			mpd->do_map = 0;
 		}
 		/* Unlock pages we didn't use */
-		mpage_release_unused_pages(&mpd, give_up_on_write);
+		mpage_release_unused_pages(mpd, give_up_on_write);
 		/* Submit prepared bio */
-		ext4_io_submit(&mpd.io_submit);
+		ext4_io_submit(&mpd->io_submit);
 
 		/*
 		 * Drop our io_end reference we got from init. We have
@@ -2890,11 +2890,11 @@ static int ext4_writepages(struct address_space *mapping,
 		 * up doing unwritten extent conversion.
 		 */
 		if (handle) {
-			ext4_put_io_end_defer(mpd.io_submit.io_end);
+			ext4_put_io_end_defer(mpd->io_submit.io_end);
 			ext4_journal_stop(handle);
 		} else
-			ext4_put_io_end(mpd.io_submit.io_end);
-		mpd.io_submit.io_end = NULL;
+			ext4_put_io_end(mpd->io_submit.io_end);
+		mpd->io_submit.io_end = NULL;
 
 		if (ret == -ENOSPC && sbi->s_journal) {
 			/*
@@ -2914,8 +2914,8 @@ static int ext4_writepages(struct address_space *mapping,
 	blk_finish_plug(&plug);
 	if (!ret && !cycled && wbc->nr_to_write > 0) {
 		cycled = 1;
-		mpd.last_page = writeback_index - 1;
-		mpd.first_page = 0;
+		mpd->last_page = writeback_index - 1;
+		mpd->first_page = 0;
 		goto retry;
 	}
 
@@ -2925,7 +2925,7 @@ static int ext4_writepages(struct address_space *mapping,
 		 * Set the writeback_index so that range_cyclic
 		 * mode will write it back later
 		 */
-		mapping->writeback_index = mpd.first_page;
+		mapping->writeback_index = mpd->first_page;
 
 out_writepages:
 	trace_ext4_writepages_result(inode, wbc, ret,
@@ -2934,6 +2934,18 @@ static int ext4_writepages(struct address_space *mapping,
 	return ret;
 }
 
+static int ext4_writepages(struct address_space *mapping,
+			   struct writeback_control *wbc)
+{
+	struct mpage_da_data mpd = {
+		.inode = mapping->host,
+		.wbc = wbc,
+		.can_map = 1,
+	};
+
+	return ext4_do_writepages(&mpd);
+}
+
 static int ext4_dax_writepages(struct address_space *mapping,
 			       struct writeback_control *wbc)
 {
-- 
2.35.3

Re: [PATCH 6/9] ext4: Provide ext4_do_writepages()

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> Provide ext4_do_writepages() function that takes mpage_da_data as an
> argument and make ext4_writepages() just a simple wrapper around it. No
> functional changes.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c | 96 +++++++++++++++++++++++++++----------------------
>  1 file changed, 54 insertions(+), 42 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 1cde20eb6500..fbea77ab470f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1543,9 +1543,12 @@ void ext4_da_release_space(struct inode *inode, int to_free)
>   */
>
>  struct mpage_da_data {
> +	/* These are input fields for ext4_do_writepages() */
>  	struct inode *inode;
>  	struct writeback_control *wbc;
> +	unsigned int can_map:1;	/* Can writepages call map blocks? */
>
> +	/* These are internal state of ext4_do_writepages() */
>  	pgoff_t first_page;	/* The first page to write */
>  	pgoff_t next_page;	/* Current page to examine */
>  	pgoff_t last_page;	/* Last page to examine */
> @@ -1557,7 +1560,6 @@ struct mpage_da_data {
>  	struct ext4_map_blocks map;
>  	struct ext4_io_submit io_submit;	/* IO submission data */
>  	unsigned int do_map:1;
> -	unsigned int can_map:1;	/* Can writepages call map blocks? */
>  	unsigned int scanned_until_end:1;
>  };
>
> @@ -2701,16 +2703,16 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
>  	return err;
>  }
>
> -static int ext4_writepages(struct address_space *mapping,
> -			   struct writeback_control *wbc)
> +static int ext4_do_writepages(struct mpage_da_data *mpd)
>  {
> +	struct writeback_control *wbc = mpd->wbc;
>  	pgoff_t	writeback_index = 0;
>  	long nr_to_write = wbc->nr_to_write;
>  	int range_whole = 0;
>  	int cycled = 1;
>  	handle_t *handle = NULL;
> -	struct mpage_da_data mpd;
> -	struct inode *inode = mapping->host;
> +	struct inode *inode = mpd->inode;
> +	struct address_space *mapping = inode->i_mapping;
>  	int needed_blocks, rsv_blocks = 0, ret = 0;
>  	struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
>  	struct blk_plug plug;
> @@ -2785,19 +2787,18 @@ static int ext4_writepages(struct address_space *mapping,
>  		writeback_index = mapping->writeback_index;
>  		if (writeback_index)
>  			cycled = 0;
> -		mpd.first_page = writeback_index;
> -		mpd.last_page = -1;
> +		mpd->first_page = writeback_index;
> +		mpd->last_page = -1;
>  	} else {
> -		mpd.first_page = wbc->range_start >> PAGE_SHIFT;
> -		mpd.last_page = wbc->range_end >> PAGE_SHIFT;
> +		mpd->first_page = wbc->range_start >> PAGE_SHIFT;
> +		mpd->last_page = wbc->range_end >> PAGE_SHIFT;
>  	}
>
> -	mpd.inode = inode;
> -	mpd.wbc = wbc;
> -	ext4_io_submit_init(&mpd.io_submit, wbc);
> +	ext4_io_submit_init(&mpd->io_submit, wbc);
>  retry:
>  	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
> -		tag_pages_for_writeback(mapping, mpd.first_page, mpd.last_page);
> +		tag_pages_for_writeback(mapping, mpd->first_page,
> +					mpd->last_page);
>  	blk_start_plug(&plug);
>
>  	/*
> @@ -2806,28 +2807,27 @@ static int ext4_writepages(struct address_space *mapping,
>  	 * in the block layer on device congestion while having transaction
>  	 * started.
>  	 */
> -	mpd.do_map = 0;
> -	mpd.scanned_until_end = 0;
> -	mpd.can_map = 1;
> -	mpd.io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
> -	if (!mpd.io_submit.io_end) {
> +	mpd->do_map = 0;
> +	mpd->scanned_until_end = 0;
> +	mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
> +	if (!mpd->io_submit.io_end) {
>  		ret = -ENOMEM;
>  		goto unplug;
>  	}
> -	ret = mpage_prepare_extent_to_map(&mpd);
> +	ret = mpage_prepare_extent_to_map(mpd);
>  	/* Unlock pages we didn't use */
> -	mpage_release_unused_pages(&mpd, false);
> +	mpage_release_unused_pages(mpd, false);
>  	/* Submit prepared bio */
> -	ext4_io_submit(&mpd.io_submit);
> -	ext4_put_io_end_defer(mpd.io_submit.io_end);
> -	mpd.io_submit.io_end = NULL;
> +	ext4_io_submit(&mpd->io_submit);
> +	ext4_put_io_end_defer(mpd->io_submit.io_end);
> +	mpd->io_submit.io_end = NULL;
>  	if (ret < 0)
>  		goto unplug;
>
> -	while (!mpd.scanned_until_end && wbc->nr_to_write > 0) {
> +	while (!mpd->scanned_until_end && wbc->nr_to_write > 0) {
>  		/* For each extent of pages we use new io_end */
> -		mpd.io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
> -		if (!mpd.io_submit.io_end) {
> +		mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
> +		if (!mpd->io_submit.io_end) {
>  			ret = -ENOMEM;
>  			break;
>  		}
> @@ -2851,16 +2851,16 @@ static int ext4_writepages(struct address_space *mapping,
>  			       "%ld pages, ino %lu; err %d", __func__,
>  				wbc->nr_to_write, inode->i_ino, ret);
>  			/* Release allocated io_end */
> -			ext4_put_io_end(mpd.io_submit.io_end);
> -			mpd.io_submit.io_end = NULL;
> +			ext4_put_io_end(mpd->io_submit.io_end);
> +			mpd->io_submit.io_end = NULL;
>  			break;
>  		}
> -		mpd.do_map = 1;
> +		mpd->do_map = 1;
>
> -		trace_ext4_da_write_pages(inode, mpd.first_page, mpd.wbc);
> -		ret = mpage_prepare_extent_to_map(&mpd);
> -		if (!ret && mpd.map.m_len)
> -			ret = mpage_map_and_submit_extent(handle, &mpd,
> +		trace_ext4_da_write_pages(inode, mpd->first_page, wbc);
> +		ret = mpage_prepare_extent_to_map(mpd);
> +		if (!ret && mpd->map.m_len)
> +			ret = mpage_map_and_submit_extent(handle, mpd,
>  					&give_up_on_write);
>  		/*
>  		 * Caution: If the handle is synchronous,
> @@ -2875,12 +2875,12 @@ static int ext4_writepages(struct address_space *mapping,
>  		if (!ext4_handle_valid(handle) || handle->h_sync == 0) {
>  			ext4_journal_stop(handle);
>  			handle = NULL;
> -			mpd.do_map = 0;
> +			mpd->do_map = 0;
>  		}
>  		/* Unlock pages we didn't use */
> -		mpage_release_unused_pages(&mpd, give_up_on_write);
> +		mpage_release_unused_pages(mpd, give_up_on_write);
>  		/* Submit prepared bio */
> -		ext4_io_submit(&mpd.io_submit);
> +		ext4_io_submit(&mpd->io_submit);
>
>  		/*
>  		 * Drop our io_end reference we got from init. We have
> @@ -2890,11 +2890,11 @@ static int ext4_writepages(struct address_space *mapping,
>  		 * up doing unwritten extent conversion.
>  		 */
>  		if (handle) {
> -			ext4_put_io_end_defer(mpd.io_submit.io_end);
> +			ext4_put_io_end_defer(mpd->io_submit.io_end);
>  			ext4_journal_stop(handle);
>  		} else
> -			ext4_put_io_end(mpd.io_submit.io_end);
> -		mpd.io_submit.io_end = NULL;
> +			ext4_put_io_end(mpd->io_submit.io_end);
> +		mpd->io_submit.io_end = NULL;
>
>  		if (ret == -ENOSPC && sbi->s_journal) {
>  			/*
> @@ -2914,8 +2914,8 @@ static int ext4_writepages(struct address_space *mapping,
>  	blk_finish_plug(&plug);
>  	if (!ret && !cycled && wbc->nr_to_write > 0) {
>  		cycled = 1;
> -		mpd.last_page = writeback_index - 1;
> -		mpd.first_page = 0;
> +		mpd->last_page = writeback_index - 1;
> +		mpd->first_page = 0;
>  		goto retry;
>  	}
>
> @@ -2925,7 +2925,7 @@ static int ext4_writepages(struct address_space *mapping,
>  		 * Set the writeback_index so that range_cyclic
>  		 * mode will write it back later
>  		 */
> -		mapping->writeback_index = mpd.first_page;
> +		mapping->writeback_index = mpd->first_page;
>
>  out_writepages:
>  	trace_ext4_writepages_result(inode, wbc, ret,
> @@ -2934,6 +2934,18 @@ static int ext4_writepages(struct address_space *mapping,
>  	return ret;
>  }
>
> +static int ext4_writepages(struct address_space *mapping,
> +			   struct writeback_control *wbc)
> +{
> +	struct mpage_da_data mpd = {
> +		.inode = mapping->host,
> +		.wbc = wbc,
> +		.can_map = 1,
> +	};
> +
> +	return ext4_do_writepages(&mpd);
> +}
> +

Nice. Looks good to me.

Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH 7/9] ext4: Move percpu_rwsem protection into ext4_writepages()

[Jan Kara]

Move protection by percpu_rwsem from ext4_do_writepages() to
ext4_writepages(). We will not want to grab this protection during
transaction commits as that would be prone to deadlocks and the
protection is not needed. Move the shutdown state checking as well since
we want to be able to complete commit while the shutdown is in progress.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fbea77ab470f..00c4d12f8270 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2718,10 +2718,6 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	struct blk_plug plug;
 	bool give_up_on_write = false;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
-		return -EIO;
-
-	percpu_down_read(&sbi->s_writepages_rwsem);
 	trace_ext4_writepages(inode, wbc);
 
 	/*
@@ -2930,20 +2926,28 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 out_writepages:
 	trace_ext4_writepages_result(inode, wbc, ret,
 				     nr_to_write - wbc->nr_to_write);
-	percpu_up_read(&sbi->s_writepages_rwsem);
 	return ret;
 }
 
 static int ext4_writepages(struct address_space *mapping,
 			   struct writeback_control *wbc)
 {
+	struct super_block *sb = mapping->host->i_sb;
 	struct mpage_da_data mpd = {
 		.inode = mapping->host,
 		.wbc = wbc,
 		.can_map = 1,
 	};
+	int ret;
+
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
+		return -EIO;
 
-	return ext4_do_writepages(&mpd);
+	percpu_down_read(&EXT4_SB(sb)->s_writepages_rwsem);
+	ret = ext4_do_writepages(&mpd);
+	percpu_up_read(&EXT4_SB(sb)->s_writepages_rwsem);
+
+	return ret;
 }
 
 static int ext4_dax_writepages(struct address_space *mapping,
-- 
2.35.3

Re: [PATCH 7/9] ext4: Move percpu_rwsem protection into ext4_writepages()

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> Move protection by percpu_rwsem from ext4_do_writepages() to
> ext4_writepages(). We will not want to grab this protection during
> transaction commits as that would be prone to deadlocks and the
> protection is not needed. Move the shutdown state checking as well since
> we want to be able to complete commit while the shutdown is in progress.

Yup. Sounds good. Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index fbea77ab470f..00c4d12f8270 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2718,10 +2718,6 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>  	struct blk_plug plug;
>  	bool give_up_on_write = false;
>
> -	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> -		return -EIO;
> -
> -	percpu_down_read(&sbi->s_writepages_rwsem);
>  	trace_ext4_writepages(inode, wbc);
>
>  	/*
> @@ -2930,20 +2926,28 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>  out_writepages:
>  	trace_ext4_writepages_result(inode, wbc, ret,
>  				     nr_to_write - wbc->nr_to_write);
> -	percpu_up_read(&sbi->s_writepages_rwsem);
>  	return ret;
>  }
>
>  static int ext4_writepages(struct address_space *mapping,
>  			   struct writeback_control *wbc)
>  {
> +	struct super_block *sb = mapping->host->i_sb;
>  	struct mpage_da_data mpd = {
>  		.inode = mapping->host,
>  		.wbc = wbc,
>  		.can_map = 1,
>  	};
> +	int ret;
> +
> +	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
> +		return -EIO;
>
> -	return ext4_do_writepages(&mpd);
> +	percpu_down_read(&EXT4_SB(sb)->s_writepages_rwsem);
> +	ret = ext4_do_writepages(&mpd);
> +	percpu_up_read(&EXT4_SB(sb)->s_writepages_rwsem);
> +
> +	return ret;
>  }
>
>  static int ext4_dax_writepages(struct address_space *mapping,
> --
> 2.35.3
>

[PATCH 8/9] ext4: Switch to using ext4_do_writepages() for ordered data writeout

[Jan Kara]

Use the standard writepages method (ext4_do_writepages()) to perform
writeout of ordered data during journal commit.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/inode.c | 16 ++++++++++++++++
 fs/ext4/super.c |  3 +--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 1b3bffc04fd0..07b55cc48578 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2999,6 +2999,7 @@ extern void ext4_set_inode_flags(struct inode *, bool init);
 extern int ext4_alloc_da_blocks(struct inode *inode);
 extern void ext4_set_aops(struct inode *inode);
 extern int ext4_writepage_trans_blocks(struct inode *);
+extern int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode);
 extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
 extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
 			     loff_t lstart, loff_t lend);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 00c4d12f8270..c131b611dabf 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2950,6 +2950,22 @@ static int ext4_writepages(struct address_space *mapping,
 	return ret;
 }
 
+int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode)
+{
+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_ALL,
+		.nr_to_write = LONG_MAX,
+		.range_start = jinode->i_dirty_start,
+		.range_end = jinode->i_dirty_end,
+	};
+	struct mpage_da_data mpd = {
+		.inode = jinode->i_vfs_inode,
+		.wbc = &wbc,
+		.can_map = 0,
+	};
+	return ext4_do_writepages(&mpd);
+}
+
 static int ext4_dax_writepages(struct address_space *mapping,
 			       struct writeback_control *wbc)
 {
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7cdd2138c897..c02329dd7574 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -540,8 +540,7 @@ static int ext4_journal_submit_inode_data_buffers(struct jbd2_inode *jinode)
 	if (ext4_should_journal_data(jinode->i_vfs_inode))
 		ret = ext4_journalled_submit_inode_data_buffers(jinode);
 	else
-		ret = jbd2_journal_submit_inode_data_buffers(jinode);
-
+		ret = ext4_normal_submit_inode_data_buffers(jinode);
 	return ret;
 }
 
-- 
2.35.3

Re: [PATCH 8/9] ext4: Switch to using ext4_do_writepages() for ordered data writeout

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> Use the standard writepages method (ext4_do_writepages()) to perform
> writeout of ordered data during journal commit.

Neat!!
Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/ext4.h  |  1 +
>  fs/ext4/inode.c | 16 ++++++++++++++++
>  fs/ext4/super.c |  3 +--
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 1b3bffc04fd0..07b55cc48578 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2999,6 +2999,7 @@ extern void ext4_set_inode_flags(struct inode *, bool init);
>  extern int ext4_alloc_da_blocks(struct inode *inode);
>  extern void ext4_set_aops(struct inode *inode);
>  extern int ext4_writepage_trans_blocks(struct inode *);
> +extern int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode);
>  extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
>  extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
>  			     loff_t lstart, loff_t lend);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 00c4d12f8270..c131b611dabf 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2950,6 +2950,22 @@ static int ext4_writepages(struct address_space *mapping,
>  	return ret;
>  }
>
> +int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode)
> +{
> +	struct writeback_control wbc = {
> +		.sync_mode = WB_SYNC_ALL,
> +		.nr_to_write = LONG_MAX,
> +		.range_start = jinode->i_dirty_start,
> +		.range_end = jinode->i_dirty_end,
> +	};
> +	struct mpage_da_data mpd = {
> +		.inode = jinode->i_vfs_inode,
> +		.wbc = &wbc,
> +		.can_map = 0,
> +	};
> +	return ext4_do_writepages(&mpd);
> +}
> +
>  static int ext4_dax_writepages(struct address_space *mapping,
>  			       struct writeback_control *wbc)
>  {
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7cdd2138c897..c02329dd7574 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -540,8 +540,7 @@ static int ext4_journal_submit_inode_data_buffers(struct jbd2_inode *jinode)
>  	if (ext4_should_journal_data(jinode->i_vfs_inode))
>  		ret = ext4_journalled_submit_inode_data_buffers(jinode);
>  	else
> -		ret = jbd2_journal_submit_inode_data_buffers(jinode);
> -
> +		ret = ext4_normal_submit_inode_data_buffers(jinode);
>  	return ret;
>  }
>
> --
> 2.35.3
>

[PATCH 9/9] ext4: Remove ordered data support from ext4_writepage()

[Jan Kara]

ext4_writepage() should not be called for ordered data anymore. Remove
support for it from the function.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 116 ++++++------------------------------------------
 1 file changed, 13 insertions(+), 103 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c131b611dabf..0c8e700265f1 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1642,12 +1642,6 @@ static void ext4_print_free_blocks(struct inode *inode)
 	return;
 }
 
-static int ext4_bh_delay_or_unwritten(handle_t *handle, struct inode *inode,
-				      struct buffer_head *bh)
-{
-	return (buffer_delay(bh) || buffer_unwritten(bh)) && buffer_dirty(bh);
-}
-
 /*
  * ext4_insert_delayed_block - adds a delayed block to the extents status
  *                             tree, incrementing the reserved cluster/block
@@ -1962,56 +1956,17 @@ static int __ext4_journalled_writepage(struct page *page,
 }
 
 /*
- * Note that we don't need to start a transaction unless we're journaling data
- * because we should have holes filled from ext4_page_mkwrite(). We even don't
- * need to file the inode to the transaction's list in ordered mode because if
- * we are writing back data added by write(), the inode is already there and if
- * we are writing back data modified via mmap(), no one guarantees in which
- * transaction the data will hit the disk. In case we are journaling data, we
- * cannot start transaction directly because transaction start ranks above page
- * lock so we have to do some magic.
- *
- * This function can get called via...
- *   - ext4_writepages after taking page lock (have journal handle)
- *   - journal_submit_inode_data_buffers (no journal handle)
- *   - shrink_page_list via the kswapd/direct reclaim (no journal handle)
- *   - grab_page_cache when doing write_begin (have journal handle)
- *
- * We don't do any block allocation in this function. If we have page with
- * multiple blocks we need to write those buffer_heads that are mapped. This
- * is important for mmaped based write. So if we do with blocksize 1K
- * truncate(f, 1024);
- * a = mmap(f, 0, 4096);
- * a[0] = 'a';
- * truncate(f, 4096);
- * we have in the page first buffer_head mapped via page_mkwrite call back
- * but other buffer_heads would be unmapped but dirty (dirty done via the
- * do_wp_page). So writepage should write the first block. If we modify
- * the mmap area beyond 1024 we will again get a page_fault and the
- * page_mkwrite callback will do the block allocation and mark the
- * buffer_heads mapped.
- *
- * We redirty the page if we have any buffer_heads that is either delay or
- * unwritten in the page.
- *
- * We can get recursively called as show below.
- *
- *	ext4_writepage() -> kmalloc() -> __alloc_pages() -> page_launder() ->
- *		ext4_writepage()
- *
- * But since we don't do any block allocation we should not deadlock.
- * Page also have the dirty flag cleared so we don't get recurive page_lock.
+ * This function is now used only when journaling data. We cannot start
+ * transaction directly because transaction start ranks above page lock so we
+ * have to do some magic.
  */
 static int ext4_writepage(struct page *page,
 			  struct writeback_control *wbc)
 {
 	struct folio *folio = page_folio(page);
-	int ret = 0;
 	loff_t size;
 	unsigned int len;
-	struct buffer_head *page_bufs = NULL;
 	struct inode *inode = page->mapping->host;
-	struct ext4_io_submit io_submit;
 
 	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
 		folio_invalidate(folio, 0, folio_size(folio));
@@ -2036,60 +1991,16 @@ static int ext4_writepage(struct page *page,
 		return 0;
 	}
 
-	page_bufs = page_buffers(page);
-	/*
-	 * We cannot do block allocation or other extent handling in this
-	 * function. If there are buffers needing that, we have to redirty
-	 * the page. But we may reach here when we do a journal commit via
-	 * journal_submit_inode_data_buffers() and in that case we must write
-	 * allocated buffers to achieve data=ordered mode guarantees.
-	 *
-	 * Also, if there is only one buffer per page (the fs block
-	 * size == the page size), if one buffer needs block
-	 * allocation or needs to modify the extent tree to clear the
-	 * unwritten flag, we know that the page can't be written at
-	 * all, so we might as well refuse the write immediately.
-	 * Unfortunately if the block size != page size, we can't as
-	 * easily detect this case using ext4_walk_page_buffers(), but
-	 * for the extremely common case, this is an optimization that
-	 * skips a useless round trip through ext4_bio_write_page().
-	 */
-	if (ext4_walk_page_buffers(NULL, inode, page_bufs, 0, len, NULL,
-				   ext4_bh_delay_or_unwritten)) {
-		redirty_page_for_writepage(wbc, page);
-		if ((current->flags & PF_MEMALLOC) ||
-		    (inode->i_sb->s_blocksize == PAGE_SIZE)) {
-			/*
-			 * For memory cleaning there's no point in writing only
-			 * some buffers. So just bail out. Warn if we came here
-			 * from direct reclaim.
-			 */
-			WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD))
-							== PF_MEMALLOC);
-			unlock_page(page);
-			return 0;
-		}
-	}
-
-	if (PageChecked(page) && ext4_should_journal_data(inode))
-		/*
-		 * It's mmapped pagecache.  Add buffers and journal it.  There
-		 * doesn't seem much point in redirtying the page here.
-		 */
-		return __ext4_journalled_writepage(page, len);
-
-	ext4_io_submit_init(&io_submit, wbc);
-	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
-	if (!io_submit.io_end) {
-		redirty_page_for_writepage(wbc, page);
+	WARN_ON_ONCE(!ext4_should_journal_data(inode));
+	if (!PageChecked(page)) {
 		unlock_page(page);
-		return -ENOMEM;
+		return 0;
 	}
-	ret = ext4_bio_write_page(&io_submit, page, len);
-	ext4_io_submit(&io_submit);
-	/* Drop io_end reference we got from init */
-	ext4_put_io_end_defer(io_submit.io_end);
-	return ret;
+	/*
+	 * It's mmapped pagecache.  Add buffers and journal it.  There
+	 * doesn't seem much point in redirtying the page here.
+	 */
+	return __ext4_journalled_writepage(page, len);
 }
 
 static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
@@ -3142,9 +3053,8 @@ static int ext4_da_write_end(struct file *file,
 	 * i_disksize since writeback will push i_disksize upto i_size
 	 * eventually. If the end of the current write is > i_size and
 	 * inside an allocated block (ext4_da_should_update_i_disksize()
-	 * check), we need to update i_disksize here as neither
-	 * ext4_writepage() nor certain ext4_writepages() paths not
-	 * allocating blocks update i_disksize.
+	 * check), we need to update i_disksize here as ext4_writepages() need
+	 * not do it in this case.
 	 *
 	 * Note that we defer inode dirtying to generic_write_end() /
 	 * ext4_da_write_inline_data_end().
-- 
2.35.3

Re: [PATCH 9/9] ext4: Remove ordered data support from ext4_writepage()

[Ritesh Harjani (IBM)]

On 22/11/30 05:36PM, Jan Kara wrote:
> ext4_writepage() should not be called for ordered data anymore. Remove
> support for it from the function.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c | 116 ++++++------------------------------------------
>  1 file changed, 13 insertions(+), 103 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c131b611dabf..0c8e700265f1 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1642,12 +1642,6 @@ static void ext4_print_free_blocks(struct inode *inode)
>  	return;
>  }
>
> -static int ext4_bh_delay_or_unwritten(handle_t *handle, struct inode *inode,
> -				      struct buffer_head *bh)
> -{
> -	return (buffer_delay(bh) || buffer_unwritten(bh)) && buffer_dirty(bh);
> -}
> -
>  /*
>   * ext4_insert_delayed_block - adds a delayed block to the extents status
>   *                             tree, incrementing the reserved cluster/block
> @@ -1962,56 +1956,17 @@ static int __ext4_journalled_writepage(struct page *page,
>  }
>
>  /*
> - * Note that we don't need to start a transaction unless we're journaling data
> - * because we should have holes filled from ext4_page_mkwrite(). We even don't
> - * need to file the inode to the transaction's list in ordered mode because if
> - * we are writing back data added by write(), the inode is already there and if
> - * we are writing back data modified via mmap(), no one guarantees in which
> - * transaction the data will hit the disk. In case we are journaling data, we
> - * cannot start transaction directly because transaction start ranks above page
> - * lock so we have to do some magic.
> - *
> - * This function can get called via...
> - *   - ext4_writepages after taking page lock (have journal handle)
> - *   - journal_submit_inode_data_buffers (no journal handle)
> - *   - shrink_page_list via the kswapd/direct reclaim (no journal handle)
> - *   - grab_page_cache when doing write_begin (have journal handle)
> - *
> - * We don't do any block allocation in this function. If we have page with
> - * multiple blocks we need to write those buffer_heads that are mapped. This
> - * is important for mmaped based write. So if we do with blocksize 1K
> - * truncate(f, 1024);
> - * a = mmap(f, 0, 4096);
> - * a[0] = 'a';
> - * truncate(f, 4096);
> - * we have in the page first buffer_head mapped via page_mkwrite call back
> - * but other buffer_heads would be unmapped but dirty (dirty done via the
> - * do_wp_page). So writepage should write the first block. If we modify
> - * the mmap area beyond 1024 we will again get a page_fault and the
> - * page_mkwrite callback will do the block allocation and mark the
> - * buffer_heads mapped.
> - *
> - * We redirty the page if we have any buffer_heads that is either delay or
> - * unwritten in the page.
> - *
> - * We can get recursively called as show below.
> - *
> - *	ext4_writepage() -> kmalloc() -> __alloc_pages() -> page_launder() ->
> - *		ext4_writepage()
> - *
> - * But since we don't do any block allocation we should not deadlock.
> - * Page also have the dirty flag cleared so we don't get recurive page_lock.
> + * This function is now used only when journaling data. We cannot start
> + * transaction directly because transaction start ranks above page lock so we
> + * have to do some magic.
>   */
>  static int ext4_writepage(struct page *page,
>  			  struct writeback_control *wbc)
>  {
>  	struct folio *folio = page_folio(page);
> -	int ret = 0;
>  	loff_t size;
>  	unsigned int len;
> -	struct buffer_head *page_bufs = NULL;
>  	struct inode *inode = page->mapping->host;
> -	struct ext4_io_submit io_submit;
>
>  	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
>  		folio_invalidate(folio, 0, folio_size(folio));
> @@ -2036,60 +1991,16 @@ static int ext4_writepage(struct page *page,
>  		return 0;
>  	}
>
> -	page_bufs = page_buffers(page);
> -	/*
> -	 * We cannot do block allocation or other extent handling in this
> -	 * function. If there are buffers needing that, we have to redirty
> -	 * the page. But we may reach here when we do a journal commit via
> -	 * journal_submit_inode_data_buffers() and in that case we must write
> -	 * allocated buffers to achieve data=ordered mode guarantees.

Maybe this description can go above mpage_prepare_extent_to_map
for can_map = 0 case.

Looks good to me. Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>



> -	 *
> -	 * Also, if there is only one buffer per page (the fs block
> -	 * size == the page size), if one buffer needs block
> -	 * allocation or needs to modify the extent tree to clear the
> -	 * unwritten flag, we know that the page can't be written at
> -	 * all, so we might as well refuse the write immediately.
> -	 * Unfortunately if the block size != page size, we can't as
> -	 * easily detect this case using ext4_walk_page_buffers(), but
> -	 * for the extremely common case, this is an optimization that
> -	 * skips a useless round trip through ext4_bio_write_page().
> -	 */
> -	if (ext4_walk_page_buffers(NULL, inode, page_bufs, 0, len, NULL,
> -				   ext4_bh_delay_or_unwritten)) {
> -		redirty_page_for_writepage(wbc, page);
> -		if ((current->flags & PF_MEMALLOC) ||
> -		    (inode->i_sb->s_blocksize == PAGE_SIZE)) {
> -			/*
> -			 * For memory cleaning there's no point in writing only
> -			 * some buffers. So just bail out. Warn if we came here
> -			 * from direct reclaim.
> -			 */
> -			WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD))
> -							== PF_MEMALLOC);
> -			unlock_page(page);
> -			return 0;
> -		}
> -	}
> -
> -	if (PageChecked(page) && ext4_should_journal_data(inode))
> -		/*
> -		 * It's mmapped pagecache.  Add buffers and journal it.  There
> -		 * doesn't seem much point in redirtying the page here.
> -		 */
> -		return __ext4_journalled_writepage(page, len);
> -
> -	ext4_io_submit_init(&io_submit, wbc);
> -	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
> -	if (!io_submit.io_end) {
> -		redirty_page_for_writepage(wbc, page);
> +	WARN_ON_ONCE(!ext4_should_journal_data(inode));
> +	if (!PageChecked(page)) {
>  		unlock_page(page);
> -		return -ENOMEM;
> +		return 0;
>  	}
> -	ret = ext4_bio_write_page(&io_submit, page, len);
> -	ext4_io_submit(&io_submit);
> -	/* Drop io_end reference we got from init */
> -	ext4_put_io_end_defer(io_submit.io_end);
> -	return ret;
> +	/*
> +	 * It's mmapped pagecache.  Add buffers and journal it.  There
> +	 * doesn't seem much point in redirtying the page here.
> +	 */
> +	return __ext4_journalled_writepage(page, len);
>  }
>
>  static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
> @@ -3142,9 +3053,8 @@ static int ext4_da_write_end(struct file *file,
>  	 * i_disksize since writeback will push i_disksize upto i_size
>  	 * eventually. If the end of the current write is > i_size and
>  	 * inside an allocated block (ext4_da_should_update_i_disksize()
> -	 * check), we need to update i_disksize here as neither
> -	 * ext4_writepage() nor certain ext4_writepages() paths not
> -	 * allocating blocks update i_disksize.
> +	 * check), we need to update i_disksize here as ext4_writepages() need
> +	 * not do it in this case.
>  	 *
>  	 * Note that we defer inode dirtying to generic_write_end() /
>  	 * ext4_da_write_inline_data_end().
> --
> 2.35.3
>

Re: [PATCH 9/9] ext4: Remove ordered data support from ext4_writepage()

[Ritesh Harjani (IBM)]

On 22/12/01 04:51PM, Ritesh Harjani (IBM) wrote:
> On 22/11/30 05:36PM, Jan Kara wrote:
> > ext4_writepage() should not be called for ordered data anymore. Remove
> > support for it from the function.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext4/inode.c | 116 ++++++------------------------------------------
> >  1 file changed, 13 insertions(+), 103 deletions(-)
> >
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index c131b611dabf..0c8e700265f1 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -1642,12 +1642,6 @@ static void ext4_print_free_blocks(struct inode *inode)
> >  	return;
> >  }
> >
> > -static int ext4_bh_delay_or_unwritten(handle_t *handle, struct inode *inode,
> > -				      struct buffer_head *bh)
> > -{
> > -	return (buffer_delay(bh) || buffer_unwritten(bh)) && buffer_dirty(bh);
> > -}
> > -
> >  /*
> >   * ext4_insert_delayed_block - adds a delayed block to the extents status
> >   *                             tree, incrementing the reserved cluster/block
> > @@ -1962,56 +1956,17 @@ static int __ext4_journalled_writepage(struct page *page,
> >  }
> >
> >  /*
> > - * Note that we don't need to start a transaction unless we're journaling data
> > - * because we should have holes filled from ext4_page_mkwrite(). We even don't
> > - * need to file the inode to the transaction's list in ordered mode because if
> > - * we are writing back data added by write(), the inode is already there and if
> > - * we are writing back data modified via mmap(), no one guarantees in which
> > - * transaction the data will hit the disk. In case we are journaling data, we
> > - * cannot start transaction directly because transaction start ranks above page
> > - * lock so we have to do some magic.
> > - *
> > - * This function can get called via...
> > - *   - ext4_writepages after taking page lock (have journal handle)
> > - *   - journal_submit_inode_data_buffers (no journal handle)
> > - *   - shrink_page_list via the kswapd/direct reclaim (no journal handle)
> > - *   - grab_page_cache when doing write_begin (have journal handle)
> > - *
> > - * We don't do any block allocation in this function. If we have page with
> > - * multiple blocks we need to write those buffer_heads that are mapped. This
> > - * is important for mmaped based write. So if we do with blocksize 1K
> > - * truncate(f, 1024);
> > - * a = mmap(f, 0, 4096);
> > - * a[0] = 'a';
> > - * truncate(f, 4096);
> > - * we have in the page first buffer_head mapped via page_mkwrite call back
> > - * but other buffer_heads would be unmapped but dirty (dirty done via the
> > - * do_wp_page). So writepage should write the first block. If we modify
> > - * the mmap area beyond 1024 we will again get a page_fault and the
> > - * page_mkwrite callback will do the block allocation and mark the
> > - * buffer_heads mapped.
> > - *
> > - * We redirty the page if we have any buffer_heads that is either delay or
> > - * unwritten in the page.
> > - *
> > - * We can get recursively called as show below.
> > - *
> > - *	ext4_writepage() -> kmalloc() -> __alloc_pages() -> page_launder() ->
> > - *		ext4_writepage()
> > - *
> > - * But since we don't do any block allocation we should not deadlock.
> > - * Page also have the dirty flag cleared so we don't get recurive page_lock.
> > + * This function is now used only when journaling data. We cannot start
> > + * transaction directly because transaction start ranks above page lock so we
> > + * have to do some magic.
> >   */
> >  static int ext4_writepage(struct page *page,
> >  			  struct writeback_control *wbc)
> >  {
> >  	struct folio *folio = page_folio(page);
> > -	int ret = 0;
> >  	loff_t size;
> >  	unsigned int len;
> > -	struct buffer_head *page_bufs = NULL;
> >  	struct inode *inode = page->mapping->host;
> > -	struct ext4_io_submit io_submit;
> >
> >  	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
> >  		folio_invalidate(folio, 0, folio_size(folio));
> > @@ -2036,60 +1991,16 @@ static int ext4_writepage(struct page *page,
> >  		return 0;
> >  	}
> >
> > -	page_bufs = page_buffers(page);
> > -	/*
> > -	 * We cannot do block allocation or other extent handling in this
> > -	 * function. If there are buffers needing that, we have to redirty
> > -	 * the page. But we may reach here when we do a journal commit via
> > -	 * journal_submit_inode_data_buffers() and in that case we must write
> > -	 * allocated buffers to achieve data=ordered mode guarantees.
>
> Maybe this description can go above mpage_prepare_extent_to_map
> for can_map = 0 case.
>
> Looks good to me. Feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>
>
> > -	 *
> > -	 * Also, if there is only one buffer per page (the fs block
> > -	 * size == the page size), if one buffer needs block
> > -	 * allocation or needs to modify the extent tree to clear the
> > -	 * unwritten flag, we know that the page can't be written at
> > -	 * all, so we might as well refuse the write immediately.
> > -	 * Unfortunately if the block size != page size, we can't as
> > -	 * easily detect this case using ext4_walk_page_buffers(), but
> > -	 * for the extremely common case, this is an optimization that
> > -	 * skips a useless round trip through ext4_bio_write_page().
> > -	 */
> > -	if (ext4_walk_page_buffers(NULL, inode, page_bufs, 0, len, NULL,
> > -				   ext4_bh_delay_or_unwritten)) {
> > -		redirty_page_for_writepage(wbc, page);
> > -		if ((current->flags & PF_MEMALLOC) ||
> > -		    (inode->i_sb->s_blocksize == PAGE_SIZE)) {
> > -			/*
> > -			 * For memory cleaning there's no point in writing only
> > -			 * some buffers. So just bail out. Warn if we came here
> > -			 * from direct reclaim.
> > -			 */
> > -			WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD))
> > -							== PF_MEMALLOC);
> > -			unlock_page(page);
> > -			return 0;
> > -		}
> > -	}
> > -
> > -	if (PageChecked(page) && ext4_should_journal_data(inode))
> > -		/*
> > -		 * It's mmapped pagecache.  Add buffers and journal it.  There
> > -		 * doesn't seem much point in redirtying the page here.
> > -		 */
> > -		return __ext4_journalled_writepage(page, len);
> > -
> > -	ext4_io_submit_init(&io_submit, wbc);
> > -	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
> > -	if (!io_submit.io_end) {
> > -		redirty_page_for_writepage(wbc, page);
> > +	WARN_ON_ONCE(!ext4_should_journal_data(inode));

Oh and one more thing, this will give a WARN_ON_ONCE(), until we change the pageout()
function from reclaim path to not call ->writepage() method.
This until then might cause random fstest to fail for sometime if it observes a
kernel warning message while the test was running.

[ 5081.820019] WARNING: CPU: 3 PID: 125 at fs/ext4/inode.c:1994 ext4_writepage+0x380/0xb80
[ 5081.822884] Modules linked in:
[ 5081.824487] CPU: 3 PID: 125 Comm: kswapd0 Not tainted 6.1.0-rc4-00054-g969d94a2d4d6 #101
[ 5081.825559] Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1200 0xf000005 of:SLOF,git-6b6c16 pSeries
[ 5081.826743] NIP:  c00000000077a2c0 LR: c00000000077a2b4 CTR: c000000000779f40
[ 5081.827547] REGS: c0000000073d72d0 TRAP: 0700   Not tainted  (6.1.0-rc4-00054-g969d94a2d4d6)
<...>
[ 5081.862838] NIP [c00000000077a2c0] ext4_writepage+0x380/0xb80
[ 5081.864963] LR [c00000000077a2b4] ext4_writepage+0x374/0xb80
[ 5081.865995] Call Trace:
[ 5081.866510]  ext4_writepage+0x190/0xb80 (unreliable)
[ 5081.867438]  pageout+0x1b0/0x550
[ 5081.868110]  shrink_folio_list+0xb48/0x1400
[ 5081.868803]  shrink_inactive_list+0x2ec/0x6b0
[ 5081.869504]  shrink_lruvec+0x6f0/0x7b0
[ 5081.870160]  shrink_node+0x5e4/0x980
[ 5081.870801]  balance_pgdat+0x4cc/0x910
[ 5081.871453]  kswapd+0x6e4/0x820
[ 5081.872062]  kthread+0x168/0x170
[ 5081.872691]  ret_from_kernel_thread+0x5c/0x64


-ritesh


> > +	if (!PageChecked(page)) {
> >  		unlock_page(page);
> > -		return -ENOMEM;
> > +		return 0;
> >  	}
> > -	ret = ext4_bio_write_page(&io_submit, page, len);
> > -	ext4_io_submit(&io_submit);
> > -	/* Drop io_end reference we got from init */
> > -	ext4_put_io_end_defer(io_submit.io_end);
> > -	return ret;
> > +	/*
> > +	 * It's mmapped pagecache.  Add buffers and journal it.  There
> > +	 * doesn't seem much point in redirtying the page here.
> > +	 */
> > +	return __ext4_journalled_writepage(page, len);
> >  }
> >
> >  static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
> > @@ -3142,9 +3053,8 @@ static int ext4_da_write_end(struct file *file,
> >  	 * i_disksize since writeback will push i_disksize upto i_size
> >  	 * eventually. If the end of the current write is > i_size and
> >  	 * inside an allocated block (ext4_da_should_update_i_disksize()
> > -	 * check), we need to update i_disksize here as neither
> > -	 * ext4_writepage() nor certain ext4_writepages() paths not
> > -	 * allocating blocks update i_disksize.
> > +	 * check), we need to update i_disksize here as ext4_writepages() need
> > +	 * not do it in this case.
> >  	 *
> >  	 * Note that we defer inode dirtying to generic_write_end() /
> >  	 * ext4_da_write_inline_data_end().
> > --
> > 2.35.3
> >

Re: [PATCH 9/9] ext4: Remove ordered data support from ext4_writepage()

[Jan Kara]

On Thu 01-12-22 19:06:19, Ritesh Harjani (IBM) wrote:
> On 22/12/01 04:51PM, Ritesh Harjani (IBM) wrote:
> > On 22/11/30 05:36PM, Jan Kara wrote:
> > > -	ext4_io_submit_init(&io_submit, wbc);
> > > -	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
> > > -	if (!io_submit.io_end) {
> > > -		redirty_page_for_writepage(wbc, page);
> > > +	WARN_ON_ONCE(!ext4_should_journal_data(inode));
> 
> Oh and one more thing, this will give a WARN_ON_ONCE(), until we change the pageout()
> function from reclaim path to not call ->writepage() method.
> This until then might cause random fstest to fail for sometime if it observes a
> kernel warning message while the test was running.
> 
> [ 5081.820019] WARNING: CPU: 3 PID: 125 at fs/ext4/inode.c:1994 ext4_writepage+0x380/0xb80
> [ 5081.822884] Modules linked in:
> [ 5081.824487] CPU: 3 PID: 125 Comm: kswapd0 Not tainted 6.1.0-rc4-00054-g969d94a2d4d6 #101
> [ 5081.825559] Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1200 0xf000005 of:SLOF,git-6b6c16 pSeries
> [ 5081.826743] NIP:  c00000000077a2c0 LR: c00000000077a2b4 CTR: c000000000779f40
> [ 5081.827547] REGS: c0000000073d72d0 TRAP: 0700   Not tainted  (6.1.0-rc4-00054-g969d94a2d4d6)
> <...>
> [ 5081.862838] NIP [c00000000077a2c0] ext4_writepage+0x380/0xb80
> [ 5081.864963] LR [c00000000077a2b4] ext4_writepage+0x374/0xb80
> [ 5081.865995] Call Trace:
> [ 5081.866510]  ext4_writepage+0x190/0xb80 (unreliable)
> [ 5081.867438]  pageout+0x1b0/0x550
> [ 5081.868110]  shrink_folio_list+0xb48/0x1400
> [ 5081.868803]  shrink_inactive_list+0x2ec/0x6b0
> [ 5081.869504]  shrink_lruvec+0x6f0/0x7b0
> [ 5081.870160]  shrink_node+0x5e4/0x980
> [ 5081.870801]  balance_pgdat+0x4cc/0x910
> [ 5081.871453]  kswapd+0x6e4/0x820
> [ 5081.872062]  kthread+0x168/0x170
> [ 5081.872691]  ret_from_kernel_thread+0x5c/0x64

Hum, right. It didn't trigger for me :). I'll see how to fix this.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 9/9] ext4: Remove ordered data support from ext4_writepage()

[Christoph Hellwig]

On Thu, Dec 01, 2022 at 03:11:44PM +0100, Jan Kara wrote:
> Hum, right. It didn't trigger for me :). I'll see how to fix this.

Just stop wiring up ->writepage (except for journalled data mode).

Re: [PATCH 0/9] ext4: Stop using ext4_writepage() for writeout of ordered data

[Ritesh Harjani (IBM)]

On 22/11/30 05:35PM, Jan Kara wrote:
> Hello,
>
> this patch series modifies ext4 so that we stop using ext4_writepage() for
> writeout of ordered data during transaction commit (through
> generic_writepages() from jbd2_journal_submit_inode_data_buffers()). Instead we
> directly call ext4_writepages() from the
> ext4_journal_submit_inode_data_buffers().

Hello Jan,

Do you think we should add a WARN_ON_ONCE() or something in
ext4_do_writepages() function where we might try to start a transaction
at [J]. Since we can now enter into ext4_do_writepages() from two places:
1. writeback
2. jbd2_journal_commit_transaction()
	mpage_submit_page
	mpage_prepare_extent_to_map
	ext4_do_writepages
	ext4_normal_submit_inode_data_buffers
	ext4_journal_submit_inode_data_buffers
	journal_submit_data_buffers
	jbd2_journal_commit_transaction
	kjournald2

So IIUC, we will call mpage_submit_page() in the first call to
mpage_prepare_extent_to_map() [1] itself. That may set mpd->scanned_until_end = 1
at the end of it. So then we should never enter into the while loop where we
start a journal txn.

But can it ever happen that we ever into this while loop for writing out
pages for journal_submit_data_buffers()?

ext4_do_writepages()
{
<...>
	mpd->do_map = 0;
	mpd->scanned_until_end = 0;
	mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
<...>
[1]	ret = mpage_prepare_extent_to_map(mpd);
	/* Unlock pages we didn't use */
	mpage_release_unused_pages(mpd, false);
	/* Submit prepared bio */
	ext4_io_submit(&mpd->io_submit);
	ext4_put_io_end_defer(mpd->io_submit.io_end);
	mpd->io_submit.io_end = NULL;
<...>
	while (!mpd->scanned_until_end && wbc->nr_to_write > 0) {
		/* For each extent of pages we use new io_end */
		mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
<....>
		BUG_ON(ext4_should_journal_data(inode));
		needed_blocks = ext4_da_writepages_trans_blocks(inode);

		/* start a new transaction */
[J]		handle = ext4_journal_start_with_reserve(inode,
				EXT4_HT_WRITE_PAGE, needed_blocks, rsv_blocks);
<...>
		mpd->do_map = 1;

		trace_ext4_da_write_pages(inode, mpd->first_page, wbc);
[2]		ret = mpage_prepare_extent_to_map(mpd);
		if (!ret && mpd->map.m_len)
			ret = mpage_map_and_submit_extent(handle, mpd,
					&give_up_on_write);
<...>
	}
<...>
}

-ritesh

Re: [PATCH 0/9] ext4: Stop using ext4_writepage() for writeout of ordered data

[Jan Kara]

On Thu 01-12-22 17:12:05, Ritesh Harjani (IBM) wrote:
> On 22/11/30 05:35PM, Jan Kara wrote:
> > Hello,
> >
> > this patch series modifies ext4 so that we stop using ext4_writepage() for
> > writeout of ordered data during transaction commit (through
> > generic_writepages() from jbd2_journal_submit_inode_data_buffers()). Instead we
> > directly call ext4_writepages() from the
> > ext4_journal_submit_inode_data_buffers().
> 
> Hello Jan,
> 
> Do you think we should add a WARN_ON_ONCE() or something in
> ext4_do_writepages() function where we might try to start a transaction
> at [J]. Since we can now enter into ext4_do_writepages() from two places:
> 1. writeback
> 2. jbd2_journal_commit_transaction()
> 	mpage_submit_page
> 	mpage_prepare_extent_to_map
> 	ext4_do_writepages
> 	ext4_normal_submit_inode_data_buffers
> 	ext4_journal_submit_inode_data_buffers
> 	journal_submit_data_buffers
> 	jbd2_journal_commit_transaction
> 	kjournald2
> 
> So IIUC, we will call mpage_submit_page() in the first call to
> mpage_prepare_extent_to_map() [1] itself. That may set mpd->scanned_until_end = 1
> at the end of it. So then we should never enter into the while loop where we
> start a journal txn.

Yes, correct. But I agree a WARN_ON_ONCE in the loop might be useful just
in case. I'll add it. Thanks for the idea.

								Honza


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR