| Message ID | 20220428200735.j3yxdpi6fgdptsph@fiona (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Remove unnecessary inode_need_compress() check | expand |
Apparently, BTRFS_INODE_NOCOMPRESS can change runtime, so this patch is incorrect. Sorry. On 15:07 28/04, Goldwyn Rodrigues wrote: > async extent writes are called only for compressed range. So, remove the > check for inode_need_compress() in compress_file_range() since it is > already checked in btrfs_run_delalloc_range(). > > Conditions for inode_can_compress() can be incorporated in > inode_need_compress() > > To make the code more elegant, change the condition order in > btrfs_run_delalloc_range(). > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > --- > fs/btrfs/inode.c | 126 ++++++++++++++++++++--------------------------- > 1 file changed, 54 insertions(+), 72 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 4dfc02fbbad5..a89768963d9f 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -480,17 +480,6 @@ static noinline int add_async_extent(struct async_chunk *cow, > return 0; > } > > -/* > - * Check if the inode has flags compatible with compression > - */ > -static inline bool inode_can_compress(struct btrfs_inode *inode) > -{ > - if (inode->flags & BTRFS_INODE_NODATACOW || > - inode->flags & BTRFS_INODE_NODATASUM) > - return false; > - return true; > -} > - > /* > * Check if the inode needs to be submitted to compression, based on mount > * options, defragmentation, properties or heuristics. > @@ -500,12 +489,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start, > { > struct btrfs_fs_info *fs_info = inode->root->fs_info; > > - if (!inode_can_compress(inode)) { > - WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG), > - KERN_ERR "BTRFS: unexpected compression for ino %llu\n", > - btrfs_ino(inode)); > + /* > + * Check if the inode has flags compatible with compression > + */ > + if (inode->flags & BTRFS_INODE_NODATACOW || > + inode->flags & BTRFS_INODE_NODATASUM) > return 0; > - } > + > /* > * Special check for subpage. > * > @@ -661,62 +651,55 @@ static noinline int compress_file_range(struct async_chunk *async_chunk) > total_in = 0; > ret = 0; > > - /* > - * we do compression for mount -o compress and when the > - * inode has not been flagged as nocompress. This flag can > - * change at any time if we discover bad compression ratios. > - */ > - if (inode_need_compress(BTRFS_I(inode), start, end)) { > - WARN_ON(pages); > - pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); > - if (!pages) { > - /* just bail out to the uncompressed code */ > - nr_pages = 0; > - goto cont; > - } > + WARN_ON(pages); > + pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); > + if (!pages) { > + /* just bail out to the uncompressed code */ > + nr_pages = 0; > + goto cont; > + } > > - if (BTRFS_I(inode)->defrag_compress) > - compress_type = BTRFS_I(inode)->defrag_compress; > - else if (BTRFS_I(inode)->prop_compress) > - compress_type = BTRFS_I(inode)->prop_compress; > + if (BTRFS_I(inode)->defrag_compress) > + compress_type = BTRFS_I(inode)->defrag_compress; > + else if (BTRFS_I(inode)->prop_compress) > + compress_type = BTRFS_I(inode)->prop_compress; > > - /* > - * we need to call clear_page_dirty_for_io on each > - * page in the range. Otherwise applications with the file > - * mmap'd can wander in and change the page contents while > - * we are compressing them. > - * > - * If the compression fails for any reason, we set the pages > - * dirty again later on. > - * > - * Note that the remaining part is redirtied, the start pointer > - * has moved, the end is the original one. > - */ > - if (!redirty) { > - extent_range_clear_dirty_for_io(inode, start, end); > - redirty = 1; > - } > + /* > + * we need to call clear_page_dirty_for_io on each > + * page in the range. Otherwise applications with the file > + * mmap'd can wander in and change the page contents while > + * we are compressing them. > + * > + * If the compression fails for any reason, we set the pages > + * dirty again later on. > + * > + * Note that the remaining part is redirtied, the start pointer > + * has moved, the end is the original one. > + */ > + if (!redirty) { > + extent_range_clear_dirty_for_io(inode, start, end); > + redirty = 1; > + } > > - /* Compression level is applied here and only here */ > - ret = btrfs_compress_pages( > + /* Compression level is applied here and only here */ > + ret = btrfs_compress_pages( > compress_type | (fs_info->compress_level << 4), > - inode->i_mapping, start, > - pages, > - &nr_pages, > - &total_in, > - &total_compressed); > + inode->i_mapping, start, > + pages, > + &nr_pages, > + &total_in, > + &total_compressed); > > - if (!ret) { > - unsigned long offset = offset_in_page(total_compressed); > - struct page *page = pages[nr_pages - 1]; > + if (!ret) { > + unsigned long offset = offset_in_page(total_compressed); > + struct page *page = pages[nr_pages - 1]; > > - /* zero the tail end of the last page, we might be > - * sending it down to disk > - */ > - if (offset) > - memzero_page(page, offset, PAGE_SIZE - offset); > - will_compress = 1; > - } > + /* zero the tail end of the last page, we might be > + * sending it down to disk > + */ > + if (offset) > + memzero_page(page, offset, PAGE_SIZE - offset); > + will_compress = 1; > } > cont: > /* > @@ -2019,18 +2002,17 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page > ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root)); > ret = run_delalloc_nocow(inode, locked_page, start, end, > page_started, nr_written); > - } else if (!inode_can_compress(inode) || > - !inode_need_compress(inode, start, end)) { > + } else if (inode_need_compress(inode, start, end)) { > + set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags); > + ret = cow_file_range_async(inode, wbc, locked_page, start, end, > + page_started, nr_written); > + } else { > if (zoned) > ret = run_delalloc_zoned(inode, locked_page, start, end, > page_started, nr_written); > else > ret = cow_file_range(inode, locked_page, start, end, > page_started, nr_written, 1); > - } else { > - set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags); > - ret = cow_file_range_async(inode, wbc, locked_page, start, end, > - page_started, nr_written); > } > ASSERT(ret <= 0); > if (ret) > -- > 2.35.3 > > > -- > Goldwyn
On 29.04.22 г. 17:41 ч., Goldwyn Rodrigues wrote: > Apparently, BTRFS_INODE_NOCOMPRESS can change runtime, so this patch is > incorrect. Sorry. > Yes, when doing writes on compressed-enabled filesystem if a particular file gets bad compression ratio that flag is getting set from compress_file_range: /* flag the file so we don't compress in the future */ 32 if (!btrfs_test_opt(fs_info, FORCE_COMPRESS) && 31 !(BTRFS_I(inode)->prop_compress)) { 30 BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS; 29 }
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 4dfc02fbbad5..a89768963d9f 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -480,17 +480,6 @@ static noinline int add_async_extent(struct async_chunk *cow, return 0; } -/* - * Check if the inode has flags compatible with compression - */ -static inline bool inode_can_compress(struct btrfs_inode *inode) -{ - if (inode->flags & BTRFS_INODE_NODATACOW || - inode->flags & BTRFS_INODE_NODATASUM) - return false; - return true; -} - /* * Check if the inode needs to be submitted to compression, based on mount * options, defragmentation, properties or heuristics. @@ -500,12 +489,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start, { struct btrfs_fs_info *fs_info = inode->root->fs_info; - if (!inode_can_compress(inode)) { - WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG), - KERN_ERR "BTRFS: unexpected compression for ino %llu\n", - btrfs_ino(inode)); + /* + * Check if the inode has flags compatible with compression + */ + if (inode->flags & BTRFS_INODE_NODATACOW || + inode->flags & BTRFS_INODE_NODATASUM) return 0; - } + /* * Special check for subpage. * @@ -661,62 +651,55 @@ static noinline int compress_file_range(struct async_chunk *async_chunk) total_in = 0; ret = 0; - /* - * we do compression for mount -o compress and when the - * inode has not been flagged as nocompress. This flag can - * change at any time if we discover bad compression ratios. - */ - if (inode_need_compress(BTRFS_I(inode), start, end)) { - WARN_ON(pages); - pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); - if (!pages) { - /* just bail out to the uncompressed code */ - nr_pages = 0; - goto cont; - } + WARN_ON(pages); + pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); + if (!pages) { + /* just bail out to the uncompressed code */ + nr_pages = 0; + goto cont; + } - if (BTRFS_I(inode)->defrag_compress) - compress_type = BTRFS_I(inode)->defrag_compress; - else if (BTRFS_I(inode)->prop_compress) - compress_type = BTRFS_I(inode)->prop_compress; + if (BTRFS_I(inode)->defrag_compress) + compress_type = BTRFS_I(inode)->defrag_compress; + else if (BTRFS_I(inode)->prop_compress) + compress_type = BTRFS_I(inode)->prop_compress; - /* - * we need to call clear_page_dirty_for_io on each - * page in the range. Otherwise applications with the file - * mmap'd can wander in and change the page contents while - * we are compressing them. - * - * If the compression fails for any reason, we set the pages - * dirty again later on. - * - * Note that the remaining part is redirtied, the start pointer - * has moved, the end is the original one. - */ - if (!redirty) { - extent_range_clear_dirty_for_io(inode, start, end); - redirty = 1; - } + /* + * we need to call clear_page_dirty_for_io on each + * page in the range. Otherwise applications with the file + * mmap'd can wander in and change the page contents while + * we are compressing them. + * + * If the compression fails for any reason, we set the pages + * dirty again later on. + * + * Note that the remaining part is redirtied, the start pointer + * has moved, the end is the original one. + */ + if (!redirty) { + extent_range_clear_dirty_for_io(inode, start, end); + redirty = 1; + } - /* Compression level is applied here and only here */ - ret = btrfs_compress_pages( + /* Compression level is applied here and only here */ + ret = btrfs_compress_pages( compress_type | (fs_info->compress_level << 4), - inode->i_mapping, start, - pages, - &nr_pages, - &total_in, - &total_compressed); + inode->i_mapping, start, + pages, + &nr_pages, + &total_in, + &total_compressed); - if (!ret) { - unsigned long offset = offset_in_page(total_compressed); - struct page *page = pages[nr_pages - 1]; + if (!ret) { + unsigned long offset = offset_in_page(total_compressed); + struct page *page = pages[nr_pages - 1]; - /* zero the tail end of the last page, we might be - * sending it down to disk - */ - if (offset) - memzero_page(page, offset, PAGE_SIZE - offset); - will_compress = 1; - } + /* zero the tail end of the last page, we might be + * sending it down to disk + */ + if (offset) + memzero_page(page, offset, PAGE_SIZE - offset); + will_compress = 1; } cont: /* @@ -2019,18 +2002,17 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root)); ret = run_delalloc_nocow(inode, locked_page, start, end, page_started, nr_written); - } else if (!inode_can_compress(inode) || - !inode_need_compress(inode, start, end)) { + } else if (inode_need_compress(inode, start, end)) { + set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags); + ret = cow_file_range_async(inode, wbc, locked_page, start, end, + page_started, nr_written); + } else { if (zoned) ret = run_delalloc_zoned(inode, locked_page, start, end, page_started, nr_written); else ret = cow_file_range(inode, locked_page, start, end, page_started, nr_written, 1); - } else { - set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags); - ret = cow_file_range_async(inode, wbc, locked_page, start, end, - page_started, nr_written); } ASSERT(ret <= 0); if (ret)
async extent writes are called only for compressed range. So, remove the check for inode_need_compress() in compress_file_range() since it is already checked in btrfs_run_delalloc_range(). Conditions for inode_can_compress() can be incorporated in inode_need_compress() To make the code more elegant, change the condition order in btrfs_run_delalloc_range(). Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> --- fs/btrfs/inode.c | 126 ++++++++++++++++++++--------------------------- 1 file changed, 54 insertions(+), 72 deletions(-)