Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 14:27:16 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Ted Tso <tytso@mit.edu>, linux-ext4@vger.kernel.org,
        Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 1/9] ext4: Handle redirtying in ext4_bio_write_page()
Message-ID: <20221201085716.g5avthxgxjzorcmq@riteshh-domain>
References: <20221130162435.2324-1-jack@suse.cz>
 <20221130163608.29034-1-jack@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221130163608.29034-1-jack@suse.cz>
Precedence: bulk

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
>