[PATCH] missing initialization in btrfs_check_shared

[PATCH] missing initialization in btrfs_check_shared

[Edmund Nadolski]

This patch addresses an issue that causes fiemap to falsely
report a shared extent.  The test case is as follows:

# cat do_xfs_io
xfs_io -f -d -c "pwrite -b 16k 0 64k" -c "fiemap -v" /media/scratch/file5
sync
xfs_io  -c "fiemap -v" /media/scratch/file5

which gives the resulting output:

# . do_xfs_io
wrote 65536/65536 bytes at offset 0
64 KiB, 4 ops; 0.0000 sec (121.359 MiB/sec and 7766.9903 ops/sec)
/media/scratch/file5:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..127]:        24576..24703       128 0x2001
/media/scratch/file5:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..127]:        24576..24703       128   0x1

This is because btrfs_check_shared calls find_parent_nodes
repeatedly in a loop, passing a share_check struct to report
the count of shared extent. But btrfs_check_shared does not
re-initialize the count value to zero for subsequent calls
from the loop, resulting in a false share count value. This
is a regressive behavior from 4.13.

With proper re-initialization the test result is as follows:

# . do_xfs_io
wrote 65536/65536 bytes at offset 0
64 KiB, 4 ops; 0.0000 sec (110.035 MiB/sec and 7042.2535 ops/sec)
/media/scratch/file5:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..127]:        24576..24703       128   0x1
/media/scratch/file5:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..127]:        24576..24703       128   0x1

which corrects the regression.

Edmund Nadolski (1):
  btrfs: add missing initialization in btrfs_check_shared

 fs/btrfs/backref.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.10.2

[PATCH] btrfs: add missing initialization in btrfs_check_shared

[Edmund Nadolski]

btrfs_check_shared calls find_parent_nodes in a loop. In each
iteration it passes in a share_check struct to request a check for
a shared extent.  The share_check::share_count must be re-initialized
to zero for each iteration in order to avoid using a stale count
value from the previous iteration, thus causing a false
shared extent indication.

Signed-off-by: Edmund Nadolski <enadolski@suse.com>
---
 fs/btrfs/backref.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 4e89598..4a33448 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1519,6 +1519,7 @@ int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
 		if (!node)
 			break;
 		bytenr = node->val;
+		shared.share_count = 0;
 		cond_resched();
 	}
 
-- 
2.10.2

Re: [PATCH] btrfs: add missing initialization in btrfs_check_shared

[Nikolay Borisov]

On 14.03.2018 17:03, Edmund Nadolski wrote:
> btrfs_check_shared calls find_parent_nodes in a loop. In each
> iteration it passes in a share_check struct to request a check for
> a shared extent.  The share_check::share_count must be re-initialized
> to zero for each iteration in order to avoid using a stale count
> value from the previous iteration, thus causing a false
> shared extent indication.
> 
> Signed-off-by: Edmund Nadolski <enadolski@suse.com>

Is there a particular commit which introduced this regression (i.e. one
of the backref walking series)? It would be useful to know so that this
can be tagged with a Fixes: tag and perhaps stable.

Also your cover letter should go as a changelog in the patch so that we
have a proper history of how this fix came to be :)

> ---
>  fs/btrfs/backref.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 4e89598..4a33448 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1519,6 +1519,7 @@ int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
>  		if (!node)
>  			break;
>  		bytenr = node->val;
> +		shared.share_count = 0;
>  		cond_resched();
>  	}
>  
> 

Re: [PATCH] btrfs: add missing initialization in btrfs_check_shared

[David Sterba]

On Wed, Mar 14, 2018 at 09:03:11AM -0600, Edmund Nadolski wrote:
> btrfs_check_shared calls find_parent_nodes in a loop. In each
> iteration it passes in a share_check struct to request a check for
> a shared extent.  The share_check::share_count must be re-initialized
> to zero for each iteration in order to avoid using a stale count
> value from the previous iteration, thus causing a false
> shared extent indication.
> 
> Signed-off-by: Edmund Nadolski <enadolski@suse.com>

I've replaced the changelog with text from cover letter and added the
Fixes: reference to 3ec4d3238ab that introduces 'shared'.

Re: [PATCH] missing initialization in btrfs_check_shared

[Filipe Manana]

On Wed, Mar 14, 2018 at 3:03 PM, Edmund Nadolski <enadolski@suse.com> wrote:
> This patch addresses an issue that causes fiemap to falsely
> report a shared extent.  The test case is as follows:
>
> # cat do_xfs_io
> xfs_io -f -d -c "pwrite -b 16k 0 64k" -c "fiemap -v" /media/scratch/file5
> sync
> xfs_io  -c "fiemap -v" /media/scratch/file5
>
> which gives the resulting output:
>
> # . do_xfs_io
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 4 ops; 0.0000 sec (121.359 MiB/sec and 7766.9903 ops/sec)
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128 0x2001
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128   0x1
>
> This is because btrfs_check_shared calls find_parent_nodes
> repeatedly in a loop, passing a share_check struct to report
> the count of shared extent. But btrfs_check_shared does not
> re-initialize the count value to zero for subsequent calls
> from the loop, resulting in a false share count value. This
> is a regressive behavior from 4.13.
>
> With proper re-initialization the test result is as follows:
>
> # . do_xfs_io
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 4 ops; 0.0000 sec (110.035 MiB/sec and 7042.2535 ops/sec)
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128   0x1
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128   0x1
>
> which corrects the regression.

All this information, about reproducer, which kernel/commit introduced
the regression, etc, should go into the changelog of the patch, not
the cover letter, since the cover letter won't go to the git
repository.

>
> Edmund Nadolski (1):
>   btrfs: add missing initialization in btrfs_check_shared
>
>  fs/btrfs/backref.c | 1 +
>  1 file changed, 1 insertion(+)
>
> --
> 2.10.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



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] missing initialization in btrfs_check_shared

[Liu Bo]

On Wed, Mar 14, 2018 at 8:03 AM, Edmund Nadolski <enadolski@suse.com> wrote:
> This patch addresses an issue that causes fiemap to falsely
> report a shared extent.  The test case is as follows:
>
> # cat do_xfs_io
> xfs_io -f -d -c "pwrite -b 16k 0 64k" -c "fiemap -v" /media/scratch/file5
> sync
> xfs_io  -c "fiemap -v" /media/scratch/file5
>
> which gives the resulting output:
>
> # . do_xfs_io
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 4 ops; 0.0000 sec (121.359 MiB/sec and 7766.9903 ops/sec)
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128 0x2001
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128   0x1
>
> This is because btrfs_check_shared calls find_parent_nodes
> repeatedly in a loop, passing a share_check struct to report
> the count of shared extent. But btrfs_check_shared does not
> re-initialize the count value to zero for subsequent calls
> from the loop, resulting in a false share count value. This
> is a regressive behavior from 4.13.
>
> With proper re-initialization the test result is as follows:
>
> # . do_xfs_io
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 4 ops; 0.0000 sec (110.035 MiB/sec and 7042.2535 ops/sec)
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128   0x1
> /media/scratch/file5:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        24576..24703       128   0x1
>
> which corrects the regression.
>

A fstest case for this would be appreciated.

thanks,
liubo