| Message ID | 20240904062925.716856-4-yi.zhang@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: clean up and refactor fallocate | expand |
On Wed 04-09-24 14:29:18, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > Since we always write back dirty data before zeroing range and punching > hole, the delalloc extended file's disksize of should be updated > properly when writing back pages, hence we don't need to update file's > disksize before discarding page cache in ext4_zero_range() and > ext4_punch_hole(), just drop ext4_update_disksize_before_punch(). > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> So when we don't write out before hole punching & company this needs to stay in some shape or form. Honza > --- > fs/ext4/ext4.h | 3 --- > fs/ext4/extents.c | 4 ---- > fs/ext4/inode.c | 37 +------------------------------------ > 3 files changed, 1 insertion(+), 43 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 08acd152261e..e8d7965f62c4 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -3414,9 +3414,6 @@ static inline int ext4_update_inode_size(struct inode *inode, loff_t newsize) > return changed; > } > > -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset, > - loff_t len); > - > struct ext4_group_info { > unsigned long bb_state; > #ifdef AGGRESSIVE_CHECK > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 19a9b14935b7..d9fccf2970e9 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -4637,10 +4637,6 @@ static long ext4_zero_range(struct file *file, loff_t offset, > flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | > EXT4_EX_NOCACHE); > > - ret = ext4_update_disksize_before_punch(inode, offset, len); > - if (ret) > - goto out_invalidate_lock; > - > /* Now release the pages and zero block aligned part of pages */ > truncate_pagecache_range(inode, start, end - 1); > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 8af25442d44d..9343ce9f2b01 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3872,37 +3872,6 @@ int ext4_can_truncate(struct inode *inode) > return 0; > } > > -/* > - * We have to make sure i_disksize gets properly updated before we truncate > - * page cache due to hole punching or zero range. Otherwise i_disksize update > - * can get lost as it may have been postponed to submission of writeback but > - * that will never happen after we truncate page cache. > - */ > -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset, > - loff_t len) > -{ > - handle_t *handle; > - int ret; > - > - loff_t size = i_size_read(inode); > - > - WARN_ON(!inode_is_locked(inode)); > - if (offset > size || offset + len < size) > - return 0; > - > - if (EXT4_I(inode)->i_disksize >= size) > - return 0; > - > - handle = ext4_journal_start(inode, EXT4_HT_MISC, 1); > - if (IS_ERR(handle)) > - return PTR_ERR(handle); > - ext4_update_i_disksize(inode, size); > - ret = ext4_mark_inode_dirty(handle, inode); > - ext4_journal_stop(handle); > - > - return ret; > -} > - > static void ext4_wait_dax_page(struct inode *inode) > { > filemap_invalidate_unlock(inode->i_mapping); > @@ -4022,13 +3991,9 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) > last_block_offset = round_down((offset + length), sb->s_blocksize) - 1; > > /* Now release the pages and zero block aligned part of pages*/ > - if (last_block_offset > first_block_offset) { > - ret = ext4_update_disksize_before_punch(inode, offset, length); > - if (ret) > - goto out_dio; > + if (last_block_offset > first_block_offset) > truncate_pagecache_range(inode, first_block_offset, > last_block_offset); > - } > > if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) > credits = ext4_writepage_trans_blocks(inode); > -- > 2.39.2 >
On 2024/9/21 0:13, Jan Kara wrote: > On Wed 04-09-24 14:29:18, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> Since we always write back dirty data before zeroing range and punching >> hole, the delalloc extended file's disksize of should be updated >> properly when writing back pages, hence we don't need to update file's >> disksize before discarding page cache in ext4_zero_range() and >> ext4_punch_hole(), just drop ext4_update_disksize_before_punch(). >> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > So when we don't write out before hole punching & company this needs to stay > in some shape or form. > Thanks for taking time to review this series! I don't fully understand this comment, please let me confirm. Do you suggested that we still don't write out all the data before punching / zeroing / collapseing(i.e. drop patch 01), so we need to keep ext4_update_disksize_before_punch()(i.e. also drop this patch), is that right? Thanks, Yi. > >> --- >> fs/ext4/ext4.h | 3 --- >> fs/ext4/extents.c | 4 ---- >> fs/ext4/inode.c | 37 +------------------------------------ >> 3 files changed, 1 insertion(+), 43 deletions(-) >> >> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h >> index 08acd152261e..e8d7965f62c4 100644 >> --- a/fs/ext4/ext4.h >> +++ b/fs/ext4/ext4.h >> @@ -3414,9 +3414,6 @@ static inline int ext4_update_inode_size(struct inode *inode, loff_t newsize) >> return changed; >> } >> >> -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset, >> - loff_t len); >> - >> struct ext4_group_info { >> unsigned long bb_state; >> #ifdef AGGRESSIVE_CHECK >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index 19a9b14935b7..d9fccf2970e9 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -4637,10 +4637,6 @@ static long ext4_zero_range(struct file *file, loff_t offset, >> flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | >> EXT4_EX_NOCACHE); >> >> - ret = ext4_update_disksize_before_punch(inode, offset, len); >> - if (ret) >> - goto out_invalidate_lock; >> - >> /* Now release the pages and zero block aligned part of pages */ >> truncate_pagecache_range(inode, start, end - 1); >> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index 8af25442d44d..9343ce9f2b01 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -3872,37 +3872,6 @@ int ext4_can_truncate(struct inode *inode) >> return 0; >> } >> >> -/* >> - * We have to make sure i_disksize gets properly updated before we truncate >> - * page cache due to hole punching or zero range. Otherwise i_disksize update >> - * can get lost as it may have been postponed to submission of writeback but >> - * that will never happen after we truncate page cache. >> - */ >> -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset, >> - loff_t len) >> -{ >> - handle_t *handle; >> - int ret; >> - >> - loff_t size = i_size_read(inode); >> - >> - WARN_ON(!inode_is_locked(inode)); >> - if (offset > size || offset + len < size) >> - return 0; >> - >> - if (EXT4_I(inode)->i_disksize >= size) >> - return 0; >> - >> - handle = ext4_journal_start(inode, EXT4_HT_MISC, 1); >> - if (IS_ERR(handle)) >> - return PTR_ERR(handle); >> - ext4_update_i_disksize(inode, size); >> - ret = ext4_mark_inode_dirty(handle, inode); >> - ext4_journal_stop(handle); >> - >> - return ret; >> -} >> - >> static void ext4_wait_dax_page(struct inode *inode) >> { >> filemap_invalidate_unlock(inode->i_mapping); >> @@ -4022,13 +3991,9 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) >> last_block_offset = round_down((offset + length), sb->s_blocksize) - 1; >> >> /* Now release the pages and zero block aligned part of pages*/ >> - if (last_block_offset > first_block_offset) { >> - ret = ext4_update_disksize_before_punch(inode, offset, length); >> - if (ret) >> - goto out_dio; >> + if (last_block_offset > first_block_offset) >> truncate_pagecache_range(inode, first_block_offset, >> last_block_offset); >> - } >> >> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) >> credits = ext4_writepage_trans_blocks(inode); >> -- >> 2.39.2 >>
On Tue 24-09-24 15:43:22, Zhang Yi wrote: > On 2024/9/21 0:13, Jan Kara wrote: > > On Wed 04-09-24 14:29:18, Zhang Yi wrote: > >> From: Zhang Yi <yi.zhang@huawei.com> > >> > >> Since we always write back dirty data before zeroing range and punching > >> hole, the delalloc extended file's disksize of should be updated > >> properly when writing back pages, hence we don't need to update file's > >> disksize before discarding page cache in ext4_zero_range() and > >> ext4_punch_hole(), just drop ext4_update_disksize_before_punch(). > >> > >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > > > So when we don't write out before hole punching & company this needs to stay > > in some shape or form. > > > > Thanks for taking time to review this series! > > I don't fully understand this comment, please let me confirm. Do you > suggested that we still don't write out all the data before punching / > zeroing / collapseing(i.e. drop patch 01), so we need to keep > ext4_update_disksize_before_punch()(i.e. also drop this patch), is > that right? Yes, this is what I meant. Sorry for not being clear. Honza
On 2024/9/24 18:11, Jan Kara wrote: > On Tue 24-09-24 15:43:22, Zhang Yi wrote: >> On 2024/9/21 0:13, Jan Kara wrote: >>> On Wed 04-09-24 14:29:18, Zhang Yi wrote: >>>> From: Zhang Yi <yi.zhang@huawei.com> >>>> >>>> Since we always write back dirty data before zeroing range and punching >>>> hole, the delalloc extended file's disksize of should be updated >>>> properly when writing back pages, hence we don't need to update file's >>>> disksize before discarding page cache in ext4_zero_range() and >>>> ext4_punch_hole(), just drop ext4_update_disksize_before_punch(). >>>> >>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> >>> >>> So when we don't write out before hole punching & company this needs to stay >>> in some shape or form. >>> >> >> Thanks for taking time to review this series! >> >> I don't fully understand this comment, please let me confirm. Do you >> suggested that we still don't write out all the data before punching / >> zeroing / collapseing(i.e. drop patch 01), so we need to keep >> ext4_update_disksize_before_punch()(i.e. also drop this patch), is >> that right? > > Yes, this is what I meant. Sorry for not being clear. > > Honza > OK, this looks fine to me. Let me revise this series. Thanks, Yi.
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 08acd152261e..e8d7965f62c4 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -3414,9 +3414,6 @@ static inline int ext4_update_inode_size(struct inode *inode, loff_t newsize) return changed; } -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset, - loff_t len); - struct ext4_group_info { unsigned long bb_state; #ifdef AGGRESSIVE_CHECK diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 19a9b14935b7..d9fccf2970e9 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -4637,10 +4637,6 @@ static long ext4_zero_range(struct file *file, loff_t offset, flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN | EXT4_EX_NOCACHE); - ret = ext4_update_disksize_before_punch(inode, offset, len); - if (ret) - goto out_invalidate_lock; - /* Now release the pages and zero block aligned part of pages */ truncate_pagecache_range(inode, start, end - 1); diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 8af25442d44d..9343ce9f2b01 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3872,37 +3872,6 @@ int ext4_can_truncate(struct inode *inode) return 0; } -/* - * We have to make sure i_disksize gets properly updated before we truncate - * page cache due to hole punching or zero range. Otherwise i_disksize update - * can get lost as it may have been postponed to submission of writeback but - * that will never happen after we truncate page cache. - */ -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset, - loff_t len) -{ - handle_t *handle; - int ret; - - loff_t size = i_size_read(inode); - - WARN_ON(!inode_is_locked(inode)); - if (offset > size || offset + len < size) - return 0; - - if (EXT4_I(inode)->i_disksize >= size) - return 0; - - handle = ext4_journal_start(inode, EXT4_HT_MISC, 1); - if (IS_ERR(handle)) - return PTR_ERR(handle); - ext4_update_i_disksize(inode, size); - ret = ext4_mark_inode_dirty(handle, inode); - ext4_journal_stop(handle); - - return ret; -} - static void ext4_wait_dax_page(struct inode *inode) { filemap_invalidate_unlock(inode->i_mapping); @@ -4022,13 +3991,9 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) last_block_offset = round_down((offset + length), sb->s_blocksize) - 1; /* Now release the pages and zero block aligned part of pages*/ - if (last_block_offset > first_block_offset) { - ret = ext4_update_disksize_before_punch(inode, offset, length); - if (ret) - goto out_dio; + if (last_block_offset > first_block_offset) truncate_pagecache_range(inode, first_block_offset, last_block_offset); - } if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) credits = ext4_writepage_trans_blocks(inode);