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 <[email protected]>
---
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
@@ -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;
--
2.35.3
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) <[email protected]>
>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> 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) <[email protected]>
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
--
Jan Kara <[email protected]>
SUSE Labs, CR
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