[PATCH 0/5] Misc cleanups

[PATCH 0/5] Misc cleanups

[Nikolay Borisov]

Here's a bunch of stuff that coverity found, this survived a full xfstest run.

Nikolay Borisov (5):
  btrfs: Remove dead code
  btrfs: Remove dead code
  btrfs: Fix possible off-by-one in  btrfs_search_path_in_tree
  btrfs: Remove redundant NULL check
  btrfs: Greatly simplify btrfs_read_dev_super

 fs/btrfs/disk-io.c | 27 ++++-----------------------
 fs/btrfs/inode.c   | 12 ++----------
 fs/btrfs/ioctl.c   |  6 +++---
 fs/btrfs/sysfs.c   |  2 --
 4 files changed, 9 insertions(+), 38 deletions(-)

-- 
2.7.4

[PATCH 1/5] btrfs: Remove dead code

[Nikolay Borisov]

trans was statically assigned to NULL and this never changed over the course of
btrfs_get_extent. So remove any code which checks whether trans != NULL and
just hardcode the fact trans is always NULL. This fixes CID#112806

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 57785eadb95c..92d140b06271 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6943,7 +6943,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	struct extent_map *em = NULL;
 	struct extent_map_tree *em_tree = &inode->extent_tree;
 	struct extent_io_tree *io_tree = &inode->io_tree;
-	struct btrfs_trans_handle *trans = NULL;
 	const bool new_inline = !page || create;
 
 	read_lock(&em_tree->lock);
@@ -6984,8 +6983,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		path->reada = READA_FORWARD;
 	}
 
-	ret = btrfs_lookup_file_extent(trans, root, path,
-				       objectid, start, trans != NULL);
+	ret = btrfs_lookup_file_extent(NULL, root, path, objectid, start, 0);
 	if (ret < 0) {
 		err = ret;
 		goto out;
@@ -7181,11 +7179,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	trace_btrfs_get_extent(root, inode, em);
 
 	btrfs_free_path(path);
-	if (trans) {
-		ret = btrfs_end_transaction(trans);
-		if (!err)
-			err = ret;
-	}
 	if (err) {
 		free_extent_map(em);
 		return ERR_PTR(err);
-- 
2.7.4

Re: [PATCH 1/5] btrfs: Remove dead code

[David Sterba]

On Fri, Dec 01, 2017 at 11:19:40AM +0200, Nikolay Borisov wrote:
> trans was statically assigned to NULL and this never changed over the course of
> btrfs_get_extent. So remove any code which checks whether trans != NULL and
> just hardcode the fact trans is always NULL. This fixes CID#112806
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Please find a better subject, at least mention the fuction name write
something more specific about the dead code that's being removed.

[PATCH] btrfs: Remove dead code btrfs_get_extent

[Nikolay Borisov]

trans was statically assigned to NULL and this never changes over the course of
btrfs_get_extent. This was added in commit a52d9a8033c4 ("Btrfs: Extent based
page cache code.  This uses an rbtree of extents and tests instead of buffer
heads.") and wasn't used even then, which indicates it was never intended to be
merged. So remove any code which checks whether trans != NULL and
just hardcode the fact trans is always NULL. This fixes CID#112806

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 57785eadb95c..92d140b06271 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6943,7 +6943,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	struct extent_map *em = NULL;
 	struct extent_map_tree *em_tree = &inode->extent_tree;
 	struct extent_io_tree *io_tree = &inode->io_tree;
-	struct btrfs_trans_handle *trans = NULL;
 	const bool new_inline = !page || create;
 
 	read_lock(&em_tree->lock);
@@ -6984,8 +6983,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		path->reada = READA_FORWARD;
 	}
 
-	ret = btrfs_lookup_file_extent(trans, root, path,
-				       objectid, start, trans != NULL);
+	ret = btrfs_lookup_file_extent(NULL, root, path, objectid, start, 0);
 	if (ret < 0) {
 		err = ret;
 		goto out;
@@ -7181,11 +7179,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	trace_btrfs_get_extent(root, inode, em);
 
 	btrfs_free_path(path);
-	if (trans) {
-		ret = btrfs_end_transaction(trans);
-		if (!err)
-			err = ret;
-	}
 	if (err) {
 		free_extent_map(em);
 		return ERR_PTR(err);
-- 
2.7.4

[PATCH 2/5] btrfs: Remove dead code

[Nikolay Borisov]

'clear' is always set to 0 (BTRFS_FEATURE_COMPAT_SAFE_CLEAR,
BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR and BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR are
all defined to 0). So remove the code that logically can never execute.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/sysfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index a8bafed931f4..37dbf2fccedc 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -84,8 +84,6 @@ static int can_modify_feature(struct btrfs_feature_attr *fa)
 
 	if (set & fa->feature_bit)
 		val |= 1;
-	if (clear & fa->feature_bit)
-		val |= 2;
 
 	return val;
 }
-- 
2.7.4

Re: [PATCH 2/5] btrfs: Remove dead code

[David Sterba]

On Fri, Dec 01, 2017 at 11:19:41AM +0200, Nikolay Borisov wrote:
> 'clear' is always set to 0 (BTRFS_FEATURE_COMPAT_SAFE_CLEAR,
> BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR and BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR are
> all defined to 0). So remove the code that logically can never execute.

The point of the macros is to wrap all values that can mean
"compatibility, safe to clear", similar to BTRFS_FEATURE_COMPAT_RO_SUPP
etc. So when we have such feature, we'll modify the defines and the rest
of the code will work.

But with your change the 'safe clear' would have to be patched again. The
macros provide complete interface, though some of the bits are not used.
A single line of unused code is IMHO not that significant that we have
to remove it and silently break the implementation.

Re: [PATCH 2/5] btrfs: Remove dead code

[Nikolay Borisov]

On  4.12.2017 15:50, David Sterba wrote:
> On Fri, Dec 01, 2017 at 11:19:41AM +0200, Nikolay Borisov wrote:
>> 'clear' is always set to 0 (BTRFS_FEATURE_COMPAT_SAFE_CLEAR,
>> BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR and BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR are
>> all defined to 0). So remove the code that logically can never execute.
> 
> The point of the macros is to wrap all values that can mean
> "compatibility, safe to clear", similar to BTRFS_FEATURE_COMPAT_RO_SUPP
> etc. So when we have such feature, we'll modify the defines and the rest
> of the code will work.

Fair enough feel free to drop it.

> 
> But with your change the 'safe clear' would have to be patched again. The
> macros provide complete interface, though some of the bits are not used.
> A single line of unused code is IMHO not that significant that we have
> to remove it and silently break the implementation.
> 

[PATCH 3/5] btrfs: Fix possible off-by-one in btrfs_search_path_in_tree

[Nikolay Borisov]

The name char array passed to btrfs_search_path_in_tree is of size
BTRFS_INO_LOOKUP_PATH_MAX (4080). So the actual accessible char indexes
are in the range of [0, 4079]. Currently the code uses the define but this
represents an off-by-one.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e8adebc8c1b0..fc148b7c4265 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2206,7 +2206,7 @@ static noinline int btrfs_search_path_in_tree(struct btrfs_fs_info *info,
 	if (!path)
 		return -ENOMEM;
 
-	ptr = &name[BTRFS_INO_LOOKUP_PATH_MAX];
+	ptr = &name[BTRFS_INO_LOOKUP_PATH_MAX - 1];
 
 	key.objectid = tree_id;
 	key.type = BTRFS_ROOT_ITEM_KEY;
@@ -2272,8 +2272,8 @@ static noinline int btrfs_search_path_in_tree(struct btrfs_fs_info *info,
 static noinline int btrfs_ioctl_ino_lookup(struct file *file,
 					   void __user *argp)
 {
-	 struct btrfs_ioctl_ino_lookup_args *args;
-	 struct inode *inode;
+	struct btrfs_ioctl_ino_lookup_args *args;
+	struct inode *inode;
 	int ret = 0;
 
 	args = memdup_user(argp, sizeof(*args));
-- 
2.7.4

Re: [PATCH 3/5] btrfs: Fix possible off-by-one in btrfs_search_path_in_tree

[David Sterba]

On Fri, Dec 01, 2017 at 11:19:42AM +0200, Nikolay Borisov wrote:
> The name char array passed to btrfs_search_path_in_tree is of size
> BTRFS_INO_LOOKUP_PATH_MAX (4080). So the actual accessible char indexes
> are in the range of [0, 4079]. Currently the code uses the define but this
> represents an off-by-one.

For such fixes it's good to think about and document potential
implications, like where could the extra byte end be written, and if this
is a patch stable.

Size of btrfs_ioctl_ino_lookup_args is 4096, so the new byte will be
written to extra space, not some padding that could be provided by the
allocator.

btrfs-progs store the arguments on stack, but kernel does own copy of
the ioctl buffer and the off-by-one overwrite does not affect userspace,
but the ending 0 might be lost.

kernel ioctl buffer is allocated dynamically so we're overwriting
somebody else's memory, and the ioctl is privileged if args.objectid is
not 256. Which is in most cases, but resolving a subvolume stored in
another directory will trigger that path.

Fixes: ac8e9819d71f907 ("Btrfs: add search and inode lookup ioctls")

Before this patch the buffer was one byte larger, but then the -1 was
not added.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ioctl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index e8adebc8c1b0..fc148b7c4265 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2206,7 +2206,7 @@ static noinline int btrfs_search_path_in_tree(struct btrfs_fs_info *info,
>  	if (!path)
>  		return -ENOMEM;
>  
> -	ptr = &name[BTRFS_INO_LOOKUP_PATH_MAX];
> +	ptr = &name[BTRFS_INO_LOOKUP_PATH_MAX - 1];
>  
>  	key.objectid = tree_id;
>  	key.type = BTRFS_ROOT_ITEM_KEY;
> @@ -2272,8 +2272,8 @@ static noinline int btrfs_search_path_in_tree(struct btrfs_fs_info *info,
>  static noinline int btrfs_ioctl_ino_lookup(struct file *file,
>  					   void __user *argp)
>  {
> -	 struct btrfs_ioctl_ino_lookup_args *args;
> -	 struct inode *inode;
> +	struct btrfs_ioctl_ino_lookup_args *args;
> +	struct inode *inode;

Unrelated change.

>  	int ret = 0;
>  
>  	args = memdup_user(argp, sizeof(*args));
> -- 
> 2.7.4
> 
> --
> 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

[PATCH 4/5] btrfs: Remove redundant NULL check

[Nikolay Borisov]

Before returning hole_em in btrfs_get_fiemap_extent we check if it's different
than null. However, by the time this null check is triggered we already know
hole_em is not null because it means it points to the em we found and it
has already been dereferenced.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 92d140b06271..9e0473c883ce 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7300,9 +7300,8 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 			em->block_start = EXTENT_MAP_DELALLOC;
 			em->block_len = found;
 		}
-	} else if (hole_em) {
+	} else
 		return hole_em;
-	}
 out:
 
 	free_extent_map(hole_em);
-- 
2.7.4

Re: [PATCH 4/5] btrfs: Remove redundant NULL check

[David Sterba]

On Fri, Dec 01, 2017 at 11:19:43AM +0200, Nikolay Borisov wrote:
> Before returning hole_em in btrfs_get_fiemap_extent we check if it's different
> than null. However, by the time this null check is triggered we already know
> hole_em is not null because it means it points to the em we found and it
> has already been dereferenced.

Well, ok The whole function could use some cleanups and reordering,
it's hard to track all the variable changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 92d140b06271..9e0473c883ce 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7300,9 +7300,8 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
>  			em->block_start = EXTENT_MAP_DELALLOC;
>  			em->block_len = found;
>  		}
> -	} else if (hole_em) {
> +	} else
>  		return hole_em;

The { } should stay. I'll fix it.

> -	}
>  out:
>  
>  	free_extent_map(hole_em);
> -- 
> 2.7.4
> 
> --
> 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

[PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[Nikolay Borisov]

Currently this function executes the inner loop at most 1 due to the i = 0;
i < 1 condition. Furthermore, the btrfs_super_generation(super) > transid code
in the if condition is never executed due to latest always set to NULL hence the
first part of the condition always triggering. The gist of btrfs_read_dev_super
is really to read the first superblock.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 82c96607fc46..6d5f632fd1e7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
 struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
 {
 	struct buffer_head *bh;
-	struct buffer_head *latest = NULL;
-	struct btrfs_super_block *super;
-	int i;
-	u64 transid = 0;
-	int ret = -EINVAL;
+	int ret;
 
 	/* we would like to check all the supers, but that would make
 	 * a btrfs mount succeed after a mkfs from a different FS.
 	 * So, we need to add a special mount option to scan for
 	 * later supers, using BTRFS_SUPER_MIRROR_MAX instead
 	 */
-	for (i = 0; i < 1; i++) {
-		ret = btrfs_read_dev_one_super(bdev, i, &bh);
-		if (ret)
-			continue;
-
-		super = (struct btrfs_super_block *)bh->b_data;
-
-		if (!latest || btrfs_super_generation(super) > transid) {
-			brelse(latest);
-			latest = bh;
-			transid = btrfs_super_generation(super);
-		} else {
-			brelse(bh);
-		}
-	}
-
-	if (!latest)
+	ret = btrfs_read_dev_one_super(bdev, 0, &bh);
+	if (ret)
 		return ERR_PTR(ret);
 
-	return latest;
+	return bh;
 }
 
 /*
-- 
2.7.4

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[Anand Jain]

On 12/01/2017 05:19 PM, Nikolay Borisov wrote:
> Currently this function executes the inner loop at most 1 due to the i = 0;
> i < 1 condition. Furthermore, the btrfs_super_generation(super) > transid code
> in the if condition is never executed due to latest always set to NULL hence the
> first part of the condition always triggering. The gist of btrfs_read_dev_super
> is really to read the first superblock.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/disk-io.c | 27 ++++-----------------------
>   1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 82c96607fc46..6d5f632fd1e7 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>   struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
>   {
>   	struct buffer_head *bh;
> -	struct buffer_head *latest = NULL;
> -	struct btrfs_super_block *super;
> -	int i;
> -	u64 transid = 0;
> -	int ret = -EINVAL;
> +	int ret;
>   
>   	/* we would like to check all the supers, but that would make
>   	 * a btrfs mount succeed after a mkfs from a different FS.
>   	 * So, we need to add a special mount option to scan for
>   	 * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>   	 */

  We need below loop to support the above comment at some point,
  instead of removing I would prefer to fix as per above comments.

Thanks, Anand


> -	for (i = 0; i < 1; i++) {
> -		ret = btrfs_read_dev_one_super(bdev, i, &bh);
> -		if (ret)
> -			continue;
> -
> -		super = (struct btrfs_super_block *)bh->b_data;
> -
> -		if (!latest || btrfs_super_generation(super) > transid) {
> -			brelse(latest);
> -			latest = bh;
> -			transid = btrfs_super_generation(super);
> -		} else {
> -			brelse(bh);
> -		}
> -	}
> -
> -	if (!latest)
> +	ret = btrfs_read_dev_one_super(bdev, 0, &bh);
> +	if (ret)
>   		return ERR_PTR(ret);
>   
> -	return latest;
> +	return bh;
>   }
>   
>   /*
> 

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[Nikolay Borisov]

On  2.12.2017 01:23, Anand Jain wrote:
> 
> 
> On 12/01/2017 05:19 PM, Nikolay Borisov wrote:
>> Currently this function executes the inner loop at most 1 due to the i
>> = 0;
>> i < 1 condition. Furthermore, the btrfs_super_generation(super) >
>> transid code
>> in the if condition is never executed due to latest always set to NULL
>> hence the
>> first part of the condition always triggering. The gist of
>> btrfs_read_dev_super
>> is really to read the first superblock.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>   fs/btrfs/disk-io.c | 27 ++++-----------------------
>>   1 file changed, 4 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 82c96607fc46..6d5f632fd1e7 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct
>> block_device *bdev, int copy_num,
>>   struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
>>   {
>>       struct buffer_head *bh;
>> -    struct buffer_head *latest = NULL;
>> -    struct btrfs_super_block *super;
>> -    int i;
>> -    u64 transid = 0;
>> -    int ret = -EINVAL;
>> +    int ret;
>>         /* we would like to check all the supers, but that would make
>>        * a btrfs mount succeed after a mkfs from a different FS.
>>        * So, we need to add a special mount option to scan for
>>        * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>>        */
> 
>  We need below loop to support the above comment at some point,

And when is that, since I don't see anyone working on it. Furthermore
what is it that we are losing in terms of functionality by not
supporting the comment? It seems this code was just slapt here without
any vision how/when to implement it?

Furthermore, you seem to be aware of what the comment is talking about,
I have to admit I'm not. Is the idea that if another filesystem does
mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at
64k, 64mb, 256gb and 1 PiB) then it's possible for this code to
erroneously detect btrfs when in fact there is a different fs?

I don't understand what problem *should* be solved here...

>  instead of removing I would prefer to fix as per above comments.
> 
> Thanks, Anand
> 
> 
>> -    for (i = 0; i < 1; i++) {
>> -        ret = btrfs_read_dev_one_super(bdev, i, &bh);
>> -        if (ret)
>> -            continue;
>> -
>> -        super = (struct btrfs_super_block *)bh->b_data;
>> -
>> -        if (!latest || btrfs_super_generation(super) > transid) {
>> -            brelse(latest);
>> -            latest = bh;
>> -            transid = btrfs_super_generation(super);
>> -        } else {
>> -            brelse(bh);
>> -        }
>> -    }
>> -
>> -    if (!latest)
>> +    ret = btrfs_read_dev_one_super(bdev, 0, &bh);
>> +    if (ret)
>>           return ERR_PTR(ret);
>>   -    return latest;
>> +    return bh;
>>   }
>>     /*
>>
> 

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[Anand Jain]

On 12/03/2017 05:43 PM, Nikolay Borisov wrote:
> 
> 
> On  2.12.2017 01:23, Anand Jain wrote:
>>
>>
>> On 12/01/2017 05:19 PM, Nikolay Borisov wrote:
>>> Currently this function executes the inner loop at most 1 due to the i
>>> = 0;
>>> i < 1 condition. Furthermore, the btrfs_super_generation(super) >
>>> transid code
>>> in the if condition is never executed due to latest always set to NULL
>>> hence the
>>> first part of the condition always triggering. The gist of
>>> btrfs_read_dev_super
>>> is really to read the first superblock.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>    fs/btrfs/disk-io.c | 27 ++++-----------------------
>>>    1 file changed, 4 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 82c96607fc46..6d5f632fd1e7 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct
>>> block_device *bdev, int copy_num,
>>>    struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
>>>    {
>>>        struct buffer_head *bh;
>>> -    struct buffer_head *latest = NULL;
>>> -    struct btrfs_super_block *super;
>>> -    int i;
>>> -    u64 transid = 0;
>>> -    int ret = -EINVAL;
>>> +    int ret;
>>>          /* we would like to check all the supers, but that would make
>>>         * a btrfs mount succeed after a mkfs from a different FS.
>>>         * So, we need to add a special mount option to scan for
>>>         * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>>>         */
>>
>>   We need below loop to support the above comment at some point,
> 
> And when is that, since I don't see anyone working on it. 

> Furthermore
> what is it that we are losing in terms of functionality by not
> supporting the comment? 

  As of now if the primary SB is corrupted we don't recover from it
  automatically and external tools are broken.

>It seems this code was just slapt here without
> any vision how/when to implement it?

  I have in my todo list. I am ok if you want to fix it as needed.

> Furthermore, you seem to be aware of what the comment is talking about,
> I have to admit I'm not.

> Is the idea that if another filesystem does
> mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at
> 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to
> erroneously detect btrfs when in fact there is a different fs?

  Right.

  IMO the above comment is wrong as well for which it made the
  for-loop to read only primary SB.

  If we have a feature to maintain backup SB, then that feature
  is only complete when we would automatically recover from the
  backup SB.
  If a user overwrites btrfs primary SB and still mounts btrfs
  with -t btrfs options, then its use-end problem we should be
  able to recover from backup SB. So IMO looping through other
  SB is fine.

Thanks, Anand

> I don't understand what problem *should* be solved here...

>>   instead of removing I would prefer to fix as per above comments.
>>
>> Thanks, Anand
>>
>>
>>> -    for (i = 0; i < 1; i++) {
>>> -        ret = btrfs_read_dev_one_super(bdev, i, &bh);
>>> -        if (ret)
>>> -            continue;
>>> -
>>> -        super = (struct btrfs_super_block *)bh->b_data;
>>> -
>>> -        if (!latest || btrfs_super_generation(super) > transid) {
>>> -            brelse(latest);
>>> -            latest = bh;
>>> -            transid = btrfs_super_generation(super);
>>> -        } else {
>>> -            brelse(bh);
>>> -        }
>>> -    }
>>> -
>>> -    if (!latest)
>>> +    ret = btrfs_read_dev_one_super(bdev, 0, &bh);
>>> +    if (ret)
>>>            return ERR_PTR(ret);
>>>    -    return latest;
>>> +    return bh;
>>>    }
>>>      /*
>>>
>>
> --
> 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
> 

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[David Sterba]

On Sun, Dec 03, 2017 at 11:43:13AM +0200, Nikolay Borisov wrote:
> On  2.12.2017 01:23, Anand Jain wrote:
> > On 12/01/2017 05:19 PM, Nikolay Borisov wrote:
> >> Currently this function executes the inner loop at most 1 due to the i
> >> = 0;
> >> i < 1 condition. Furthermore, the btrfs_super_generation(super) >
> >> transid code
> >> in the if condition is never executed due to latest always set to NULL
> >> hence the
> >> first part of the condition always triggering. The gist of
> >> btrfs_read_dev_super
> >> is really to read the first superblock.
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>   fs/btrfs/disk-io.c | 27 ++++-----------------------
> >>   1 file changed, 4 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >> index 82c96607fc46..6d5f632fd1e7 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct
> >> block_device *bdev, int copy_num,
> >>   struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
> >>   {
> >>       struct buffer_head *bh;
> >> -    struct buffer_head *latest = NULL;
> >> -    struct btrfs_super_block *super;
> >> -    int i;
> >> -    u64 transid = 0;
> >> -    int ret = -EINVAL;
> >> +    int ret;
> >>         /* we would like to check all the supers, but that would make
> >>        * a btrfs mount succeed after a mkfs from a different FS.
> >>        * So, we need to add a special mount option to scan for
> >>        * later supers, using BTRFS_SUPER_MIRROR_MAX instead
> >>        */
> > 
> >  We need below loop to support the above comment at some point,
> 
> And when is that, since I don't see anyone working on it. Furthermore
> what is it that we are losing in terms of functionality by not
> supporting the comment? It seems this code was just slapt here without
> any vision how/when to implement it?
> 
> Furthermore, you seem to be aware of what the comment is talking about,
> I have to admit I'm not. Is the idea that if another filesystem does
> mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at
> 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to
> erroneously detect btrfs when in fact there is a different fs?
> 
> I don't understand what problem *should* be solved here...

Without any further checks and validation, we cannot simply iterate over
all superblocks and try to mount anything that looks ok. Even if the
offsets exist on the block device.  The fsid should be at least checked,
but that's still not enough to ensure the 1st copy is what we want to
mount.

In that case it's up to the user to deal with the problem vs automating
all fallbacks in the kernel by default. We could enhance the recovery
options (usebackuproot) to look at the other superblocks.

An example scenario:

- mkfs, contains 2 superblocks (primary and 1st copy)
- shrink the filesystem, 1st copy contains stale version of the same
  filesystem
- mount, primary is broken, use 1st backup copy -- now, this could
  actually succeed if all metadata pointed by the 1st copy exist and are
  consistent and we'd end up with an older version of the filesystem
  metadata, and partially data


Another example, and this was probably meant by the comment:

- mkfs filesystem, contains primary and 1st (or more) superblock copy
- mkfs again, now contains fewer sb copies
- mount, similar situation as above, though the fsid will mismatch
  (provided we're able to read the primary sb at all)


The commit introducing the comment is from 2008, nobody saw the need to
add the validation. And from the usability POV, the current behaviour
looks better because the user/admin has a chance to analyze the problem
first, run fsck before mounting from the sb copies and then use
'btrfs-select-super' to overwrite the primary sb with the backup copy.

If there are clear guarantees and enough validation for the sb copy use,
then fine, otherwise we can apply this patch and extend the comment so
it's documented why we don't iterate.

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[Nikolay Borisov]

On  4.12.2017 16:13, David Sterba wrote:
> On Sun, Dec 03, 2017 at 11:43:13AM +0200, Nikolay Borisov wrote:
>> On  2.12.2017 01:23, Anand Jain wrote:
>>> On 12/01/2017 05:19 PM, Nikolay Borisov wrote:
>>>> Currently this function executes the inner loop at most 1 due to the i
>>>> = 0;
>>>> i < 1 condition. Furthermore, the btrfs_super_generation(super) >
>>>> transid code
>>>> in the if condition is never executed due to latest always set to NULL
>>>> hence the
>>>> first part of the condition always triggering. The gist of
>>>> btrfs_read_dev_super
>>>> is really to read the first superblock.
>>>>
>>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>>> ---
>>>>   fs/btrfs/disk-io.c | 27 ++++-----------------------
>>>>   1 file changed, 4 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index 82c96607fc46..6d5f632fd1e7 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct
>>>> block_device *bdev, int copy_num,
>>>>   struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
>>>>   {
>>>>       struct buffer_head *bh;
>>>> -    struct buffer_head *latest = NULL;
>>>> -    struct btrfs_super_block *super;
>>>> -    int i;
>>>> -    u64 transid = 0;
>>>> -    int ret = -EINVAL;
>>>> +    int ret;
>>>>         /* we would like to check all the supers, but that would make
>>>>        * a btrfs mount succeed after a mkfs from a different FS.
>>>>        * So, we need to add a special mount option to scan for
>>>>        * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>>>>        */
>>>
>>>  We need below loop to support the above comment at some point,
>>
>> And when is that, since I don't see anyone working on it. Furthermore
>> what is it that we are losing in terms of functionality by not
>> supporting the comment? It seems this code was just slapt here without
>> any vision how/when to implement it?
>>
>> Furthermore, you seem to be aware of what the comment is talking about,
>> I have to admit I'm not. Is the idea that if another filesystem does
>> mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at
>> 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to
>> erroneously detect btrfs when in fact there is a different fs?
>>
>> I don't understand what problem *should* be solved here...
> 
> Without any further checks and validation, we cannot simply iterate over
> all superblocks and try to mount anything that looks ok. Even if the
> offsets exist on the block device.  The fsid should be at least checked,
> but that's still not enough to ensure the 1st copy is what we want to
> mount.

I'm more inclined to agree with Anand here, that if the users wants to
mount with -t btrfs then the filesystem should assume it's correct to
use any of the additional superblocks.  Otherwise we should explicitly
state somewhere that the superblock copies are there only for the sake
of the user-space tools, in which case this patch can go in, albeit with
some rewording.

> 
> In that case it's up to the user to deal with the problem vs automating
> all fallbacks in the kernel by default. We could enhance the recovery
> options (usebackuproot) to look at the other superblocks.
> 
> An example scenario:
> 
> - mkfs, contains 2 superblocks (primary and 1st copy)
> - shrink the filesystem, 1st copy contains stale version of the same
>   filesystem
> - mount, primary is broken, use 1st backup copy -- now, this could
>   actually succeed if all metadata pointed by the 1st copy exist and are
>   consistent and we'd end up with an older version of the filesystem
>   metadata, and partially data
> 
> 
> Another example, and this was probably meant by the comment:
> 
> - mkfs filesystem, contains primary and 1st (or more) superblock copy
> - mkfs again, now contains fewer sb copies
> - mount, similar situation as above, though the fsid will mismatch
>   (provided we're able to read the primary sb at all)
> 
> 
> The commit introducing the comment is from 2008, nobody saw the need to
> add the validation. And from the usability POV, the current behaviour
> looks better because the user/admin has a chance to analyze the problem
> first, run fsck before mounting from the sb copies and then use
> 'btrfs-select-super' to overwrite the primary sb with the backup copy.
> 
> If there are clear guarantees and enough validation for the sb copy use,
> then fine, otherwise we can apply this patch and extend the comment so
> it's documented why we don't iterate.
> 

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[David Sterba]

On Mon, Dec 04, 2017 at 06:20:09PM +0200, Nikolay Borisov wrote:
> >> I don't understand what problem *should* be solved here...
> > 
> > Without any further checks and validation, we cannot simply iterate over
> > all superblocks and try to mount anything that looks ok. Even if the
> > offsets exist on the block device.  The fsid should be at least checked,
> > but that's still not enough to ensure the 1st copy is what we want to
> > mount.
> 
> I'm more inclined to agree with Anand here, that if the users wants to
> mount with -t btrfs then the filesystem should assume it's correct to
> use any of the additional superblocks.

If and only if the additional superblocks are valid. And if we can't
read the primary superblock, we don't have enough information to
establish the validation.  EIO?  We don't have anything.  CRC mismatch?
Can't trust the whole data.  We need FSID and total_size at least. Other
actions are limited from inside kernel.

> Otherwise we should explicitly
> state somewhere that the superblock copies are there only for the sake
> of the user-space tools, in which case this patch can go in, albeit with
> some rewording.

Unless there's something I'm missing to address the concerns above, I'm
for (an updated version) of your patch.

Re: [PATCH 5/5] btrfs: Greatly simplify btrfs_read_dev_super

[Anand Jain]

On 12/07/2017 11:07 PM, Anand Jain wrote:
> 
> 
> On 12/07/2017 12:24 AM, David Sterba wrote:
>> On Mon, Dec 04, 2017 at 06:20:09PM +0200, Nikolay Borisov wrote:
>>>>> I don't understand what problem *should* be solved here...
>>>>
>>>> Without any further checks and validation, we cannot simply iterate 
>>>> over
>>>> all superblocks and try to mount anything that looks ok. Even if the
>>>> offsets exist on the block device.  The fsid should be at least 
>>>> checked,
>>>> but that's still not enough to ensure the 1st copy is what we want to
>>>> mount.
>>>
>>> I'm more inclined to agree with Anand here, that if the users wants to
>>> mount with -t btrfs then the filesystem should assume it's correct to
>>> use any of the additional superblocks.
>>
>> If and only if the additional superblocks are valid. And if we can't
>> read the primary superblock, we don't have enough information to
>> establish the validation.  EIO?  We don't have anything.  CRC mismatch?
>> Can't trust the whole data.  We need FSID and total_size at least. Other
>> actions are limited from inside kernel.

  ext4 has mount option -o sb=<copy_num> to specify the copy num to use.

  Not sure if I have dug enough but as of now I don't find any pitfall.
  Only funny thing is you can mount a device even after wipefs -a and
  recover its primary SB, to which my view is that the problem is at
  the wipefs -a end, not able to wipe all SB copies.

  I sent out an experimental RFC patch here:
    [PATCH RFC] btrfs: self heal from SB fail

  Pls try out, any problem that you could think off by this approach.

Thanks, Anand