Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Nikolay Borisov]

On  5.04.2018 20:02, David Sterba wrote:
> On Tue, Apr 03, 2018 at 01:59:47AM +0800, Liu Bo wrote:
>> 0, 1 and <0 can be returned by btrfs_next_leaf(), and when <0 is
>> returned, path->nodes[0] could be NULL, log_dir_items lacks such a
>> check for <0 and we may run into a null pointer dereference panic.
>>
>> Fixes: e02119d5a7b4 ("Btrfs: Add a write ahead tree log to optimize 
>> synchronous operations")
>> Reviewed-by: Nikolay Borisov 
>> Signed-off-by: Liu Bo 
>> ---
> 
> Added to next, thanks.
> 
>> v2: Add Fixes tag and reviewed-by.
>>
>>  fs/btrfs/tree-log.c | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index 4344577..4ee9431 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -3518,8 +3518,11 @@ static noinline int log_dir_items(struct 
>> btrfs_trans_handle *trans,
>>   * from this directory and from this transaction
>>   */
>>  ret = btrfs_next_leaf(root, path);
>> -if (ret == 1) {
>> -last_offset = (u64)-1;
>> +if (ret) {
>> +if (ret == 1)
>> +last_offset = (u64)-1;
>> +else
>> +err = ret;
> 
> I wonder if we could find some more consistent if/else pattern of the
> error handling for this function. Each caller cares about something else
> so it's hard to tell from a quick look which part is the expected one.
> 
> Something like:
> 
> if (ret < 0) {
>   unexpected error
> } else if (ret > 0 ) {
>   no more leaves, probably a terminating condition
> } else {
>   more leaves to scan, possibly this can be ommitted in most cases
> }

I agree this is better, generally when I see disparate if branches but
which pertain to the same 'ret' value I tend to rewrite them to follow
if/else if/else. This makes the code 'semantically closer' (for the lack
of a better term).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Greg KH]

On Thu, Apr 05, 2018 at 07:11:14PM +0200, David Sterba wrote:
> On Thu, Apr 05, 2018 at 04:42:34PM +, Sasha Levin wrote:
> > Hi.
> > 
> > [This is an automated email]
> > 
> > This commit has been processed by the -stable helper bot and determined
> > to be a high probability candidate for -stable trees. (score: 9.9156)
> > 
> > The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, 
> > v4.4.126, 
> > 
> > v4.15.15: Build OK!
> > v4.14.32: Build OK!
> > v4.9.92: Build OK!
> > v4.4.126: Build OK!
> > 
> > Please let us know if you'd like to have this patch included in a stable 
> > tree.
> 
> Yes, in this case we expect that the Fixes: tag will let the patch flow
> to stable after it gets applied to master.

Fixes: tags almost never cause that to happen, unless I am bored and go
digging for them.

Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly if you want a patch applied to the stable
tree.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent

[Liu Bo]

On Thu, Apr 5, 2018 at 9:48 AM, David Sterba  wrote:
> On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
>> This is running in a typical write path, not inside a critical path
>> where we have to abort the running transaction, so it's OK to return
>> errors to callers and eventually to userspace.
>
> I'm not sure this is entierly correct, several other places do not abort
> after btrfs_drop_extents as there's nothing that would leave the
> structres in some half-state.
>
>> Signed-off-by: Liu Bo 
>> ---
>>  fs/btrfs/inode.c | 5 +
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index c7b75dd..b9310f8 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root 
>> *root, struct inode *inode,
>>
>>   ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
>>   if (ret) {
>> - btrfs_abort_transaction(trans, ret);
>>   btrfs_end_transaction(trans);
>>   return ret;
>>   }
>>
>>   ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
>>   offset, 0, 0, len, 0, len, 0, 0, 0);
>
> But here the extents have been already dropped and missing to insert the
> items does not seem to lead to a consistent state.
>
> It's possible that I'm missing something. In a call path that can be
> safely rolled back even with a started transaction, we don't need to
> abort in all cases. But if the rollback requires some non-trivial
> modifications, I don't see options how to avoid the abort.
>
> __btrfs_drop_extents does a lot of state changes and can itself fail
> in the middle of dropping the range, aborting looks like the safest
> option.
>

As maybe_insert_hole is only called by btrfs_cont_expand here, which
means it's a really hole, I don't expect drop_extents would drop
anything, we can remove this drop_extents and put an assert after
btrfs_insert_file_extent for checking EEXIST.

It's different from punch hole where we need to explicitly drop an
actual extent and replace it with a hole range.

thanks,
liubo

>> - if (ret)
>> - btrfs_abort_transaction(trans, ret);
>> - else
>> + if (!ret)
>>   btrfs_update_inode(trans, root, inode);
>>   btrfs_end_transaction(trans);
>>   return ret;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: clean up resources during umount after trans is aborted

[Liu Bo]

On Thu, Apr 5, 2018 at 9:11 AM, David Sterba  wrote:
> On Sat, Mar 31, 2018 at 06:11:56AM +0800, Liu Bo wrote:
>> Currently if some fatal errors occur, like all IO get -EIO, resources
>> would be cleaned up when
>> a) transaction is being committed or
>> b) BTRFS_FS_STATE_ERROR is set
>>
>> However, in some rare cases, resources may be left alone after transaction
>> gets aborted and umount may run into some ASSERT(), e.g.
>> ASSERT(list_empty(_group->dirty_list));
>>
>> For case a), in btrfs_commit_transaciton(), there're several places at the
>> beginning where we just call btrfs_end_transaction() without cleaning up
>> resources.  For case b), it is possible that the trans handle doesn't have
>> any dirty stuff, then only trans hanlde is marked as aborted while
>> BTRFS_FS_STATE_ERROR is not set, so resources remain in memory.
>>
>> This makes btrfs also check BTRFS_FS_STATE_TRANS_ABORTED to make sure that
>> all resources won't stay in memory after umount.
>>
>> Signed-off-by: Liu Bo 
>
> Is it possible that the following stactrace could be caused by the
> missing check? It roughly matches what you describe (ie. close_ctree and
> unreleased resources). This is from generic/475, that does some error
> injection:
>
> [16991.455178] WARNING: CPU: 6 PID: 23518 at fs/btrfs/extent-tree.c:9896 
> btrfs_free_block_groups+0x2c8/0x420 [btrfs]
>

Hmm...I don't think so, while running 475, the one I got pretty stable is
ASSERT(list_empty(_group->dirty_list));

And I did see this warning a few times, but I thought that was due to
the new flag (ZERO) of fallocate for which we had fixes from Filipe,
not sure if they've been merged?

Anyway, let me double check.

thanks,
liubo

> [16991.621105]  close_ctree+0x114/0x2d0 [btrfs]
> [16991.625482]  generic_shutdown_super+0x6c/0x120
> [16991.630025]  kill_anon_super+0xe/0x20
> [16991.633820]  btrfs_kill_super+0x13/0x100 [btrfs]
> [16991.638550]  deactivate_locked_super+0x3f/0x70
> [16991.643332]  cleanup_mnt+0x3b/0x70
> [16991.646889]  task_work_run+0x89/0xa0
> [16991.650565]  exit_to_usermode_loop+0x79/0xa3
> [16991.654985]  do_syscall_64+0xe9/0x110
> [16991.658841]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Sasha Levin]

On Thu, Apr 05, 2018 at 07:11:14PM +0200, David Sterba wrote:
>On Thu, Apr 05, 2018 at 04:42:34PM +, Sasha Levin wrote:
>> Hi.
>>
>> [This is an automated email]
>>
>> This commit has been processed by the -stable helper bot and determined
>> to be a high probability candidate for -stable trees. (score: 9.9156)
>>
>> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, 
>> v4.4.126,
>>
>> v4.15.15: Build OK!
>> v4.14.32: Build OK!
>> v4.9.92: Build OK!
>> v4.4.126: Build OK!
>>
>> Please let us know if you'd like to have this patch included in a stable 
>> tree.
>
>Yes, in this case we expect that the Fixes: tag will let the patch flow
>to stable after it gets applied to master.
>
>The automated stable candidate patch scanning would be helpful in cases
>where the Fixes tag is not identified or we forget to add it. I don't
>mind helping to train the bot, so I'll try respond to the messages.

Just to clarify, having just the "Fixes:" tag is not necessarily an
indicator that a patch should go into -stable.

For example, if I fix up documentation and add a Fixes: tag to point to
the commit that added the original documentation, it's not -stable
material since we don't take documentation patches. Or, if the patch
that the new commit fixes didn't make it into any releases, it's not
stable material either.--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Drop delayed_refs argument from btrfs_check_delayed_seq

[David Sterba]

On Wed, Apr 04, 2018 at 03:57:42PM +0300, Nikolay Borisov wrote:
> It's used to print its pointer in a debug statement but doesn't really
> bring any useful information to the error message.
> 
> Signed-off-by: Nikolay Borisov 

Reviewed-by: David Sterba 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: rename btrfs_get_block_group_info and make it static

[David Sterba]

On Mon, Apr 02, 2018 at 05:24:11PM +0800, Su Yue wrote:
> The function btrfs_get_block_group_info() was introduced by the
> commit 5af3e8cce8b7 ("Btrfs: make filesystem read-only when submitting
>  barrier fails") which used it in disk-io.c.
> 
> However, the function is only called in ioctl.c now.
> Its parameter type btrfs_ioctl_space_info* is only for ioctl.
> 
> So, make it static and rename it to be original name
> get_block_group_info.
> 
> No functional change.
> 
> Signed-off-by: Su Yue 

Added to next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: bail out on error during replay_dir_deletes

[David Sterba]

On Tue, Apr 03, 2018 at 01:59:48AM +0800, Liu Bo wrote:
> If errors were returned by btrfs_next_leaf(), replay_dir_deletes needs
> to bail out, otherwise @ret would be forced to be 0 after 'break;' and
> the caller won't be aware of it.
> 
> Fixes: e02119d5a7b4 ("Btrfs: Add a write ahead tree log to optimize 
> synchronous operations")
> Reviewed-by: Nikolay Borisov 
> Signed-off-by: Liu Bo 

Added to next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[David Sterba]

On Thu, Apr 05, 2018 at 04:42:34PM +, Sasha Levin wrote:
> Hi.
> 
> [This is an automated email]
> 
> This commit has been processed by the -stable helper bot and determined
> to be a high probability candidate for -stable trees. (score: 9.9156)
> 
> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, 
> v4.4.126, 
> 
> v4.15.15: Build OK!
> v4.14.32: Build OK!
> v4.9.92: Build OK!
> v4.4.126: Build OK!
> 
> Please let us know if you'd like to have this patch included in a stable tree.

Yes, in this case we expect that the Fixes: tag will let the patch flow
to stable after it gets applied to master.

The automated stable candidate patch scanning would be helpful in cases
where the Fixes tag is not identified or we forget to add it. I don't
mind helping to train the bot, so I'll try respond to the messages.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[David Sterba]

On Tue, Apr 03, 2018 at 01:59:47AM +0800, Liu Bo wrote:
> 0, 1 and <0 can be returned by btrfs_next_leaf(), and when <0 is
> returned, path->nodes[0] could be NULL, log_dir_items lacks such a
> check for <0 and we may run into a null pointer dereference panic.
> 
> Fixes: e02119d5a7b4 ("Btrfs: Add a write ahead tree log to optimize 
> synchronous operations")
> Reviewed-by: Nikolay Borisov 
> Signed-off-by: Liu Bo 
> ---

Added to next, thanks.

> v2: Add Fixes tag and reviewed-by.
> 
>  fs/btrfs/tree-log.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 4344577..4ee9431 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3518,8 +3518,11 @@ static noinline int log_dir_items(struct 
> btrfs_trans_handle *trans,
>* from this directory and from this transaction
>*/
>   ret = btrfs_next_leaf(root, path);
> - if (ret == 1) {
> - last_offset = (u64)-1;
> + if (ret) {
> + if (ret == 1)
> + last_offset = (u64)-1;
> + else
> + err = ret;

I wonder if we could find some more consistent if/else pattern of the
error handling for this function. Each caller cares about something else
so it's hard to tell from a quick look which part is the expected one.

Something like:

if (ret < 0) {
unexpected error
} else if (ret > 0 ) {
no more leaves, probably a terminating condition
} else {
more leaves to scan, possibly this can be ommitted in most cases
}
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Replace owner argument in add_pinned_bytes with a boolean

[David Sterba]

On Fri, Mar 30, 2018 at 12:58:47PM +0300, Nikolay Borisov wrote:
> add_pinned_bytes reallyc ares whether the bytes being pinned are either
> data or metadata. To that effect is checks whether the 'owner' argument
> is less than BTRFS_FIRST_FREE_OBJECTID (256). This works because
> owner can really have 2 types of values:
>  a) For metadata extents it holds the level at which the parent is in
>  the btree. This amounts to owner having the values 0-7
> 
>  b) In case of modifying data extentsi, owner is the inode number
>  to which those extents belongs.
> 
> Let's make this more explicit byt converting the owner parameter to a
> boolean value and either pass it directly when we know the type of
> extents we are working with (i.e. in btrfs_free_tree_block). In cases
> when the parent function can be called on both metadata/data extents
> perform the check in the caller. This hopefully makes the interface
> of add_pinned_bytes more intuitive.

Agreed, looks better with the bool.
> 
> Signed-off-by: Nikolay Borisov 

Reviewed-by: David Sterba 

> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -756,12 +756,12 @@ static struct btrfs_space_info 
> *__find_space_info(struct btrfs_fs_info *info,
>  }
>  
>  static void add_pinned_bytes(struct btrfs_fs_info *fs_info, s64 num_bytes,
> -  u64 owner, u64 root_objectid)
> +  bool metadata, u64 root_objectid)
>  {
>   struct btrfs_space_info *space_info;
>   u64 flags;
>  
> - if (owner < BTRFS_FIRST_FREE_OBJECTID) {
> + if (metadata) {
>   if (root_objectid == BTRFS_CHUNK_TREE_OBJECTID)
>   flags = BTRFS_BLOCK_GROUP_SYSTEM;
>   else
> @@ -2212,8 +2212,10 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle 
> *trans,
>_ref_mod, _ref_mod);
>   }
>  
> - if (ret == 0 && old_ref_mod < 0 && new_ref_mod >= 0)
> - add_pinned_bytes(fs_info, -num_bytes, owner, root_objectid);
> + if (ret == 0 && old_ref_mod < 0 && new_ref_mod >= 0) {
> + bool metadata = owner < BTRFS_FIRST_FREE_OBJECTID;

Missing newline.

> + add_pinned_bytes(fs_info, -num_bytes, metadata, root_objectid);
> + }
>  
>   return ret;
>  }
> @@ -7231,7 +7233,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle 
> *trans,
>   }
>  out:
>   if (pin)
> - add_pinned_bytes(fs_info, buf->len, btrfs_header_level(buf),
> + add_pinned_bytes(fs_info, buf->len, true,
>root->root_key.objectid);
>  
>   if (last_ref) {
> @@ -7285,8 +7287,10 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
>_ref_mod, _ref_mod);
>   }
>  
> - if (ret == 0 && old_ref_mod >= 0 && new_ref_mod < 0)
> - add_pinned_bytes(fs_info, num_bytes, owner, root_objectid);
> + if (ret == 0 && old_ref_mod >= 0 && new_ref_mod < 0) {
> + bool metadata = owner < BTRFS_FIRST_FREE_OBJECTID;

Missing newline.

> + add_pinned_bytes(fs_info, num_bytes, metadata, root_objectid);
> + }
>  
>   return ret;
>  }

And a few typos in the changelog but I'll fix that, no need to resend.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent

[David Sterba]

On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
> This is running in a typical write path, not inside a critical path
> where we have to abort the running transaction, so it's OK to return
> errors to callers and eventually to userspace.

I'm not sure this is entierly correct, several other places do not abort
after btrfs_drop_extents as there's nothing that would leave the
structres in some half-state.

> Signed-off-by: Liu Bo 
> ---
>  fs/btrfs/inode.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c7b75dd..b9310f8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, 
> struct inode *inode,
>  
>   ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
>   if (ret) {
> - btrfs_abort_transaction(trans, ret);
>   btrfs_end_transaction(trans);
>   return ret;
>   }
>  
>   ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
>   offset, 0, 0, len, 0, len, 0, 0, 0);

But here the extents have been already dropped and missing to insert the
items does not seem to lead to a consistent state.

It's possible that I'm missing something. In a call path that can be
safely rolled back even with a started transaction, we don't need to
abort in all cases. But if the rollback requires some non-trivial
modifications, I don't see options how to avoid the abort.

__btrfs_drop_extents does a lot of state changes and can itself fail
in the middle of dropping the range, aborting looks like the safest
option.

> - if (ret)
> - btrfs_abort_transaction(trans, ret);
> - else
> + if (!ret)
>   btrfs_update_inode(trans, root, inode);
>   btrfs_end_transaction(trans);
>   return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Sasha Levin]

Hi.

[This is an automated email]

This commit has been processed by the -stable helper bot and determined
to be a high probability candidate for -stable trees. (score: 9.9156)

The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 

v4.15.15: Build OK!
v4.14.32: Build OK!
v4.9.92: Build OK!
v4.4.126: Build OK!

Please let us know if you'd like to have this patch included in a stable tree.

--
Thanks.
Sasha--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: clean up resources during umount after trans is aborted

[David Sterba]

On Sat, Mar 31, 2018 at 06:11:56AM +0800, Liu Bo wrote:
> Currently if some fatal errors occur, like all IO get -EIO, resources
> would be cleaned up when
> a) transaction is being committed or
> b) BTRFS_FS_STATE_ERROR is set
> 
> However, in some rare cases, resources may be left alone after transaction
> gets aborted and umount may run into some ASSERT(), e.g.
> ASSERT(list_empty(_group->dirty_list));
> 
> For case a), in btrfs_commit_transaciton(), there're several places at the
> beginning where we just call btrfs_end_transaction() without cleaning up
> resources.  For case b), it is possible that the trans handle doesn't have
> any dirty stuff, then only trans hanlde is marked as aborted while
> BTRFS_FS_STATE_ERROR is not set, so resources remain in memory.
> 
> This makes btrfs also check BTRFS_FS_STATE_TRANS_ABORTED to make sure that
> all resources won't stay in memory after umount.
> 
> Signed-off-by: Liu Bo 

Is it possible that the following stactrace could be caused by the
missing check? It roughly matches what you describe (ie. close_ctree and
unreleased resources). This is from generic/475, that does some error
injection:

[16991.455178] WARNING: CPU: 6 PID: 23518 at fs/btrfs/extent-tree.c:9896 
btrfs_free_block_groups+0x2c8/0x420 [btrfs]

[16991.621105]  close_ctree+0x114/0x2d0 [btrfs]
[16991.625482]  generic_shutdown_super+0x6c/0x120
[16991.630025]  kill_anon_super+0xe/0x20
[16991.633820]  btrfs_kill_super+0x13/0x100 [btrfs]
[16991.638550]  deactivate_locked_super+0x3f/0x70
[16991.643332]  cleanup_mnt+0x3b/0x70
[16991.646889]  task_work_run+0x89/0xa0
[16991.650565]  exit_to_usermode_loop+0x79/0xa3
[16991.654985]  do_syscall_64+0xe9/0x110
[16991.658841]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Fix possible softlock on single core machines

[David Sterba]

On Thu, Apr 05, 2018 at 06:04:12PM +0300, Nikolay Borisov wrote:
> 
> 
> On  5.04.2018 16:55, David Sterba wrote:
> > On Thu, Apr 05, 2018 at 10:40:15AM +0300, Nikolay Borisov wrote:
> >> do_chunk_alloc implements a loop checking whether there is a pending
> >> chunk allocation and if so causes the caller do loop. Generally this
> >> loop is executed only once, however testing with btrfs/072 on a
> >> single core vm machines uncovered an extreme case where the system
> >> could loop indefinitely. This is due to a missing cond_resched when
> >> loop which doesn't give a chance to the previous chunk allocator finish
> >> its job.
> >>
> >> The fix is to simply add the missing cond_resched.
> >>
> >> Fixes: 6d74119f1a3e ("Btrfs: avoid taking the chunk_mutex in 
> >> do_chunk_alloc")
> > 
> > Does this commit really lead to the endless loop on UP? I don't see any
> > obvious connection.
> 
> This is the commit that introduced the loop there without adding
> cond_resched, hence the fixes tag.

Makes sense, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Fix possible softlock on single core machines

[Nikolay Borisov]

On  5.04.2018 16:55, David Sterba wrote:
> On Thu, Apr 05, 2018 at 10:40:15AM +0300, Nikolay Borisov wrote:
>> do_chunk_alloc implements a loop checking whether there is a pending
>> chunk allocation and if so causes the caller do loop. Generally this
>> loop is executed only once, however testing with btrfs/072 on a
>> single core vm machines uncovered an extreme case where the system
>> could loop indefinitely. This is due to a missing cond_resched when
>> loop which doesn't give a chance to the previous chunk allocator finish
>> its job.
>>
>> The fix is to simply add the missing cond_resched.
>>
>> Fixes: 6d74119f1a3e ("Btrfs: avoid taking the chunk_mutex in do_chunk_alloc")
> 
> Does this commit really lead to the endless loop on UP? I don't see any
> obvious connection.

This is the commit that introduced the loop there without adding
cond_resched, hence the fixes tag.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] btrfs: return required error from btrfs_check_super_csum

[David Sterba]

On Wed, Mar 28, 2018 at 04:43:02AM +0800, Anand Jain wrote:
>   I understand reason behind string should not be split, but I
>   thought here will be an exception, as if you want to search
>   for the string you will still use "unsupported checksum algorithm"
>   which is still in one line.
>   About the grep not able to find the string which are split,
>   I look at it as fixing the wrong end of the problem. That is Grep
>   end problem being fixes at the c code end. Anyway as its kind of
>   linux general guidlines, I shall fix my patch.

It's not just grep, but any other similar line-oriented searching tool
or internal searches in editors or IDEs. Searching for fragments of the
string will likely lead to the result but splitting lines makes it
unnecessarily more difficult.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/16 v1] Kill fs_info::volume_mutex

[David Sterba]

On Tue, Apr 03, 2018 at 08:34:01PM +0200, David Sterba wrote:
> This series gets rid of the volume mutex. The fstests do not pass
> cleanly, 2 or more tests fail so this needs to be fixed, but otherwise
> majority of the work ready for review.
> 
> The merge target is 4.18 and I'll probably not get back to this pathset
> during merge window (nor add it to next), so there should be enough time
> for review.
> 
> David Sterba (16):
>   btrfs: squeeze btrfs_dev_replace_continue_on_mount to its caller
>   btrfs: make success path out of btrfs_init_dev_replace_tgtdev more
> clear
>   btrfs: export and rename free_device
>   btrfs: move btrfs_init_dev_replace_tgtdev to dev-replace.c and make
> static
>   btrfs: move volume_mutex to callers of btrfs_rm_device
>   btrfs: move clearing of EXCL_OP out of __cancel_balance
>   btrfs: add proper safety check before resuming dev-replace
>   btrfs: add sanity check when resuming balance after mount
>   btrfs: cleanup helpers that reset balance state
>   btrfs: remove wrong use of volume_mutex from btrfs_dev_replace_start
>   btrfs: kill btrfs_fs_info::volume_mutex
>   btrfs: track running balance in a simpler way
>   btrfs: remove redundant read-only check from btrfs_cancel_balance
>   btrfs: drop lock parameter from update_ioctl_balance_args and rename
>   btrfs: use mutex in btrfs_resume_balance_async
>   btrfs: open code set_balance_control

Branch updated in

git://github.com/kdave/btrfs-devel dev/remove-volume-mutex

the misapplied patch hunks have been sorted out and now it also compiles.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/16] btrfs: move volume_mutex to callers of btrfs_rm_device

[David Sterba]

On Thu, Apr 05, 2018 at 05:41:57PM +0800, Anand Jain wrote:
> 
> > @@ -2716,6 +2718,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, 
> > void __user *arg)
> > ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> > goto out_drop_write;
> > }
> > +   mutex_lock(_info->volume_mutex);
> >   
> > vol_args = memdup_user(arg, sizeof(*vol_args));
> > if (IS_ERR(vol_args)) { > @@ -2730,6 +2733,7 @@ static long 
> > btrfs_ioctl_rm_dev(struct file 
> *file, void __user *arg)
> > btrfs_info(fs_info, "disk deleted %s", vol_args->name);
> > kfree(vol_args);
> >   out:
> > +   mutex_unlock(_info->volume_mutex);
> > clear_bit(BTRFS_FS_EXCL_OP, _info->flags);
> >   out_drop_write:
> > mnt_drop_write_file(file);
> 
>   Why not memdup_user() be outside of volume_mutex?

The point of the patch is to put the mutex_lock right next to the
exclusive operation bit setting. It's not optimal regarding the size of
critical section and normally the memdup should be outside.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/16] btrfs: move clearing of EXCL_OP out of __cancel_balance

[David Sterba]

On Thu, Apr 05, 2018 at 05:42:14PM +0800, Anand Jain wrote:
> 
> 
> > @@ -3926,11 +3925,11 @@ int btrfs_balance(struct btrfs_balance_control 
> > *bctl,
> > return ret;
> >   out:
> > if (bctl->flags & BTRFS_BALANCE_RESUME)
> > -   __cancel_balance(fs_info);
> > -   else {
> > +   reset_balance_state(fs_info);
> 
>   reset_balance_state() is something unrelated. More over compile fails 
> at this patch.
> 
> 
> fs/btrfs/volumes.c: In function ‘btrfs_balance’:
> fs/btrfs/volumes.c:3928:3: error: implicit declaration of function 
> ‘reset_balance_state’; did you mean ‘insert_balance_item’? 
> [-Werror=implicit-function-declaration]
> reset_balance_state(fs_info);

Right, it looks like a misapplied fixup for patch "btrfs: cleanup
helpers that reset balance state" that's farther in the series. Will
fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Fix possible softlock on single core machines

[David Sterba]

On Thu, Apr 05, 2018 at 10:40:15AM +0300, Nikolay Borisov wrote:
> do_chunk_alloc implements a loop checking whether there is a pending
> chunk allocation and if so causes the caller do loop. Generally this
> loop is executed only once, however testing with btrfs/072 on a
> single core vm machines uncovered an extreme case where the system
> could loop indefinitely. This is due to a missing cond_resched when
> loop which doesn't give a chance to the previous chunk allocator finish
> its job.
> 
> The fix is to simply add the missing cond_resched.
> 
> Fixes: 6d74119f1a3e ("Btrfs: avoid taking the chunk_mutex in do_chunk_alloc")

Does this commit really lead to the endless loop on UP? I don't see any
obvious connection.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Couldn't read tree root BTRFS - SEED

[Qu Wenruo]

On 2018年04月05日 19:27, Senén Vidal Blanco wrote:
> Hello,
> I have a problem when mounting the btrfs system with two units (one in SEED 
> mode and the other in read-write mode)
>  
> Initially it is functioning correctly in production:
>  
> btrfs fi sh 
> Label: 'SEED_MD2'  uuid: 285b16fa-b297-43ed-bb3b-311950729eb6 
>Total devices 2 FS bytes used 2.00TiB 
>devid1 size 5.44TiB used 1.97TiB path /dev/bcache1 
>devid2 size 1.80TiB used 119.03GiB path /dev/bcache0 
>  
> Label: 'BOOT'  uuid: ce8fd2ef-975c-417a-b90c-280f8d324c44 
>Total devices 1 FS bytes used 536.86MiB 
>devid1 size 1.86GiB used 1.15GiB path /dev/md1 
>  
> Label: 'SEED_MD2'  uuid: 851e4474-d375-4a25-b202-949e51f05877 
>Total devices 1 FS bytes used 1.94TiB 
>devid1 size 5.44TiB used 1.97TiB path /dev/bcache1
>  
> The bcache0 and bcache1 units are what I mention.
> bcache1 is the SEED
> bcache0 is the read-write
>  
> When it comes to mounting the two copy mirror devices that is in production I 
> have this result:
>  
> parent transid verify failed on 2184045428736 wanted 269593 found 266275 
> parent transid verify failed on 2184045428736 wanted 269593 found 266275

It's not the problem that btrfs fails to assemble correct seed/rw
devices list, it's something wrong (metadata corruption) happened.



> Ignoring transid failure

So this is btrfs-progs (btrfs check). Kernel doesn't ignore any transid
failure AFAIK.

> Couldn't map the block 2287467560960 
> No mapping for 2287467560960-2287467577344 
> Couldn't map the block 2287467560960 
> bytenr mismatch, want=2287467560960, have=0 
> Couldn't read tree root 
> Label: 'SEED_MD2'  uuid: 851e4474-d375-4a25-b202-949e51f05877 
>Total devices 1 FS bytes used 1.94TiB 
>devid1 size 5.44TiB used 1.97TiB path /dev/bcache1 
>  
> Label: 'SEED_MD2'  uuid: 285b16fa-b297-43ed-bb3b-311950729eb6 
>Total devices 2 FS bytes used 1.98TiB 
>devid2 size 1.80TiB used 102.03GiB path /dev/bcache2 
>*** Some devices missing
>  
> Would there be any way to tell BTRFS that the missing unit is the one that 
> corresponds to the SEED unit so that it correctly rebuilds the file system?

The rw device is corrupted.

To ensure your seed device is safe, please run "btrfs check " to see if there is any problem.

From current output, it seems that your chunk tree is corrupted, so that
btrfs check can't even map the logical address for your tree root.

And to further debug the corrupted fs, please attach the following data:

1) btrfs inspect dump-super -fa 
2) btrfs inspect dump-tree -t chunk 

Thanks,
Qu

> 
> Thank you.
> 



signature.asc
Description: OpenPGP digital signature

Couldn't read tree root BTRFS - SEED

[Senén Vidal Blanco]

Hello,
I have a problem when mounting the btrfs system with two units (one in SEED 
mode and the other in read-write mode)
 
Initially it is functioning correctly in production:
 
btrfs fi sh 
Label: 'SEED_MD2'  uuid: 285b16fa-b297-43ed-bb3b-311950729eb6 
   Total devices 2 FS bytes used 2.00TiB 
   devid1 size 5.44TiB used 1.97TiB path /dev/bcache1 
   devid2 size 1.80TiB used 119.03GiB path /dev/bcache0 
 
Label: 'BOOT'  uuid: ce8fd2ef-975c-417a-b90c-280f8d324c44 
   Total devices 1 FS bytes used 536.86MiB 
   devid1 size 1.86GiB used 1.15GiB path /dev/md1 
 
Label: 'SEED_MD2'  uuid: 851e4474-d375-4a25-b202-949e51f05877 
   Total devices 1 FS bytes used 1.94TiB 
   devid1 size 5.44TiB used 1.97TiB path /dev/bcache1
 
The bcache0 and bcache1 units are what I mention.
bcache1 is the SEED
bcache0 is the read-write
 
When it comes to mounting the two copy mirror devices that is in production I 
have this result:
 
parent transid verify failed on 2184045428736 wanted 269593 found 266275 
parent transid verify failed on 2184045428736 wanted 269593 found 266275 
Ignoring transid failure 
Couldn't map the block 2287467560960 
No mapping for 2287467560960-2287467577344 
Couldn't map the block 2287467560960 
bytenr mismatch, want=2287467560960, have=0 
Couldn't read tree root 
Label: 'SEED_MD2'  uuid: 851e4474-d375-4a25-b202-949e51f05877 
   Total devices 1 FS bytes used 1.94TiB 
   devid1 size 5.44TiB used 1.97TiB path /dev/bcache1 
 
Label: 'SEED_MD2'  uuid: 285b16fa-b297-43ed-bb3b-311950729eb6 
   Total devices 2 FS bytes used 1.98TiB 
   devid2 size 1.80TiB used 102.03GiB path /dev/bcache2 
   *** Some devices missing
 
Would there be any way to tell BTRFS that the missing unit is the one that 
corresponds to the SEED unit so that it correctly rebuilds the file system?

Thank you.



signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 06/16] btrfs: move clearing of EXCL_OP out of __cancel_balance

[Anand Jain]

@@ -3926,11 +3925,11 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
return ret;
  out:
if (bctl->flags & BTRFS_BALANCE_RESUME)
-   __cancel_balance(fs_info);
-   else {
+   reset_balance_state(fs_info);


 reset_balance_state() is something unrelated. More over compile fails 
at this patch.



fs/btrfs/volumes.c: In function ‘btrfs_balance’:
fs/btrfs/volumes.c:3928:3: error: implicit declaration of function 
‘reset_balance_state’; did you mean ‘insert_balance_item’? 
[-Werror=implicit-function-declaration]

   reset_balance_state(fs_info);


Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/16] btrfs: move volume_mutex to callers of btrfs_rm_device

[Anand Jain]

@@ -2716,6 +2718,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void 
__user *arg)
ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
goto out_drop_write;
}
+   mutex_lock(_info->volume_mutex);
  
  	vol_args = memdup_user(arg, sizeof(*vol_args));
  	if (IS_ERR(vol_args)) { > @@ -2730,6 +2733,7 @@ static long btrfs_ioctl_rm_dev(struct file 

*file, void __user *arg)

btrfs_info(fs_info, "disk deleted %s", vol_args->name);
kfree(vol_args);
  out:
+   mutex_unlock(_info->volume_mutex);
clear_bit(BTRFS_FS_EXCL_OP, _info->flags);
  out_drop_write:
mnt_drop_write_file(file);


 Why not memdup_user() be outside of volume_mutex?
 But not a big deal either.


Reviewed-by: Anand Jain 

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/16] btrfs: move btrfs_init_dev_replace_tgtdev to dev-replace.c and make static

[Anand Jain]

On 04/04/2018 02:34 AM, David Sterba wrote:

The function logically belongs there and there's only a single caller,
no need to export it. No code changes.

Signed-off-by: David Sterba 



Reviewed-by: Anand Jain 

Thanks, Anand

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

Re: [PATCH 03/16] btrfs: export and rename free_device

[Anand Jain]

On 04/04/2018 02:34 AM, David Sterba wrote:

The function will be used outside of volumes.c, the allocation
btrfs_alloc_device is also exported.

Signed-off-by: David Sterba 


Reviewed-by: Anand Jain 

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/16] btrfs: make success path out of btrfs_init_dev_replace_tgtdev more clear

[Anand Jain]

On 04/04/2018 02:34 AM, David Sterba wrote:

This is a preparatory cleanup that will make clear that the only
successful way out of btrfs_init_dev_replace_tgtdev will also set the
device_out to a valid pointer. With this guarantee, the callers can be
simplified.

Signed-off-by: David Sterba 


Reviewed-by: Anand Jain 

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/16] btrfs: squeeze btrfs_dev_replace_continue_on_mount to its caller

[Anand Jain]

On 04/04/2018 02:34 AM, David Sterba wrote:

The function is called once and is fairly small, we can merge it with
the caller.

Signed-off-by: David Sterba 


Reviewed-by: Anand Jain 

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs-progs: build: Do not use cp -a to install files

[Anand Jain]

On 04/04/2018 10:04 PM, Peter Kjellerstedt wrote:

Using cp -a to install files will preserve the ownership of the
original files (if possible), which is typically not wanted. E.g., if
the files were built by a normal user, but are being installed by
root, then the installed files would maintain the UIDs/GIDs of the
user that built the files rather than be owned by root.


 Reviewed-by: Anand Jain 

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] fstests: btrfs/159 superblock corruption test case

[Anand Jain]

Verify if the superblock corruption is handled correctly.

Signed-off-by: Anand Jain 
---
v1->v2:
 $subject slightly changed
 Added more info about the test-case
 Keep the stuff from the ./new btrfs
 Add mount_opt_minus_args() to get the options (if) set at the config file
 Move DEV_GOOD & DEV_BAD to where it starts to use
 To help debugging added run_check where possible
 Remove {} in the out file
 Use _filter_error_mount for mount fail cases other than -EINVAL

 tests/btrfs/159 | 177 
 tests/btrfs/159.out |  23 +++
 tests/btrfs/group   |   1 +
 3 files changed, 201 insertions(+)
 create mode 100755 tests/btrfs/159
 create mode 100644 tests/btrfs/159.out

diff --git a/tests/btrfs/159 b/tests/btrfs/159
new file mode 100755
index ..521cfdab0242
--- /dev/null
+++ b/tests/btrfs/159
@@ -0,0 +1,177 @@
+#! /bin/bash
+# FS QA Test 159
+#
+# Test if the superblock corruption is handled correctly:
+#  - Test fsid miss-match (csum ok) between primary and copy superblock
+#  Fixed by the ML patch:
+#  btrfs: check if the fsid in the primary sb and copy sb are same
+#  - Test if the mount fails if the primary superblock csum is
+#  corrupted on any disk
+#  - Test if the mount does not fail if the copy1 sb csum is corrupted
+#  Fixed by the ML patches:
+#  btrfs: verify superblock checksum during scan
+#  btrfs: verify checksum for all devices in mount context
+#
+#-
+# Copyright (c) 2018 Oracle.  All Rights Reserved.
+# Author: Anand Jain 
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#---
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1   # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+   cd /
+   rm -f $tmp.*
+   _scratch_dev_pool_put
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/module
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+_supported_fs btrfs
+_supported_os Linux
+_require_scratch_dev_pool 2
+_require_loadable_fs_module "btrfs"
+_require_command "$WIPEFS_PROG" wipefs
+
+_scratch_dev_pool_get 2
+
+mount_opt_minus_args()
+{
+   local nr
+   local mnt_opt=""
+
+   nr=`_scratch_mount_options | awk '{print NF}'`
+   if [ $nr -eq 4 ]; then
+   #gets only the mount option set in the config file
+   mnt_opt=`_scratch_mount_options | awk '{print $2}'`
+   fi
+   #Append the additional opts provide as func args.
+   #Make sure -o is not echo-ed if both config file mnt-option
+   #and the test case mnt-option are null.
+   if [ -z $mnt_opt ]; then
+   if [ ! -z $* ]; then
+   echo "-o $*"
+   fi
+   else
+   if [ -z $* ]; then
+   echo "-o $mnt_opt"
+   else
+   echo "-o $mnt_opt,$*"
+   fi
+   fi
+}
+
+wipe()
+{
+   $WIPEFS_PROG -a $DEV_GOOD > /dev/null 2>&1
+   $WIPEFS_PROG -a $DEV_BAD > /dev/null 2>&1
+}
+
+# Test for fsid miss-match (csum ok) with primary and copy superblock.
+check_copy1_fsid()
+{
+   local bytenr=67108864
+   echo -e "\\ncheck_copy1_fsid\\n" | tee -a $seqres.full
+
+   wipe
+   $MKFS_BTRFS_PROG -fq $DEV_GOOD
+   $MKFS_BTRFS_PROG -fq $DEV_BAD
+   _reload_fs_module "btrfs"
+
+   run_check dd status=none of=$DEV_BAD if=$DEV_GOOD ibs=1 obs=1\
+   skip=$bytenr seek=$bytenr count=4K
+
+   #must fail
+   _mount `mount_opt_minus_args` $DEV_BAD $SCRATCH_MNT 2>&1 | 
_filter_scratch_pool
+   #If mount didn't fail
+   _scratch_unmount > /dev/null 2>&1
+}
+
+# Test if the mount fails if the primary superblock csum is corrupted.
+check_primary()
+{
+   local bytenr=65536
+   echo -e "\\ncheck_primary\\n" | tee -a $seqres.full
+
+   wipe
+   _scratch_pool_mkfs "-mraid1 -draid1"
+   _reload_fs_module "btrfs"
+
+   #corrupt bytenr DEV_BAD
+   od -j$bytenr -N1 -An -x $DEV_BAD |\
+   dd status=none of=$DEV_BAD 

Re: [PATCH typo-fixed] fstests: btrfs: 159 superblock corruption test case

[Anand Jain]

+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+status=1   # failure is the default!


Please use './new btrfs' to generate new test template, which defines
variables like 'tmp' and 'here', these variables may not be used in the
test explicitly, but they could be used by some helper functions,
especially the "$tmp" var. (Or please don't remove them from template :)


 Ok. May be its a good idea to have an _init() to set mandatory stuffs?


+MNT_OPT=$(echo $MOUNT_OPTIONS | cut -d" " -f2-)



I'm not sure about the purpose of this MNT_OPT, and it could be empty
and causes problems in test. Please see below.


 Test case here need control of the device(s) used for the mount.
 Thats awkward I missed no mount-option secnario. Will fix.


+   echo -e "\\ncheck_copy1_fsid\\n{" | tee -a $seqres.full


Hmm, I don't think the "{ }" block is necessary in .out file, IMHO it
makes the diff context harder to read when test fails. Just echo the
test name would be fine.


 Ok. Will take that out.



+   _mount -o $MNT_OPT $DEV_BAD $SCRATCH_MNT 2>&1 | _filter_scratch_pool


When $MOUNT_OPTIONS is empty, this mount fails as

+mount: can't find /mnt/scratch in /etc/fstab

And I think we need to umount $SCRATCH_MNT here in case a buggy btrfs
module allowed the mount, as the next test assumes the device is not
mounted anywhere. Otherwise I see failures like:

+ERROR: /dev/loop1 is mounted
+mount failed


 got it.


+
+   echo -e "good\\n}" | tee -a $seqres.full
+}
+
+# As in Linux kernel 4.16 if the primary is corrupted mount will fail.
+# Which might change in the long run.
+check_primary()
+{
+   local bytenr=65536
+   echo -e "\\ncheck_primary\\n{" | tee -a $seqres.full
+
+   wipe
+   _scratch_pool_mkfs "-mraid1 -draid1"
+   _reload_fs_module "btrfs"
+
+   #To corrupt a disk block, read in hex, write in dec
+   od -j$bytenr -N1 -An -x $DEV_BAD |\
+   dd status=none of=$DEV_BAD ibs=1 obs=1 seek=$bytenr count=1|\
+   tee -a $seqres.full
+   _mount -o $MNT_OPT,device=$DEV_GOOD $DEV_BAD $SCRATCH_MNT 2>&1 | 
_filter_scratch_pool


Same here, need to umount $SCRATCH_MNT.

Also, if a mount failure is expected (as it's recorded in the golden
image file), you need _filter_error_mount here, and _filter_scratch_pool
can be dropped, because _filter_error_mount will remove all
$SCRATCH_DEV/MNT from the mount output. For more details please see
commit 94d3a4f00cbd ("fstests: filter mount error message for EUCLEAN
and ESTALE").


 I am fixing this in v2.

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html