Message ID | 1374099015-6829-1-git-send-email-jack@suse.cz |
---|---|
State | New, archived |
Headers | show |
Hi Jan, Sorry for the late reply. On Thu, Jul 18, 2013 at 12:10:15AM +0200, Jan Kara wrote: > ext4_da_page_release_reservation() gets called from > ext4_da_invalidatepage(). This function is used when we are truncating > page cache for punch hole or truncate operations. In either case these > operations take care of removing extents from the extent tree. This is > more efficient and the code in ext4_da_page_release_reservation() is > actually buggy anyway. So just remove it. I remember that I try to remove the entry from extent status tree here because at the end of this function it tries to relase the reserved space for delalloc. For 4k block we can simply release it because ->s_cluster_ratio == 1. But when bigalloc is enabled, we need to determine whether we can release the reserved space according to the result of ext4_find_delalloc_cluster() as the comment described. If we don't remove the entry from extent status tree here, we could lost some spaces that could be reused by other files. If I remember correctly, I have hitted a warning message when I run xfstests to test it. These days I try to trigger it using xfstests but I failed. Have you seen a prblem that is caused by this code? Maybe we need to refactor out the code and release the reserved space outside this function. - Zheng > > CC: Zheng Liu <wenqing.lz@taobao.com> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ext4/inode.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 0188e65..98a9972 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -1388,11 +1388,6 @@ static void ext4_da_page_release_reservation(struct page *page, > curr_off = next_off; > } while ((bh = bh->b_this_page) != head); > > - if (to_release) { > - lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits); > - ext4_es_remove_extent(inode, lblk, to_release); > - } > - > /* If we have released all the blocks belonging to a cluster, then we > * need to release the reserved space for that cluster. */ > num_clusters = EXT4_NUM_B2C(sbi, to_release); > -- > 1.8.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Zheng, On Fri 19-07-13 08:44:39, Zheng Liu wrote: > On Thu, Jul 18, 2013 at 12:10:15AM +0200, Jan Kara wrote: > > ext4_da_page_release_reservation() gets called from > > ext4_da_invalidatepage(). This function is used when we are truncating > > page cache for punch hole or truncate operations. In either case these > > operations take care of removing extents from the extent tree. This is > > more efficient and the code in ext4_da_page_release_reservation() is > > actually buggy anyway. So just remove it. > > I remember that I try to remove the entry from extent status tree here > because at the end of this function it tries to relase the reserved > space for delalloc. For 4k block we can simply release it because > ->s_cluster_ratio == 1. But when bigalloc is enabled, we need to > determine whether we can release the reserved space according to the > result of ext4_find_delalloc_cluster() as the comment described. If we > don't remove the entry from extent status tree here, we could lost some > spaces that could be reused by other files. If I remember correctly, I > have hitted a warning message when I run xfstests to test it. These > days I try to trigger it using xfstests but I failed. Have you seen a > prblem that is caused by this code? Maybe we need to refactor out the > code and release the reserved space outside this function. Ah, I see. No, I didn't observe any problem due to this code, I just didn't understand why is it there. Also when blocksize < pagesize, the code is wrong because delayed buffers to release need not be contiguous so ext4_es_remove_extent(inode, lblk, to_release) may not free all the buffers we want. But subsequent extent tree truncation in ext4_ext_truncate() hides this problem. So I think we might just change the condition: if (to_release) { to if (to_release && sbi->s_cluster_ratio > 1) { and add explanatory comment why cluster_ratio > 1 needs the truncation and other cases don't. It will also save some needlessly burned CPU cycles spent when manipulating extent tree. Honza
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 0188e65..98a9972 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1388,11 +1388,6 @@ static void ext4_da_page_release_reservation(struct page *page, curr_off = next_off; } while ((bh = bh->b_this_page) != head); - if (to_release) { - lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits); - ext4_es_remove_extent(inode, lblk, to_release); - } - /* If we have released all the blocks belonging to a cluster, then we * need to release the reserved space for that cluster. */ num_clusters = EXT4_NUM_B2C(sbi, to_release);
ext4_da_page_release_reservation() gets called from ext4_da_invalidatepage(). This function is used when we are truncating page cache for punch hole or truncate operations. In either case these operations take care of removing extents from the extent tree. This is more efficient and the code in ext4_da_page_release_reservation() is actually buggy anyway. So just remove it. CC: Zheng Liu <wenqing.lz@taobao.com> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/inode.c | 5 ----- 1 file changed, 5 deletions(-)