linux-ext4 - Re: [PATCH 1/9] ext4: Handle redirtying in ext4_bio_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221201085716.g5avthxgxjzorcmq@riteshh-domain>
Date:   Thu, 1 Dec 2022 14:27:16 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 1/9] ext4: Handle redirtying in ext4_bio_write_page()

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@...il.com>


> Signed-off-by: Jan Kara <jack@...e.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
>