[PATCH 0/4] Just bunch of btrfs patches

[PATCH 0/4] Just bunch of btrfs patches

[Timofey Titovets]

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

[PATCH 1/4] Btrfs: compress_file_range() remove dead variable num_bytes

[Timofey Titovets]

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

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b728397ba6e1..237df8fdf7b8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-

Re: [PATCH 1/4] Btrfs: compress_file_range() remove dead variable num_bytes

[David Sterba]

On Tue, Oct 03, 2017 at 06:06:01PM +0300, Timofey Titovets wrote:
> 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
> 
> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Added to the queue.

[PATCH 2/4] Btrfs: clear_dirty only on pages only in compression range

[Timofey Titovets]

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(-)

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;
--
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

Re: [PATCH 2/4] Btrfs: clear_dirty only on pages only in compression range

[David Sterba]

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.

Re: [PATCH 2/4] Btrfs: clear_dirty only on pages only in compression range

[Timofey Titovets]

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.
-- 
Have a nice day,
Timofey.

[PATCH 3/4] Btrfs: handle unaligned tail of data ranges more efficient

[Timofey Titovets]

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

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/extent_io.c | 12 ++++++++++--
 fs/btrfs/inode.c     |  6 +++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0538bf85adc3..131b7d1df9f7 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b6e81bd650ea..b4974d969f67 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-

Re: [PATCH 3/4] Btrfs: handle unaligned tail of data ranges more efficient

[David Sterba]

On Tue, Oct 03, 2017 at 06:06:03PM +0300, Timofey Titovets wrote:
> 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

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).

Re: [PATCH 3/4] Btrfs: handle unaligned tail of data ranges more efficient

[Timofey Titovets]

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 <dsterba@suse.cz>:
> On Tue, Oct 03, 2017 at 06:06:03PM +0300, Timofey Titovets wrote:
>> 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
>
> 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).



-- 
Have a nice day,
Timofey.

Re: [PATCH 3/4] Btrfs: handle unaligned tail of data ranges more efficient

[David Sterba]

On Mon, Oct 16, 2017 at 01:09:04AM +0300, Timofey Titovets wrote:
> May be then just add a comment at least at one of that functions?
> Like:
> /*
>  * Handle unaligned end, end is inclusive, so always unaligned
>  */

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.

> 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?

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.

[PATCH 4/4] Btrfs: btrfs_dedupe_file_range() ioctl, remove 16MiB restriction

[Timofey Titovets]

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.

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 fs/btrfs/ioctl.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 31407c62da63..4b468e5dfa11 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-

Re: [PATCH 4/4] Btrfs: btrfs_dedupe_file_range() ioctl, remove 16MiB restriction

[David Sterba]

On Tue, Oct 03, 2017 at 06:06:04PM +0300, Timofey Titovets wrote:
> 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.

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?

Re: [PATCH 4/4] Btrfs: btrfs_dedupe_file_range() ioctl, remove 16MiB restriction

[Timofey Titovets]

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 <dsterba@suse.cz>:
> On Tue, Oct 03, 2017 at 06:06:04PM +0300, Timofey Titovets wrote:
>> 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.
>
> 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?



-- 
Have a nice day,
Timofey.