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 16:45:37 +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 6/9] ext4: Provide ext4_do_writepages()
Message-ID: <20221201111537.oai5xaibwynyst4u@riteshh-domain>
References: <20221130162435.2324-1-jack@suse.cz>
 <20221130163608.29034-6-jack@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221130163608.29034-6-jack@suse.cz>
Precedence: bulk

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>