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:43:59 +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 5/9] ext4: Add support for writepages calls that cannot
 map blocks
Message-ID: <20221201111359.onr5edsaaxcr2ndh@riteshh-domain>
References: <20221130162435.2324-1-jack@suse.cz>
 <20221130163608.29034-5-jack@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221130163608.29034-5-jack@suse.cz>
Precedence: bulk

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
>