Just bunch of btrfs patches

Some patches has review, some not, all compile tested and hand tested. (i.e. boot into patched system and do some small tests).

All based on kDave for-next branch

Patches:
1. Just remove useless u64 num_bytes from compress_file_range() No functional changes
2. For make compression on on mmap'd files safe,
while compression logic works, we switch page dirty page bit on whole input range, but input range can be much bigger the 128KiB
So try optimize that by only switch bits on current compression range
3. Function:
extent_range_clear_dirty_for_io() extent_range_redirty_for_io()
btrfs_set_range_writeback()
Used to switch some bits on pages,
but use not obvious while (index <= end_index) to cover unaligned end to pages.
(I don't think that not obvious for me only, as on IRC no one can help me understand that until i found answer)
So i change handling of unaligned end to more obvious way
4. btrfs_dedupe_file_range() on range bigger then 16MiB
instead of return error, silently set it to 16MiB.
So just add loop over input range, to get working bigger range P.S. May be that make a sense to change loop iterator to some lower value if one of deduped files are compressed?

Thanks.

Timofey Titovets (4):
Btrfs: compress_file_range() remove dead variable num_bytes
Btrfs: clear_dirty only on pages in compression range
Btrfs: handle unaligned tail of data ranges more efficient
Btrfs: btrfs_dedupe_file_range() ioctl, remove 16MiB restriction

 fs/btrfs/extent_io.c | 12 ++++++++++--
 fs/btrfs/inode.c     | 43 ++++++++++++++++++++++++++++++-------------
 fs/btrfs/ioctl.c     | 22 ++++++++++++++++++----
 3 files changed, 58 insertions(+), 19 deletions(-)

--
2.14.2

Remove dead assigment of num_bytes

Also as num_bytes only used in will_compress block as
copy of total_in just replace that with total_in and drop num_bytes entire

---
 fs/btrfs/inode.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

index b728397ba6e1..237df8fdf7b8 100644
@@ -458,7 +458,6 @@ static noinline void compress_file_range(struct inode *inode,
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	u64 num_bytes;
 	u64 blocksize = fs_info->sectorsize;
 	u64 actual_end;
 	u64 isize = i_size_read(inode);

@@ -508,8 +507,6 @@ static noinline void compress_file_range(struct inode *inode,
 
 	total_compressed = min_t(unsigned long, total_compressed,
 			BTRFS_MAX_UNCOMPRESSED);
-	num_bytes = ALIGN(end - start + 1, blocksize);
-	num_bytes = max(blocksize,  num_bytes);
 	total_in = 0;
 	ret = 0;
 

@@ -628,7 +625,6 @@ static noinline void compress_file_range(struct inode *inode,
 		 */
 		total_in = ALIGN(total_in, PAGE_SIZE);
 		if (total_compressed + blocksize <= total_in) {
-			num_bytes = total_in;
 			*num_added += 1;
 
 			/*

@@ -636,12 +632,12 @@ static noinline void compress_file_range(struct inode *inode,
 			 * allocation on disk for these compressed pages, and
 			 * will submit them to the elevator.
 			 */
-			add_async_extent(async_cow, start, num_bytes,
+			add_async_extent(async_cow, start, total_in,
 					total_compressed, pages, nr_pages,
 					compress_type);
 
-			if (start + num_bytes < end) {
-				start += num_bytes;
+			if (start + total_in < end) {
+				start += total_in;
 				pages = NULL;
 				cond_resched();
 				goto again;

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-

Added to the queue.

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

---
 fs/btrfs/inode.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

index 237df8fdf7b8..b6e81bd650ea 100644
@@ -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;

--
2.14.2
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo

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.

2017-10-10 19:22 GMT+03:00 David Sterba <[email protected]>:

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.

Have a nice day,
Timofey.

At now while switch page bits in data ranges
we always hande +1 page, for cover case
where end of data range is not page aligned

Let's handle that case more obvious and efficient
Check end aligment directly and touch +1 page
only then needed

---
 fs/btrfs/extent_io.c | 12 ++++++++++--
 fs/btrfs/inode.c     |  6 +++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

index 0538bf85adc3..131b7d1df9f7 100644
@@ -1359,7 +1359,11 @@ void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
 	unsigned long end_index = end >> PAGE_SHIFT;
 	struct page *page;
 
-	while (index <= end_index) {
+	/* Don't miss unaligned end */
+	if (!IS_ALIGNED(end, PAGE_SIZE))
+		end_index++;
+
+	while (index < end_index) {
 		page = find_get_page(inode->i_mapping, index);
 		BUG_ON(!page); /* Pages should be in the extent_io_tree */
 		clear_page_dirty_for_io(page);

@@ -1374,7 +1378,11 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)
 	unsigned long end_index = end >> PAGE_SHIFT;
 	struct page *page;
 
-	while (index <= end_index) {
+	/* Don't miss unaligned end */
+	if (!IS_ALIGNED(end, PAGE_SIZE))
+		end_index++;
+
+	while (index < end_index) {
 		page = find_get_page(inode->i_mapping, index);
 		BUG_ON(!page); /* Pages should be in the extent_io_tree */
 		__set_page_dirty_nobuffers(page);

index b6e81bd650ea..b4974d969f67 100644
@@ -10799,7 +10799,11 @@ void btrfs_set_range_writeback(void *private_data, u64 start, u64 end)
 	unsigned long end_index = end >> PAGE_SHIFT;
 	struct page *page;
 
-	while (index <= end_index) {
+	/* Don't miss unaligned end */
+	if (!IS_ALIGNED(end, PAGE_SIZE))
+		end_index++;
+
+	while (index < end_index) {
 		page = find_get_page(inode->i_mapping, index);
 		ASSERT(page); /* Pages should be in the extent_io_tree */
 		set_page_writeback(page);

--
2.14.2
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-

The 'end' is inclusive and thus not aligned in most cases, ie. it's offset 4095 in the page, so the IS_ALIGNED is allways true and the code is equivalent to the existing condition (index <= end_index).

May be then just add a comment at least at one of that functions? Like:
/*
* Handle unaligned end, end is inclusive, so always unaligned */

Or something like:
/*
* It's obvious, kernel use paging, so range
* Almost always have aligned start like 0
* and unaligned end like 8192 - 1
*/

Or we assume that everybody who look at kernel code, must understood that basic things?

Thanks

2017-10-10 19:37 GMT+03:00 David Sterba <[email protected]>:

Have a nice day,
Timofey.

This would be best documented in the data structure definition, so we don't have to comment each and every test that compares something against the range.

Some level of understanding is required of course, but it's different for everybody. If the semantics or constraints of code/structures are not discoverable, then it should be documented. Adding comment-only patches is ok, but adding trivial or commenting on the obvious is pointless. IME the sense of what is/not trivial comes after reading a lot of code.

At now btrfs_dedupe_file_range() restricted to 16MiB range for limit locking time and memory requirement for dedup ioctl()

For too big input rage code silently set range to 16MiB

Let's remove that restriction by do iterating over dedup range. That's backward compatible and will not change anything for request less then 16MiB.

---
 fs/btrfs/ioctl.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

index 31407c62da63..4b468e5dfa11 100644
@@ -3200,11 +3200,9 @@ ssize_t btrfs_dedupe_file_range(struct file *src_file, u64 loff, u64 olen,
 	struct inode *src = file_inode(src_file);
 	struct inode *dst = file_inode(dst_file);
 	u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
+	u64 i, tail_len, chunk_count;
 	ssize_t res;
 
-	if (olen > BTRFS_MAX_DEDUPE_LEN)
-		olen = BTRFS_MAX_DEDUPE_LEN;
-
 	if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
 		/*
 		 * Btrfs does not support blocksize < page_size. As a

@@ -3214,7 +3212,23 @@ ssize_t btrfs_dedupe_file_range(struct file *src_file, u64 loff, u64 olen,
 		return -EINVAL;
 	}
 
-	res = btrfs_extent_same(src, loff, olen, dst, dst_loff);
+	tail_len = olen % BTRFS_MAX_DEDUPE_LEN;
+	chunk_count = div_u64(olen, BTRFS_MAX_DEDUPE_LEN);
+
+	for (i = 0; i < chunk_count; i++) {
+		res = btrfs_extent_same(src, loff, BTRFS_MAX_DEDUPE_LEN,
+					dst, dst_loff);
+		if (res)
+			return res;
+
+		loff += BTRFS_MAX_DEDUPE_LEN;
+		dst_loff += BTRFS_MAX_DEDUPE_LEN;
+	}
+
+	if (tail_len > 0)
+		res = btrfs_extent_same(src, loff, tail_len,
+					dst, dst_loff);
+
 	if (res)
 		return res;
 	return olen;

--
2.14.2
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-

This would make the ioctl more pleasant to use. So far I haven't found any problems to do the iteration. One possible speedup could be done to avoid the repeated allocations in btrfs_extent_same if we're going to iterate more than once.

As this would mean the 16MiB length restriction is gone, this needs to bubble up to the documentation
(http://man7.org/linux/man-pages/man2/ioctl_fideduperange.2.html)

Have you tested the behaviour with larger ranges?

Sorry, i just thinking that i can test that and send you some feedback, But for now, no time.
I will check that later and try adds memory reusing.

So, just ignore patches for now.

Thanks

2017-10-10 20:36 GMT+03:00 David Sterba <[email protected]>:

Have a nice day,
Timofey.