| Message ID | 20221130163608.29034-5-jack@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: Stop using ext4_writepage() for writeout of ordered data | expand |
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 >
On Thu 01-12-22 16:43:59, Ritesh Harjani (IBM) wrote: > 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> Thanks for review! > > /* > > * 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? Well, it was somewhat overloaded already before but you're right some documentation update is in order :) I'll do 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. > <...> Correct. > > + /* > > + * 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. Yes, it is mostly for consistency but it is also needed so that once we exit the loop, mpage_release_unused_pages() starts working from a correct page. Honza
On 22/12/01 12:50PM, Jan Kara wrote: > > > + /* > > > + * 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. > > Yes, it is mostly for consistency but it is also needed so that once we > exit the loop, mpage_release_unused_pages() starts working from a correct > page. Oh yes! right. -ritesh
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 @@ -2651,14 +2665,30 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd) 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++; + } + } 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;
Add support for calls to ext4_writepages() than cannot map blocks. These will be issued from jbd2 transaction commit code. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/inode.c | 47 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-)