Optimize __ext4_block_zero_page_range() for unwritten buffers
Ojaswin Mujoo
reviewercover Optimize __ext4_block_zero_page_range() for unwritten buffers
As per discussion with Jan here [1], this patchset intends to exit early from __ext4_block_zero_page_range() incase the block we are about to zero (partially) is unwritten and unmapped, since such a block doesn't require zeroing.
Further, also make sure that calls to ext4_zero_partial_blocks() truncate the page cache completely beforehand, so that they don't rely on ext4_zero_partial_block() -> __ext4_block_zero_page_range() to zero out non block aligned edges of pagecache.
Reviews and comments are appreciated!
Regards,
ojaswin
[1]
https://lore.kernel.org/linux-ext4/20230914141920.lw2nlpzhcxwuz2y6@quack3/
Ojaswin Mujoo (3):
ext4: treat end of range as exclusive in ext4_zero_range()
ext4: truncate complete range in pagecache before calling
ext4_zero_partial_blocks()
ext4: Skip unwritten buffers in __ext4_block_zero_page_range()
fs/ext4/extents.c | 20 +++++++++++++------- fs/ext4/inode.c | 7 +++++-- 2 files changed, 18 insertions(+), 9 deletions(-)
Signature · 1 line
2.39.3
Ojaswin Mujoo
reviewerPATCH 1/3 ext4: treat end of range as exclusive in ext4_zero_range()
The call to filemap_write_and_wait_range() assumes the range passed to be inclusive, so fix the call to make sure we follow that.
--- fs/ext4/extents.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
fs/ext4/extents.c +3 -2
@@ -4522,7 +4522,8 @@ static long ext4_zero_range(struct file *file, loff_t offset,* Round up offset. This is not fallocate, we need to zero out * blocks, so convert interior block aligned part of the range to * unwritten and possibly manually zero out unaligned parts of the -* range. +* range. Here, start and partial_begin are inclusive, end and +* partial_end are exclusive. */ start =round_up (offset,1 << blkbits); end =round_down ((offset + len),1 << blkbits);
@@ -4608,7 +4609,7 @@ static long ext4_zero_range(struct file *file, loff_t offset,* disk in case of crash before zeroing trans is committed. */ if (ext4_should_journal_data (inode)) { - ret =filemap_write_and_wait_range (mapping, start, end); + ret =filemap_write_and_wait_range (mapping, start, end -1 );if (ret) {filemap_invalidate_unlock (mapping); goto out_mutex;
Signature · 1 line
2.39.3
Ojaswin Mujoo
reviewerPATCH 2/3 ext4: truncate complete range in pagecache before calling ext4_zero_partial_blocks()
In reply to Ojaswin MujooIn ext4_zero_range() and ext4_punch_hole(), the range passed could be unaligned however we only zero out the pagecache range that is block aligned. These functions are relying on ext4_zero_partial_blocks() -> __ext4_block_zero_page_range() to take care of zeroing the unaligned edges in the pageacache. However, the right thing to do is to properly zero out the whole range in these functions before and not rely on a different function to do it for us. Hence, modify ext4_zero_range() and ext4_punch_hole() to zero the complete range.
This will also allow us to now exit early for unwritten buffer heads in __ext4_block_zero_page_range(), in upcoming patch.
--- fs/ext4/extents.c | 17 +++++++++++------ fs/ext4/inode.c | 3 +-- 2 files changed, 12 insertions(+), 8 deletions(-)
fs/ext4/extents.c +11 -6
@@ -4582,9 +4582,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,/* Zero range excluding the unaligned edges */ if (max_blocks >0 ) { - flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | -EXT4_EX_NOCACHE ); -/* * Prevent page faults from reinstantiating pages we have * released from page cache.
@@ -4609,17 +4606,25 @@ static long ext4_zero_range(struct file *file, loff_t offset,* disk in case of crash before zeroing trans is committed. */ if (ext4_should_journal_data (inode)) { - ret =filemap_write_and_wait_range (mapping, start, end -1 ); + ret =filemap_write_and_wait_range (mapping, start, + end -1 );if (ret) {filemap_invalidate_unlock (mapping); goto out_mutex; } } + } + +/* +* Now truncate the pagecache and zero out non page aligned edges of the +* range (if any) +*/ +truncate_pagecache_range (inode, offset, offset + len -1 ); -/* Now release the pages and zero block aligned part of pages */ -truncate_pagecache_range (inode, start, end -1 ); +if (max_blocks >0 ) { inode->i_mtime = inode->i_ctime =current_time (inode); + flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |EXT4_EX_NOCACHE ); ret =ext4_alloc_file_blocks (file, lblk, max_blocks, new_size, flags);filemap_invalidate_unlock (mapping);
fs/ext4/inode.c +1 -2
@@ -3974,9 +3974,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) ret =ext4_update_disksize_before_punch (inode, offset, length);if (ret) goto out_dio; -truncate_pagecache_range (inode, first_block_offset, - last_block_offset); } +truncate_pagecache_range (inode, offset, offset + length -1 );if (ext4_test_inode_flag (inode,EXT4_INODE_EXTENTS )) credits =ext4_writepage_trans_blocks (inode);
Signature · 1 line
2.39.3
Ojaswin Mujoo
reviewerPATCH 3/3 ext4: Skip unwritten buffers in __ext4_block_zero_page_range()
In reply to Ojaswin MujooIf the buffer is unwritten then the underlying block should already return zero for reads. Further, if it is not dirty then we can be sure that there is no data on the folio that might get written back later. Hence we skip zeroing out the folio and underlying block in this case.
--- fs/ext4/inode.c | 4 ++++ 1 file changed, 4 insertions(+)
fs/ext4/inode.c +4 -0
@@ -3659,6 +3659,10 @@ static int __ext4_block_zero_page_range(handle_t *handle,BUFFER_TRACE (bh,"freed: skip" ); goto unlock; } +if (buffer_unwritten (bh) && !buffer_dirty (bh)) { +BUFFER_TRACE (bh,"unwritten and non-dirty: skip" ); + goto unlock; + }if (!buffer_mapped (bh)) {BUFFER_TRACE (bh,"unmapped" );ext4_get_block (inode, iblock, bh,0 );
Signature · 1 line
2.39.3
Ojaswin Mujoo
reviewerRe: [PATCH 2/3] ext4: truncate complete range in pagecache before calling ext4_zero_partial_blocks()
In reply to Ojaswin MujooQuoted 33 lines · Ojaswin Mujoo
In
ext4_zero_range()andext4_punch_hole(), the range passed could be unaligned however we only zero out the pagecache range that is block aligned. These functions are relying onext4_zero_partial_blocks()->__ext4_block_zero_page_range()to take care of zeroing the unaligned edges in the pageacache. However, the right thing to do is to properly zero out the whole range in these functions before and not rely on a different function to do it for us. Hence, modifyext4_zero_range()andext4_punch_hole()to zero the complete range.This will also allow us to now exit early for unwritten buffer heads in
__ext4_block_zero_page_range(), in upcoming patch.Signed-off-by Ojaswin Mujoo <[email protected]>--- fs/ext4/extents.c | 17 +++++++++++------ fs/ext4/inode.c | 3 +-- 2 files changed, 12 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-3@@ -4582,9 +4582,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,/* Zero range excluding the unaligned edges */ if (max_blocks >0 ) { - flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | -EXT4_EX_NOCACHE ); -/* * Prevent page faults from reinstantiating pages we have * released from page cache.
fs/ext4/extents.c static long ext4_zero_range(struct file *file, loff_t offset, · line 4610
Quoted 6 lines
fs/ext4/extents.c +2
-1@@ -4609,17 +4606,25 @@ static long ext4_zero_range(struct file *file, loff_t offset,* disk in case of crash before zeroing trans is committed. */ if (ext4_should_journal_data (inode)) { - ret =filemap_write_and_wait_range (mapping, start, end -1 ); + ret =filemap_write_and_wait_range (mapping, start, + end -1 );
I think this accidentally creeped in, will fix it in next rev.
Anyways, I had some questions that might be unrelated to this patch, I'll add them inline:
fs/ext4/extents.c static long ext4_zero_range(struct file *file, loff_t offset, · line 4616
Quoted 6 lines
fs/ext4/extents.c +1
-0if (ret) {filemap_invalidate_unlock (mapping); goto out_mutex; } } + }
So the above if (max_blocks) {...} block runs when the range spans multiple blocks but I think the filemap_write_and_wait_range() and ext4_update_disksize_before_punch() should be called when we are actually spanning multiple pages, since the disksize not updating issue and the truncate racing with checkpoint only happen when the complete page is truncated. Is this understanding correct?
Quoted 21 lines
fs/ext4/extents.c +8
-2+ +/* +* Now truncate the pagecache and zero out non page aligned edges of the +* range (if any) +*/ +truncate_pagecache_range (inode, offset, offset + len -1 ); -/* Now release the pages and zero block aligned part of pages */ -truncate_pagecache_range (inode, start, end -1 ); +if (max_blocks >0 ) { inode->i_mtime = inode->i_ctime =current_time (inode); + flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |EXT4_EX_NOCACHE ); ret =ext4_alloc_file_blocks (file, lblk, max_blocks, new_size, flags);filemap_invalidate_unlock (mapping);fs/ext4/inode.c +0
-0
fs/ext4/inode.c int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) · line 3974
Quoted 1 line
fs/ext4/inode.c +0
-0@@ -3974,9 +3974,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) ret =ext4_update_disksize_before_punch (inode, offset, length);
In this function ext4_punch_hole() I see that we call filemap_write_and_wait_range() and then take the inode_lock() later. Doesn't this leave a window for the pages to get dirty again?
For example, in ext4_zero_range(), we checkpoint using filemap_write_and_wait_range() in case of data=journal under inode_lock() but that's not the case here. Just wondering if this or any other code path might still race here?
Regards,
ojaswin
Quoted 12 lines
fs/ext4/inode.c +1
-2if (ret) goto out_dio; -truncate_pagecache_range (inode, first_block_offset, - last_block_offset); } +truncate_pagecache_range (inode, offset, offset + length -1 );if (ext4_test_inode_flag (inode,EXT4_INODE_EXTENTS )) credits =ext4_writepage_trans_blocks (inode);Signature · 1 line
2.39.3
Re: [PATCH 1/3] ext4: treat end of range as exclusive in ext4_zero_range()
In reply to Ojaswin MujooQuoted 4 lines · Ojaswin Mujoo
The call to
filemap_write_and_wait_range()assumes the range passed to be inclusive, so fix the call to make sure we follow that.Signed-off-by Ojaswin Mujoo <[email protected]>
Yes, makes sense. Feel free to add:
Honza
Quoted 30 lines
--- fs/ext4/extents.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)fs/ext4/extents.c +3
-2@@ -4522,7 +4522,8 @@ static long ext4_zero_range(struct file *file, loff_t offset,* Round up offset. This is not fallocate, we need to zero out * blocks, so convert interior block aligned part of the range to * unwritten and possibly manually zero out unaligned parts of the -* range. +* range. Here, start and partial_begin are inclusive, end and +* partial_end are exclusive. */ start =round_up (offset,1 << blkbits); end =round_down ((offset + len),1 << blkbits);@@ -4608,7 +4609,7 @@ static long ext4_zero_range(struct file *file, loff_t offset,* disk in case of crash before zeroing trans is committed. */ if (ext4_should_journal_data (inode)) { - ret =filemap_write_and_wait_range (mapping, start, end); + ret =filemap_write_and_wait_range (mapping, start, end -1 );if (ret) {filemap_invalidate_unlock (mapping); goto out_mutex;Signature · 1 line
2.39.3
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jan Kara
reviewerRe: [PATCH 2/3] ext4: truncate complete range in pagecache before calling ext4_zero_partial_blocks()
In reply to Ojaswin MujooQuoted 34 lines · Ojaswin Mujoo
Quoted 33 lines · Ojaswin Mujoo
In
ext4_zero_range()andext4_punch_hole(), the range passed could be unaligned however we only zero out the pagecache range that is block aligned. These functions are relying onext4_zero_partial_blocks()->__ext4_block_zero_page_range()to take care of zeroing the unaligned edges in the pageacache. However, the right thing to do is to properly zero out the whole range in these functions before and not rely on a different function to do it for us. Hence, modifyext4_zero_range()andext4_punch_hole()to zero the complete range.This will also allow us to now exit early for unwritten buffer heads in
__ext4_block_zero_page_range(), in upcoming patch.Signed-off-by Ojaswin Mujoo <[email protected]>--- fs/ext4/extents.c | 17 +++++++++++------ fs/ext4/inode.c | 3 +-- 2 files changed, 12 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-3@@ -4582,9 +4582,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,/* Zero range excluding the unaligned edges */ if (max_blocks >0 ) { - flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | -EXT4_EX_NOCACHE ); -/* * Prevent page faults from reinstantiating pages we have * released from page cache. @@ -4609,17 +4606,25 @@ static long ext4_zero_range(struct file *file, loff_t offset,
Quoted 8 lines
fs/ext4/extents.c static long ext4_zero_range(struct file *file, loff_t offset, · line 4610
Quoted 6 lines
fs/ext4/extents.c +2
-1* disk in case of crash before zeroing trans is committed. */ if (ext4_should_journal_data (inode)) { - ret =filemap_write_and_wait_range (mapping, start, end -1 ); + ret =filemap_write_and_wait_range (mapping, start, + end -1 );I think this accidentally creeped in, will fix it in next rev.
Yeah, just pull it in patch 1.
Quoted 5 lines
Quoted 5 lines
fs/ext4/extents.c +0
-0if (ret) {filemap_invalidate_unlock (mapping); goto out_mutex; } }
Quoted 8 lines
fs/ext4/extents.c static long ext4_zero_range(struct file *file, loff_t offset, · line 4616
Quoted 1 line
fs/ext4/extents.c +1
-0+ }So the above if (
max_blocks) {...} block runs when the range spans multiple blocks but I think thefilemap_write_and_wait_range()andext4_update_disksize_before_punch()should be called when we are actually spanning multiple pages, since the disksize not updating issue and the truncate racing with checkpoint only happen when the complete page is truncated. Is this understanding correct?
Why do you think the issues apply only to multiple pages? I mean even if a single block is dirty in memory, it may be pushing i_disksize or carrying journalled data we need to commit.
Quoted 11 lines
Quoted 11 lines
fs/ext4/extents.c +6
-2+/* +* Now truncate the pagecache and zero out non page aligned edges of the +* range (if any) +*/ +truncate_pagecache_range (inode, offset, offset + len -1 ); -/* Now release the pages and zero block aligned part of pages */ -truncate_pagecache_range (inode, start, end -1 ); +if (max_blocks >0 ) { inode->i_mtime = inode->i_ctime =current_time (inode);
Quoted 14 lines
fs/ext4/inode.c int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) · line 3974
Quoted 10 lines
fs/ext4/extents.c +1
-0+ flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |EXT4_EX_NOCACHE ); ret =ext4_alloc_file_blocks (file, lblk, max_blocks, new_size, flags);filemap_invalidate_unlock (mapping);fs/ext4/inode.c +0
-0@@ -3974,9 +3974,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) ret =ext4_update_disksize_before_punch (inode, offset, length);In this function
ext4_punch_hole()I see that we callfilemap_write_and_wait_range()and then take theinode_lock()later. Doesn't this leave a window for the pages to get dirty again?
There's definitely a race window but I think the call to filemap_write_and_wait_range() is a leftover from the past when hole punching could race in a nasty way. These days we have invalidate_lock so I *think* we can just remove that filemap_write_and_wait_range() call. At least I don't see a good reason for it now because the pages are going away anyway. But it needs testing :).
Quoted 4 lines
For example, in
ext4_zero_range(), we checkpoint usingfilemap_write_and_wait_range()in case of data=journal underinode_lock()but that's not the case here. Just wondering if this or any other code path might still race here?
Well, that's a bit different story as the comment there explains. And yes, invalidate_lock protects us from possible races there.
Quoted 6 lines
fs/ext4/inode.c int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) · line 3978
Quoted 6 lines
fs/ext4/inode.c +1
-2if (ret) goto out_dio; -truncate_pagecache_range (inode, first_block_offset, - last_block_offset); } +truncate_pagecache_range (inode, offset, offset + length -1 );
But I have realized that changes done in this patch actually don't help with changing ext4_zero_partial_blocks() because as soon as we drop invalidate_lock, a page fault can come in and modify contents of partial pages we need zeroed.
So thinking about this again with fresh mind, these block vs pagecache consistency issues aren't probably worth it and current code flow is good enough. Sorry for misleading you. We might just add a comment to __ext4_block_zero_page_range() to explain that buffer_unwritten() buffers can get there but they should be already zeroed-out and uptodate and we do need to process them because of page cache zeroing. What do you think?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Ojaswin Mujoo
reviewerRe: [PATCH 2/3] ext4: truncate complete range in pagecache before calling ext4_zero_partial_blocks()
Quoted 60 lines · Jan Kara
Quoted 34 lines · Ojaswin Mujoo
Quoted 33 lines · Ojaswin Mujoo
In
ext4_zero_range()andext4_punch_hole(), the range passed could be unaligned however we only zero out the pagecache range that is block aligned. These functions are relying onext4_zero_partial_blocks()->__ext4_block_zero_page_range()to take care of zeroing the unaligned edges in the pageacache. However, the right thing to do is to properly zero out the whole range in these functions before and not rely on a different function to do it for us. Hence, modifyext4_zero_range()andext4_punch_hole()to zero the complete range.This will also allow us to now exit early for unwritten buffer heads in
__ext4_block_zero_page_range(), in upcoming patch.Signed-off-by Ojaswin Mujoo <[email protected]>--- fs/ext4/extents.c | 17 +++++++++++------ fs/ext4/inode.c | 3 +-- 2 files changed, 12 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-3@@ -4582,9 +4582,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,/* Zero range excluding the unaligned edges */ if (max_blocks >0 ) { - flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | -EXT4_EX_NOCACHE ); -/* * Prevent page faults from reinstantiating pages we have * released from page cache. @@ -4609,17 +4606,25 @@ static long ext4_zero_range(struct file *file, loff_t offset,Quoted 8 lines
fs/ext4/extents.c static long ext4_zero_range(struct file *file, loff_t offset, · line 4610
Quoted 6 lines
fs/ext4/extents.c +2
-1* disk in case of crash before zeroing trans is committed. */ if (ext4_should_journal_data (inode)) { - ret =filemap_write_and_wait_range (mapping, start, end -1 ); + ret =filemap_write_and_wait_range (mapping, start, + end -1 );I think this accidentally creeped in, will fix it in next rev.
Yeah, just pull it in patch 1.
Quoted 5 lines
Quoted 5 lines
fs/ext4/extents.c +0
-0if (ret) {filemap_invalidate_unlock (mapping); goto out_mutex; } }Quoted 8 lines
fs/ext4/extents.c static long ext4_zero_range(struct file *file, loff_t offset, · line 4616
Quoted 1 line
fs/ext4/extents.c +1
-0+ }So the above if (
max_blocks) {...} block runs when the range spans multiple blocks but I think thefilemap_write_and_wait_range()andext4_update_disksize_before_punch()should be called when we are actually spanning multiple pages, since the disksize not updating issue and the truncate racing with checkpoint only happen when the complete page is truncated. Is this understanding correct?
Quoted 3 lines
Why do you think the issues apply only to multiple pages? I mean even if a single block is dirty in memory, it may be pushing
i_disksizeor carrying journalled data we need to commit.
Hey Jan,
You are right, I think I was misunderstanding this code a bit, thinking that these things would only be needed if the complete folio is absent. Upon rechecking the paths like the writeback path I can now see that even if the blocks till i_size are already allocated (eg, through ext4_zero_range) then we won't actually be calling mpage_map_and_submit_extent() which is where disksize updates.
Quoted 35 lines
Quoted 12 lines
Quoted 12 lines
fs/ext4/extents.c +6
-2+/* +* Now truncate the pagecache and zero out non page aligned edges of the +* range (if any) +*/ +truncate_pagecache_range (inode, offset, offset + len -1 ); -/* Now release the pages and zero block aligned part of pages */ -truncate_pagecache_range (inode, start, end -1 ); +if (max_blocks >0 ) { inode->i_mtime = inode->i_ctime =current_time (inode);Quoted 14 lines
fs/ext4/inode.c int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) · line 3974
Quoted 10 lines
fs/ext4/extents.c +1
-0+ flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |EXT4_EX_NOCACHE ); ret =ext4_alloc_file_blocks (file, lblk, max_blocks, new_size, flags);filemap_invalidate_unlock (mapping);fs/ext4/inode.c +0
-0@@ -3974,9 +3974,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) ret =ext4_update_disksize_before_punch (inode, offset, length);In this function
ext4_punch_hole()I see that we callfilemap_write_and_wait_range()and then take theinode_lock()later. Doesn't this leave a window for the pages to get dirty again?There's definitely a race window but I think the call to
filemap_write_and_wait_range()is a leftover from the past when hole punching could race in a nasty way. These days we haveinvalidate_lockso I *think* we can just remove thatfilemap_write_and_wait_range()call. At least I don't see a good reason for it now because the pages are going away anyway. But it needs testing :).
Quoted 7 lines
Quoted 4 lines
For example, in
ext4_zero_range(), we checkpoint usingfilemap_write_and_wait_range()in case of data=journal underinode_lock()but that's not the case here. Just wondering if this or any other code path might still race here?Well, that's a bit different story as the comment there explains. And yes,
invalidate_lockprotects us from possible races there.
Ahh okay, got it.
Quoted 13 lines
Quoted 7 lines
fs/ext4/inode.c int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) · line 3978
Quoted 7 lines
fs/ext4/inode.c +1
-2if (ret) goto out_dio; -truncate_pagecache_range (inode, first_block_offset, - last_block_offset); } +truncate_pagecache_range (inode, offset, offset + length -1 );But I have realized that changes done in this patch actually don't help with changing
ext4_zero_partial_blocks()because as soon as we dropinvalidate_lock, a page fault can come in and modify contents of partial pages we need zeroed.
Quoted 6 lines
So thinking about this again with fresh mind, these block vs pagecache consistency issues aren't probably worth it and current code flow is good enough. Sorry for misleading you. We might just add a comment to
__ext4_block_zero_page_range()to explain thatbuffer_unwritten()buffers can get there but they should be already zeroed-out and uptodate and we do need to process them because of page cache zeroing. What do you think?
Oh right, I was not thinking from the mmap path, sorry about that. In that case I think your point makes sense, lets just let this be for now. I'll send a v2 with the first patch of the series and also add a comment as you suggested.
Thanks for the review and taking the time to answer my questions!
Regards,
ojaswin
Quoted 5 lines
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR