re: Btrfs: don't start the log transaction if the log tree init fails

re: Btrfs: don't start the log transaction if the log tree init fails

[Dan Carpenter]

Hello Miao Xie,

The patch e87ac1368700: "Btrfs: don't start the log transaction if
the log tree init fails" from Feb 20, 2014, leads to the following
static checker warning:

	fs/btrfs/tree-log.c:178 start_log_trans()
	warn: we tested 'root->log_root' before and it was 'false'

fs/btrfs/tree-log.c
   147          if (root->log_root) {

We test "root->log_root" here.

   148                  if (btrfs_need_log_full_commit(root->fs_info, trans)) {
   149                          ret = -EAGAIN;
   150                          goto out;
   151                  }
   152                  if (!root->log_start_pid) {
   153                          root->log_start_pid = current->pid;
   154                          clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
   155                  } else if (root->log_start_pid != current->pid) {
   156                          set_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
   157                  }
   158  
   159                  atomic_inc(&root->log_batch);
   160                  atomic_inc(&root->log_writers);
   161                  if (ctx) {
   162                          index = root->log_transid % 2;
   163                          list_add_tail(&ctx->list, &root->log_ctxs[index]);
   164                          ctx->log_transid = root->log_transid;
   165                  }
   166                  mutex_unlock(&root->log_mutex);
   167                  return 0;
   168          }
   169  
   170          ret = 0;
   171          mutex_lock(&root->fs_info->tree_log_mutex);
   172          if (!root->fs_info->log_root_tree)
   173                  ret = btrfs_init_log_root_tree(trans, root->fs_info);
   174          mutex_unlock(&root->fs_info->tree_log_mutex);
   175          if (ret)
   176                  goto out;
   177  
   178          if (!root->log_root) {

Couldn't we just remove this condition here?  This is a new Smatch thing
I am working on and I am investigating false positives.

   179                  ret = btrfs_add_log_tree(trans, root);
   180                  if (ret)
   181                          goto out;
   182          }

regards,
dan carpenter

RE: Btrfs: don't start the log transaction if the log tree init fails

[Zhao Lei]

Hi, Dan Carpenter

Thanks for reporting this problem.
Keeping code in good logic can reduce bug and make it easy to maintenance.

> -----Original Message-----
> From: linux-btrfs-owner@vger.kernel.org
> [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Dan Carpenter
> Sent: Friday, August 14, 2015 5:28 PM
> To: miaox@cn.fujitsu.com
> Cc: linux-btrfs@vger.kernel.org
> Subject: re: Btrfs: don't start the log transaction if the log tree init fails
> 
> Hello Miao Xie,
> 
> The patch e87ac1368700: "Btrfs: don't start the log transaction if the log tree
> init fails" from Feb 20, 2014, leads to the following static checker warning:
> 
> 	fs/btrfs/tree-log.c:178 start_log_trans()
> 	warn: we tested 'root->log_root' before and it was 'false'
> 
> fs/btrfs/tree-log.c
>    147          if (root->log_root) {
> 
> We test "root->log_root" here.
> 
>    148                  if (btrfs_need_log_full_commit(root->fs_info,
> trans)) {
>    149                          ret = -EAGAIN;
>    150                          goto out;
>    151                  }
>    152                  if (!root->log_start_pid) {
>    153                          root->log_start_pid = current->pid;
>    154
> clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
>    155                  } else if (root->log_start_pid != current->pid) {
>    156
> set_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
>    157                  }
>    158
>    159                  atomic_inc(&root->log_batch);
>    160                  atomic_inc(&root->log_writers);
>    161                  if (ctx) {
>    162                          index = root->log_transid % 2;
>    163                          list_add_tail(&ctx->list,
> &root->log_ctxs[index]);
>    164                          ctx->log_transid = root->log_transid;
>    165                  }
>    166                  mutex_unlock(&root->log_mutex);
>    167                  return 0;
>    168          }
>    169
>    170          ret = 0;
>    171          mutex_lock(&root->fs_info->tree_log_mutex);
>    172          if (!root->fs_info->log_root_tree)
>    173                  ret = btrfs_init_log_root_tree(trans,
> root->fs_info);
>    174          mutex_unlock(&root->fs_info->tree_log_mutex);
>    175          if (ret)
>    176                  goto out;
>    177
>    178          if (!root->log_root) {
> 
> Couldn't we just remove this condition here?  This is a new Smatch thing I am
> working on and I am investigating false positives.
> 
Above problem was introduced by commit: 7237f1833,
and this patch(e87ac1368700) make the problem easy to find out.

We can remove above condition to make code pass smatch checking,
but a better way is to do some cleanup to remove duplicated code
in above function.

I'll fix it.

Thanks
Zhaolei

>    179                  ret = btrfs_add_log_tree(trans, root);
>    180                  if (ret)
>    181                          goto out;
>    182          }
> 
> regards,
> dan carpenter
> --
> 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