| Message ID | 20171003150604.19596-3-nefelim4ag@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 03, 2017 at 06:06:02PM +0300, Timofey Titovets wrote: > We need to call extent_range_clear_dirty_for_io() > on compression range to prevent application from changing > page content, while pages compressing. > > but "(end - start)" can be much (up to 1024 times) bigger > then compression range (BTRFS_MAX_UNCOMPRESSED), so optimize that > by calculating compression range for > that loop iteration, and flip bits only on that range I'm not sure this is safe to do. Compress_file_range gets the whole range [start,end] from async_cow_start, and other parts of code might rely on the status of the whole range, ie. not partially redirtied. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-10-10 19:22 GMT+03:00 David Sterba <dsterba@suse.cz>: > On Tue, Oct 03, 2017 at 06:06:02PM +0300, Timofey Titovets wrote: >> We need to call extent_range_clear_dirty_for_io() >> on compression range to prevent application from changing >> page content, while pages compressing. >> >> but "(end - start)" can be much (up to 1024 times) bigger >> then compression range (BTRFS_MAX_UNCOMPRESSED), so optimize that >> by calculating compression range for >> that loop iteration, and flip bits only on that range > > I'm not sure this is safe to do. Compress_file_range gets the whole > range [start,end] from async_cow_start, and other parts of code might > rely on the status of the whole range, ie. not partially redirtied. I check some kernel code, io path are complex =\. I see 3 approaches: 1. That used to signal upper level, that we fail to write that pages and on other sync() call, kernel can send that data again to writing. 2. We lock pages from any changes while processing data. 3. Serialize writes, i.e. if i understood correctly, that allow to not send down pages again for several sync requests. My code above will handle ok first and second case, and in theory will cause some issues with 3, but doesn't matter. The main design problem from my point of view for now, that we call that function many times in the loop, example: compress_file_range get: 0 - 1048576 extent_range_clear_dirty_for_io(), will get a call for: 0 - 1048576 131072 - 1048576 262144 - 1048576 ... & etc So, we can beat that in different way. I first think about move extent_range_clear_dirty_for_io() out of loop, but i think that a better approach. Whats about: if (!redirty) { extent_range_clear_dirty_for_io(inode, start, end); redirty = 1; } That will also cover all above cases, because that lock whole range, as before. But we only call that once, and that will fix design issue. That you think? Thanks.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 237df8fdf7b8..b6e81bd650ea 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -460,6 +460,7 @@ static noinline void compress_file_range(struct inode *inode, struct btrfs_root *root = BTRFS_I(inode)->root; u64 blocksize = fs_info->sectorsize; u64 actual_end; + u64 current_end; u64 isize = i_size_read(inode); int ret = 0; struct page **pages = NULL; @@ -505,6 +506,21 @@ static noinline void compress_file_range(struct inode *inode, (start > 0 || end + 1 < BTRFS_I(inode)->disk_i_size)) goto cleanup_and_bail_uncompressed; + /* + * We need to call extent_range_clear_dirty_for_io() + * on compression range to prevent application from changing + * page content, while pages compressing. + * + * but (end - start) can be much (up to 1024 times) bigger + * then compression range, so optimize that + * by calculating compression range for + * that iteration, and flip bits only on that range + */ + if (end - start > BTRFS_MAX_UNCOMPRESSED) + current_end = start + BTRFS_MAX_UNCOMPRESSED; + else + current_end = end; + total_compressed = min_t(unsigned long, total_compressed, BTRFS_MAX_UNCOMPRESSED); total_in = 0; @@ -515,7 +531,7 @@ static noinline void compress_file_range(struct inode *inode, * inode has not been flagged as nocompress. This flag can * change at any time if we discover bad compression ratios. */ - if (inode_need_compress(inode, start, end)) { + if (inode_need_compress(inode, start, current_end)) { WARN_ON(pages); pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); if (!pages) { @@ -530,14 +546,15 @@ static noinline void compress_file_range(struct inode *inode, /* * 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 + * page in compression 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. */ - extent_range_clear_dirty_for_io(inode, start, end); + extent_range_clear_dirty_for_io(inode, start, current_end); redirty = 1; /* Compression level is applied here and only here */ @@ -678,7 +695,7 @@ static noinline void compress_file_range(struct inode *inode, /* unlocked later on in the async handlers */ if (redirty) - extent_range_redirty_for_io(inode, start, end); + extent_range_redirty_for_io(inode, start, current_end); add_async_extent(async_cow, start, end - start + 1, 0, NULL, 0, BTRFS_COMPRESS_NONE); *num_added += 1;
We need to call extent_range_clear_dirty_for_io() on compression range to prevent application from changing page content, while pages compressing. but "(end - start)" can be much (up to 1024 times) bigger then compression range (BTRFS_MAX_UNCOMPRESSED), so optimize that by calculating compression range for that loop iteration, and flip bits only on that range v1 -> v2: - Make that more obviously and more safeprone v2 -> v3: - Rebased on: Btrfs: compress_file_range() remove dead variable num_bytes - Update change log - Add comments Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com> --- fs/btrfs/inode.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) -- 2.14.2 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html