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