On 15.05.2018 11:30, Qu Wenruo wrote: > > > On 2018年05月15日 16:21, Nikolay Borisov wrote: >> >> >> On 15.05.2018 10:36, Qu Wenruo wrote: >>> As btrfs(5) specified: >>> >>> Note >>> If nodatacow or nodatasum are enabled, compression is disabled. >>> >>> If NODATASUM or NODATACOW set, we should not compress the extent. >>> >>> Normally NODATACOW is detected properly in run_delalloc_range() so >>> compression won't happen for NODATACOW. >>> >>> However for NODATASUM we don't have any check, and it can cause >>> compressed extent without csum pretty easily, just by: >>> ------ >>> mkfs.btrfs -f $dev >>> mount $dev $mnt -o nodatasum >>> touch $mnt/foobar >>> mount -o remount,datasum,compress $mnt >>> xfs_io -f -c "pwrite 0 128K" $mnt/foobar >>> ------ >>> >>> And in fact, we have bug report about corrupted compressed extent >>> without proper data checksum so even RAID1 can't recover the corruption. >>> (https://bugzilla.kernel.org/show_bug.cgi?id=199707) >>> >>> Running compression without proper checksum could cause more damage when >>> corruption happens, so there is no need to allow compression for >>> NODATACSUM. >>> >>> Reported-by: James Harvey <jamespharvey20@xxxxxxxxx> >>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx> >>> --- >>> fs/btrfs/inode.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >>> index d241285a0d2a..dbef3f404559 100644 >>> --- a/fs/btrfs/inode.c >>> +++ b/fs/btrfs/inode.c >>> @@ -396,6 +396,14 @@ static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) >>> { >>> struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); >>> >>> + /* >>> + * Btrfs doesn't support compression without csum or CoW. >>> + * This should have the highest priority. >>> + */ >>> + if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW || >>> + BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM) >>> + return 0; >>> + >> >> How is this not buggy, given that if inode_need_compress as called from >> compress_file_range will return zero, meaning we jump to cont: label. >> Then in the case of an inline extent we can execute : > > In that case, you won't go into compress_file_range() at all. > > As the only caller of compress_file_range() is async_cow_start(), which > get queued in cow_file_range_async(). > > And cow_file_range_async() traces back to run_delalloc_range(). > Here we determine (basically) where some dirty range goes. > > The modification in inode_need_compress() mostly affects the decision in > run_delalloc_range(), so we won't go cow_file_range_async(), thus we > won't hit the problem you described. So you have re-iterated what I've described further below. This means it should be possible to remove the invocation of inode_need_compress in compress_file_range and simplify the code there, no? Perhaps will_compress can also be removed etc? As it stands currently it's spaghetti code. >> >> ret = cow_file_range_inline(inode, start, end, >> total_compressed, >> compress_type, pages); >> >> where compress_type would have been set at the beginning of the >> function unconditionally to fs_info->compress_type. >> >> For non-inline extents I guess we are ok, given that will_compress >> will not be set. However, this code is rather messy and I'm not sure >> it's well defined what's going to happen in this case with inline extents. >> >> OTOH, I think there is something fundamentally wrong in calling >> inode_need_compress in compress_file_range. I.e they work at different >> abstractions. IMO compress_file_range should only be called if we know >> we have to compress the range. >> >> So looking around the code in run_delalloc_range (the only function >> which calls cow_file_range_async) we already have : >> >> } else if (!inode_need_compress(inode, start, end)) { >> ret = cow_file_range(inode, locked_page, start, end, end, >> page_started, nr_written, 1, NULL); >> >> and in the else branch we have the cow_file_range_async. So the code >> is sort of half-way there to actually decoupling compression checking from >> performing the actual compression. >> >> >>> /* force compress */ >>> if (btrfs_test_opt(fs_info, FORCE_COMPRESS)) >>> return 1; >> >> One more thing, in inode_need_compress shouldn't the inode specific >> checks come first something like : >> >> >> static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) >> { >> struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); >> >> /* defrag ioctl */ >> if (BTRFS_I(inode)->defrag_compress) >> return 1; >> /* bad compression ratios */ >> if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS) >> return 0; > > Not exactly. > Force-compress should less us bypass bad compression ratio, so it should > be at least before ratio check. > > Thanks, > Qu > >> /* force compress */ >> if (btrfs_test_opt(fs_info, FORCE_COMPRESS)) >> return 1; >> if (btrfs_test_opt(fs_info, COMPRESS) || >> BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS || >> BTRFS_I(inode)->prop_compress) >> return btrfs_compress_heuristic(inode, start, end); >> return 0; >> } >> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
