[PATCH 0/3] btrfs: fsync changes, a bug fix and a couple improvements

[PATCH 0/3] btrfs: fsync changes, a bug fix and a couple improvements

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The first patch in the series fixes a bug where a directory fsync, following
by inode eviction, followed by renaming a file and syncing the log results
in losing a file if a power failure happens and the log is replayed.

The remaining two changes are independent, and are about avoiding unnecessary
work during operations that need to check or modify the log (renames, adding
a hard link, unlinks) and making renames hold log commits for a shorter
period.

Filipe Manana (3):
  btrfs: fix lost inode on log replay after mix of fsync, rename and inode eviction
  btrfs: eliminate some false positives when checking if inode was logged
  btrfs: do not pin logs too early during renames

 fs/btrfs/inode.c    | 48 +++++++++++++++++++++++++++++++++++++++------
 fs/btrfs/tree-log.c | 29 ++++++++++++++++-----------
 2 files changed, 60 insertions(+), 17 deletions(-)

-- 
2.28.0

[PATCH 1/3] btrfs: fix lost inode on log replay after mix of fsync, rename and inode eviction

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When checking if we need to log the new name of a renamed inode, we are
checking if the inode and its parent inode have been logged before, and if
not we don't log the new name. The check however is buggy, as it directly
compares the logged_trans field of the inodes versus the ID of the current
transaction. The problem is that logged_trans is a transient field, only
stored in memory and never persisted in the inode item, so if an inode
was logged before, evicted and reloaded, its logged_trans field is set to
a value of 0, meaning the check will return false and the new name of the
renamed inode is not logged. If the old parent directory was previously
fsynced and we deleted the logged directory entries corresponding to the
old name, we end up with a log that when replayed will delete the renamed
inode.

The following example triggers the problem:

  $ mkfs.btrfs -f /dev/sdc
  $ mount /dev/sdc /mnt

  $ mkdir /mnt/A
  $ mkdir /mnt/B
  $ echo -n "hello world" > /mnt/A/foo

  $ sync

  # Add some new file to A and fsync directory A.
  $ touch /mnt/A/bar
  $ xfs_io -c "fsync" /mnt/A

  # Now trigger inode eviction. We are only interested in triggering
  # eviction for the inode of directory A.
  $ echo 2 > /proc/sys/vm/drop_caches

  # Move foo from directory A to directory B.
  # This deletes the directory entries for foo in A from the log, and
  # does not add the new name for foo in directory B to the log, because
  # logged_trans of A is 0, which is less than the current transaction ID.
  $ mv /mnt/A/foo /mnt/B/foo

  # Now make an fsync to anything except A, B or any file inside them,
  # like for example create a file at the root directory and fsync this
  # new file. This syncs the log that contains all the changes done by
  # previous rename operation.
  $ touch /mnt/baz
  $ xfs_io -c "fsync" /mnt/baz

  <power fail>

  # Mount the filesystem and replay the log.
  $ mount /dev/sdc /mnt

  # Check the filesystem content.
  $ ls -1R /mnt
  /mnt/:
  A
  B
  baz

  /mnt/A:
  bar

  /mnt/B:
  $

  # File foo is gone, it's neither in A/ nor in B/.

Fix this by using the inode_logged() helper at btrfs_log_new_name(), which
safely checks if an inode was logged before in the current transaction.

A test case for fstests will follow soon.

CC: stable@vger.kernel.org # 4.14+
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index c2ebe700d6e2..8dde5c08a48f 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6536,8 +6536,8 @@ void btrfs_log_new_name(struct btrfs_trans_handle *trans,
 	 * if this inode hasn't been logged and directory we're renaming it
 	 * from hasn't been logged, we don't need to log it
 	 */
-	if (inode->logged_trans < trans->transid &&
-	    (!old_dir || old_dir->logged_trans < trans->transid))
+	if (!inode_logged(trans, inode) &&
+	    (!old_dir || !inode_logged(trans, old_dir)))
 		return;
 
 	/*
-- 
2.28.0

[PATCH 2/3] btrfs: eliminate some false positives when checking if inode was logged

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When checking if an inode was previously logged in the current transaction
through the helper inode_logged(), we can return some false positives that
can be easily eliminated. These correspond to the cases where an inode has
a ->logged_trans value that is not zero and its value is smaller then the
ID of the current transaction. This means we know exactly that the inode
was never logged before in the current transaction, so we can return false
and avoid the callers to do extra work:

1) Having btrfs_del_dir_entries_in_log() and btrfs_del_inode_ref_in_log()
   unnecessarily join a log transaction and do deletion searches in a log
   tree that will not find anything. This just adds unnecessary contention
   on extent buffer locks;

2) Having btrfs_log_new_name() unnecessarily log an inode when it is not
   needed. If the inode was not logged before, we don't need to log it in
   LOG_INODE_EXISTS mode.

So just make sure that any false positive only happens when ->logged_trans
has a value of 0.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 8dde5c08a48f..fc98b7a7a8e6 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3421,14 +3421,10 @@ int btrfs_free_log_root_tree(struct btrfs_trans_handle *trans,
 }
 
 /*
- * Check if an inode was logged in the current transaction. We can't always rely
- * on an inode's logged_trans value, because it's an in-memory only field and
- * therefore not persisted. This means that its value is lost if the inode gets
- * evicted and loaded again from disk (in which case it has a value of 0, and
- * certainly it is smaller then any possible transaction ID), when that happens
- * the full_sync flag is set in the inode's runtime flags, so on that case we
- * assume eviction happened and ignore the logged_trans value, assuming the
- * worst case, that the inode was logged before in the current transaction.
+ * Check if an inode was logged in the current transaction. This may often
+ * return some false positives, because logged_trans is an in memory only field,
+ * not persisted anywhere. This is meant to be used in contexts where a false
+ * positive has no functional consequences.
  */
 static bool inode_logged(struct btrfs_trans_handle *trans,
 			 struct btrfs_inode *inode)
@@ -3436,7 +3432,18 @@ static bool inode_logged(struct btrfs_trans_handle *trans,
 	if (inode->logged_trans == trans->transid)
 		return true;
 
-	if (inode->last_trans == trans->transid &&
+	/*
+	 * The inode's logged_trans is always 0 when we load it (because it is
+	 * not persisted in the inode item or elsewhere). So if it is 0, the
+	 * inode was last modified in the current transaction and has the
+	 * full_sync flag set, then the inode may have been logged before in
+	 * the current transaction, then evicted and loaded again in the current
+	 * transaction - or may have never been logged in the current transaction,
+	 * but since we can not be sure, we have to assume it was, otherwise our
+	 * callers can leave an inconsistent log.
+	 */
+	if (inode->logged_trans == 0 &&
+	    inode->last_trans == trans->transid &&
 	    test_bit(BTRFS_INODE_NEEDS_FULL_SYNC, &inode->runtime_flags) &&
 	    !test_bit(BTRFS_FS_LOG_RECOVERING, &trans->fs_info->flags))
 		return true;
-- 
2.28.0

[PATCH 3/3] btrfs: do not pin logs too early during renames

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

During renames we pin the logs of the roots a bit too early, before the
calls to btrfs_insert_inode_ref(). We can pin the logs after those calls,
since those will not change anything in a log tree.

In a scenario where we have multiple and diverse filesystem operations
running in parallel, those calls can take a significant amount of time,
due to lock contention on extent buffers, and delay log commits from other
tasks for longer than necessary.

So just pin logs after calls to btrfs_insert_inode_ref() and right before
the first operation that can update a log tree.

The following script that uses dbench was used for testing:

  $ cat dbench-test.sh
  #!/bin/bash

  DEV=/dev/nvme0n1
  MNT=/mnt/nvme0n1
  MOUNT_OPTIONS="-o ssd"
  MKFS_OPTIONS="-m single -d single"

  echo "performance" | tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor

  umount $DEV &> /dev/null
  mkfs.btrfs -f $MKFS_OPTIONS $DEV
  mount $MOUNT_OPTIONS $DEV $MNT

  dbench -D $MNT -t 120 16

  umount $MNT

The tests were run on a machine with 12 cores, 64G of RAN, a NVME device
and using a non-debug kernel config (Debian's default config).

The results compare a branch without this patch and without the previous
patch in the series, that has the subject:

 "btrfs: eliminate some false positives when checking if inode was logged"

Versus the same branch with these two patches applied.

dbench with 8 clients, results before:

 Operation      Count    AvgLat    MaxLat
 ----------------------------------------
 NTCreateX    4391359     0.009   249.745
 Close        3225882     0.001     3.243
 Rename        185953     0.065   240.643
 Unlink        886669     0.049   249.906
 Deltree          112     2.455   217.433
 Mkdir             56     0.002     0.004
 Qpathinfo    3980281     0.004     3.109
 Qfileinfo     697579     0.001     0.187
 Qfsinfo       729780     0.002     2.424
 Sfileinfo     357764     0.004     1.415
 Find         1538861     0.016     4.863
 WriteX       2189666     0.010     3.327
 ReadX        6883443     0.002     0.729
 LockX          14298     0.002     0.073
 UnlockX        14298     0.001     0.042
 Flush         307777     2.447   303.663

Throughput 1149.6 MB/sec  8 clients  8 procs  max_latency=303.666 ms

dbench with 8 clients, results after:

 Operation      Count    AvgLat    MaxLat
 ----------------------------------------
 NTCreateX    4269920     0.009   213.532
 Close        3136653     0.001     0.690
 Rename        180805     0.082   213.858
 Unlink        862189     0.050   172.893
 Deltree          112     2.998   218.328
 Mkdir             56     0.002     0.003
 Qpathinfo    3870158     0.004     5.072
 Qfileinfo     678375     0.001     0.194
 Qfsinfo       709604     0.002     0.485
 Sfileinfo     347850     0.004     1.304
 Find         1496310     0.017     5.504
 WriteX       2129613     0.010     2.882
 ReadX        6693066     0.002     1.517
 LockX          13902     0.002     0.075
 UnlockX        13902     0.001     0.055
 Flush         299276     2.511   220.189

Throughput 1187.33 MB/sec  8 clients  8 procs  max_latency=220.194 ms

+3.2% throughput, -31.8% max latency

dbench with 16 clients, results before:

 Operation      Count    AvgLat    MaxLat
 ----------------------------------------
 NTCreateX    5978334     0.028   156.507
 Close        4391598     0.001     1.345
 Rename        253136     0.241   155.057
 Unlink       1207220     0.182   257.344
 Deltree          160     6.123    36.277
 Mkdir             80     0.003     0.005
 Qpathinfo    5418817     0.012     6.867
 Qfileinfo     949929     0.001     0.941
 Qfsinfo       993560     0.002     1.386
 Sfileinfo     486904     0.004     2.829
 Find         2095088     0.059     8.164
 WriteX       2982319     0.017     9.029
 ReadX        9371484     0.002     4.052
 LockX          19470     0.002     0.461
 UnlockX        19470     0.001     0.990
 Flush         418936     2.740   347.902

Throughput 1495.31 MB/sec  16 clients  16 procs  max_latency=347.909 ms

dbench with 16 clients, results after:

 Operation      Count    AvgLat    MaxLat
 ----------------------------------------
 NTCreateX    5711833     0.029   131.240
 Close        4195897     0.001     1.732
 Rename        241849     0.204   147.831
 Unlink       1153341     0.184   231.322
 Deltree          160     6.086    30.198
 Mkdir             80     0.003     0.021
 Qpathinfo    5177011     0.012     7.150
 Qfileinfo     907768     0.001     0.793
 Qfsinfo       949205     0.002     1.431
 Sfileinfo     465317     0.004     2.454
 Find         2001541     0.058     7.819
 WriteX       2850661     0.017     9.110
 ReadX        8952289     0.002     3.991
 LockX          18596     0.002     0.655
 UnlockX        18596     0.001     0.179
 Flush         400342     2.879   293.607

Throughput 1565.73 MB/sec  16 clients  16 procs  max_latency=293.611 ms

+4.6% throughput, -16.9% max latency

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6089c5e7763c..59b35e12bb81 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9271,8 +9271,6 @@ static int btrfs_rename_exchange(struct inode *old_dir,
 		/* force full log commit if subvolume involved. */
 		btrfs_set_log_full_commit(trans);
 	} else {
-		btrfs_pin_log_trans(root);
-		root_log_pinned = true;
 		ret = btrfs_insert_inode_ref(trans, dest,
 					     new_dentry->d_name.name,
 					     new_dentry->d_name.len,
@@ -9289,8 +9287,6 @@ static int btrfs_rename_exchange(struct inode *old_dir,
 		/* force full log commit if subvolume involved. */
 		btrfs_set_log_full_commit(trans);
 	} else {
-		btrfs_pin_log_trans(dest);
-		dest_log_pinned = true;
 		ret = btrfs_insert_inode_ref(trans, root,
 					     old_dentry->d_name.name,
 					     old_dentry->d_name.len,
@@ -9321,6 +9317,29 @@ static int btrfs_rename_exchange(struct inode *old_dir,
 				BTRFS_I(new_inode), 1);
 	}
 
+	/*
+	 * Now pin the logs of the roots. We do it to ensure that no other task
+	 * can sync the logs while we are in progress with the rename, because
+	 * that could result in an inconsistency in case any of the inodes that
+	 * are part of this rename operation were logged before.
+	 *
+	 * We pin the logs even if at this precise moment none of the inodes was
+	 * logged before. This is because right after we checked for that, some
+	 * other task fsyncing some other inode not involved with this rename
+	 * operation could log that one of our inodes exists.
+	 *
+	 * We don't need to pin the logs before the above calls to
+	 * btrfs_insert_inode_ref(), since those don't ever need to change a log.
+	 */
+	if (old_ino != BTRFS_FIRST_FREE_OBJECTID) {
+		btrfs_pin_log_trans(root);
+		root_log_pinned = true;
+	}
+	if (new_ino != BTRFS_FIRST_FREE_OBJECTID) {
+		btrfs_pin_log_trans(dest);
+		dest_log_pinned = true;
+	}
+
 	/* src is a subvolume */
 	if (old_ino == BTRFS_FIRST_FREE_OBJECTID) {
 		ret = btrfs_unlink_subvol(trans, old_dir, old_dentry);
@@ -9573,8 +9592,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		/* force full log commit if subvolume involved. */
 		btrfs_set_log_full_commit(trans);
 	} else {
-		btrfs_pin_log_trans(root);
-		log_pinned = true;
 		ret = btrfs_insert_inode_ref(trans, dest,
 					     new_dentry->d_name.name,
 					     new_dentry->d_name.len,
@@ -9598,6 +9615,25 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (unlikely(old_ino == BTRFS_FIRST_FREE_OBJECTID)) {
 		ret = btrfs_unlink_subvol(trans, old_dir, old_dentry);
 	} else {
+		/*
+		 * Now pin the log. We do it to ensure that no other task can
+		 * sync the log while we are in progress with the rename, as
+		 * that could result in an inconsistency in case any of the
+		 * inodes that are part of this rename operation were logged
+		 * before.
+		 *
+		 * We pin the log even if at this precise moment none of the
+		 * inodes was logged before. This is because right after we
+		 * checked for that, some other task fsyncing some other inode
+		 * not involved with this rename operation could log that one of
+		 * our inodes exists.
+		 *
+		 * We don't need to pin the logs before the above call to
+		 * btrfs_insert_inode_ref(), since that does not need to change
+		 * a log.
+		 */
+		btrfs_pin_log_trans(root);
+		log_pinned = true;
 		ret = __btrfs_unlink_inode(trans, root, BTRFS_I(old_dir),
 					BTRFS_I(d_inode(old_dentry)),
 					old_dentry->d_name.name,
-- 
2.28.0

Re: [PATCH 0/3] btrfs: fsync changes, a bug fix and a couple improvements

[David Sterba]

On Tue, Jul 27, 2021 at 11:24:42AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The first patch in the series fixes a bug where a directory fsync, following
> by inode eviction, followed by renaming a file and syncing the log results
> in losing a file if a power failure happens and the log is replayed.
> 
> The remaining two changes are independent, and are about avoiding unnecessary
> work during operations that need to check or modify the log (renames, adding
> a hard link, unlinks) and making renames hold log commits for a shorter
> period.

Added to misc-next, thanks.

Re: [PATCH 3/3] btrfs: do not pin logs too early during renames

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 21494 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <019dba0981817da70e0820550267b368e3ce7389.1627379796.git.fdmanana@suse.com>
References: <019dba0981817da70e0820550267b368e3ce7389.1627379796.git.fdmanana@suse.com>
TO: fdmanana(a)kernel.org

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.14-rc3 next-20210727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/fdmanana-kernel-org/btrfs-fsync-changes-a-bug-fix-and-a-couple-improvements/20210727-182628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: arm64-randconfig-m031-20210728 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 10.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
fs/btrfs/inode.c:9338 btrfs_rename_exchange() warn: variable dereferenced before check 'new_inode' (see line 9225)

Old smatch warnings:
fs/btrfs/inode.c:285 insert_inline_extent() error: we previously assumed 'compressed_pages' could be null (see line 254)

vim +/new_inode +9338 fs/btrfs/inode.c

39279cc3d2704c Chris Mason     2007-06-12  9117  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9118  static int btrfs_rename_exchange(struct inode *old_dir,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9119  			      struct dentry *old_dentry,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9120  			      struct inode *new_dir,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9121  			      struct dentry *new_dentry)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9122  {
0b246afa62b0cf Jeff Mahoney    2016-06-22  9123  	struct btrfs_fs_info *fs_info = btrfs_sb(old_dir->i_sb);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9124  	struct btrfs_trans_handle *trans;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9125  	struct btrfs_root *root = BTRFS_I(old_dir)->root;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9126  	struct btrfs_root *dest = BTRFS_I(new_dir)->root;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9127  	struct inode *new_inode = new_dentry->d_inode;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9128  	struct inode *old_inode = old_dentry->d_inode;
95582b00838837 Deepa Dinamani  2018-05-08  9129  	struct timespec64 ctime = current_time(old_inode);
4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10  9130  	u64 old_ino = btrfs_ino(BTRFS_I(old_inode));
4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10  9131  	u64 new_ino = btrfs_ino(BTRFS_I(new_inode));
cdd1fedf8261cd Dan Fuhry       2016-03-17  9132  	u64 old_idx = 0;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9133  	u64 new_idx = 0;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9134  	int ret;
75b463d2b47aef Filipe Manana   2020-08-11  9135  	int ret2;
86e8aa0e772cab Filipe Manana   2016-05-05  9136  	bool root_log_pinned = false;
86e8aa0e772cab Filipe Manana   2016-05-05  9137  	bool dest_log_pinned = false;
dc09ef3562726c Josef Bacik     2021-05-19  9138  	bool need_abort = false;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9139  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9140  	/* we only allow rename subvolume link between subvolumes */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9141  	if (old_ino != BTRFS_FIRST_FREE_OBJECTID && root != dest)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9142  		return -EXDEV;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9143  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9144  	/* close the race window with snapshot create/destroy ioctl */
943eb3bf25f4a7 Josef Bacik     2019-11-19  9145  	if (old_ino == BTRFS_FIRST_FREE_OBJECTID ||
943eb3bf25f4a7 Josef Bacik     2019-11-19  9146  	    new_ino == BTRFS_FIRST_FREE_OBJECTID)
0b246afa62b0cf Jeff Mahoney    2016-06-22  9147  		down_read(&fs_info->subvol_sem);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9148  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9149  	/*
cdd1fedf8261cd Dan Fuhry       2016-03-17  9150  	 * We want to reserve the absolute worst case amount of items.  So if
cdd1fedf8261cd Dan Fuhry       2016-03-17  9151  	 * both inodes are subvols and we need to unlink them then that would
cdd1fedf8261cd Dan Fuhry       2016-03-17  9152  	 * require 4 item modifications, but if they are both normal inodes it
cdd1fedf8261cd Dan Fuhry       2016-03-17  9153  	 * would require 5 item modifications, so we'll assume their normal
cdd1fedf8261cd Dan Fuhry       2016-03-17  9154  	 * inodes.  So 5 * 2 is 10, plus 2 for the new links, so 12 total items
cdd1fedf8261cd Dan Fuhry       2016-03-17  9155  	 * should cover the worst case number of items we'll modify.
cdd1fedf8261cd Dan Fuhry       2016-03-17  9156  	 */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9157  	trans = btrfs_start_transaction(root, 12);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9158  	if (IS_ERR(trans)) {
cdd1fedf8261cd Dan Fuhry       2016-03-17  9159  		ret = PTR_ERR(trans);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9160  		goto out_notrans;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9161  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9162  
00aa8e87c9dc63 Josef Bacik     2021-03-12  9163  	if (dest != root) {
00aa8e87c9dc63 Josef Bacik     2021-03-12  9164  		ret = btrfs_record_root_in_trans(trans, dest);
00aa8e87c9dc63 Josef Bacik     2021-03-12  9165  		if (ret)
00aa8e87c9dc63 Josef Bacik     2021-03-12  9166  			goto out_fail;
00aa8e87c9dc63 Josef Bacik     2021-03-12  9167  	}
3e1740993e4311 Josef Bacik     2019-11-15  9168  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9169  	/*
cdd1fedf8261cd Dan Fuhry       2016-03-17  9170  	 * We need to find a free sequence number both in the source and
cdd1fedf8261cd Dan Fuhry       2016-03-17  9171  	 * in the destination directory for the exchange.
cdd1fedf8261cd Dan Fuhry       2016-03-17  9172  	 */
877574e2548bbf Nikolay Borisov 2017-02-20  9173  	ret = btrfs_set_inode_index(BTRFS_I(new_dir), &old_idx);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9174  	if (ret)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9175  		goto out_fail;
877574e2548bbf Nikolay Borisov 2017-02-20  9176  	ret = btrfs_set_inode_index(BTRFS_I(old_dir), &new_idx);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9177  	if (ret)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9178  		goto out_fail;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9179  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9180  	BTRFS_I(old_inode)->dir_index = 0ULL;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9181  	BTRFS_I(new_inode)->dir_index = 0ULL;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9182  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9183  	/* Reference for the source. */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9184  	if (old_ino == BTRFS_FIRST_FREE_OBJECTID) {
cdd1fedf8261cd Dan Fuhry       2016-03-17  9185  		/* force full log commit if subvolume involved. */
907877664e2d85 David Sterba    2019-03-20  9186  		btrfs_set_log_full_commit(trans);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9187  	} else {
cdd1fedf8261cd Dan Fuhry       2016-03-17  9188  		ret = btrfs_insert_inode_ref(trans, dest,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9189  					     new_dentry->d_name.name,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9190  					     new_dentry->d_name.len,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9191  					     old_ino,
f85b7379cd76ad David Sterba    2017-01-20  9192  					     btrfs_ino(BTRFS_I(new_dir)),
f85b7379cd76ad David Sterba    2017-01-20  9193  					     old_idx);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9194  		if (ret)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9195  			goto out_fail;
dc09ef3562726c Josef Bacik     2021-05-19  9196  		need_abort = true;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9197  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9198  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9199  	/* And now for the dest. */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9200  	if (new_ino == BTRFS_FIRST_FREE_OBJECTID) {
cdd1fedf8261cd Dan Fuhry       2016-03-17  9201  		/* force full log commit if subvolume involved. */
907877664e2d85 David Sterba    2019-03-20  9202  		btrfs_set_log_full_commit(trans);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9203  	} else {
cdd1fedf8261cd Dan Fuhry       2016-03-17  9204  		ret = btrfs_insert_inode_ref(trans, root,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9205  					     old_dentry->d_name.name,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9206  					     old_dentry->d_name.len,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9207  					     new_ino,
f85b7379cd76ad David Sterba    2017-01-20  9208  					     btrfs_ino(BTRFS_I(old_dir)),
f85b7379cd76ad David Sterba    2017-01-20  9209  					     new_idx);
dc09ef3562726c Josef Bacik     2021-05-19  9210  		if (ret) {
dc09ef3562726c Josef Bacik     2021-05-19  9211  			if (need_abort)
dc09ef3562726c Josef Bacik     2021-05-19  9212  				btrfs_abort_transaction(trans, ret);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9213  			goto out_fail;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9214  		}
dc09ef3562726c Josef Bacik     2021-05-19  9215  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9216  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9217  	/* Update inode version and ctime/mtime. */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9218  	inode_inc_iversion(old_dir);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9219  	inode_inc_iversion(new_dir);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9220  	inode_inc_iversion(old_inode);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9221  	inode_inc_iversion(new_inode);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9222  	old_dir->i_ctime = old_dir->i_mtime = ctime;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9223  	new_dir->i_ctime = new_dir->i_mtime = ctime;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9224  	old_inode->i_ctime = ctime;
cdd1fedf8261cd Dan Fuhry       2016-03-17 @9225  	new_inode->i_ctime = ctime;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9226  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9227  	if (old_dentry->d_parent != new_dentry->d_parent) {
f85b7379cd76ad David Sterba    2017-01-20  9228  		btrfs_record_unlink_dir(trans, BTRFS_I(old_dir),
f85b7379cd76ad David Sterba    2017-01-20  9229  				BTRFS_I(old_inode), 1);
f85b7379cd76ad David Sterba    2017-01-20  9230  		btrfs_record_unlink_dir(trans, BTRFS_I(new_dir),
f85b7379cd76ad David Sterba    2017-01-20  9231  				BTRFS_I(new_inode), 1);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9232  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9233  
c8758bb7bea761 Filipe Manana   2021-07-27  9234  	/*
c8758bb7bea761 Filipe Manana   2021-07-27  9235  	 * Now pin the logs of the roots. We do it to ensure that no other task
c8758bb7bea761 Filipe Manana   2021-07-27  9236  	 * can sync the logs while we are in progress with the rename, because
c8758bb7bea761 Filipe Manana   2021-07-27  9237  	 * that could result in an inconsistency in case any of the inodes that
c8758bb7bea761 Filipe Manana   2021-07-27  9238  	 * are part of this rename operation were logged before.
c8758bb7bea761 Filipe Manana   2021-07-27  9239  	 *
c8758bb7bea761 Filipe Manana   2021-07-27  9240  	 * We pin the logs even if at this precise moment none of the inodes was
c8758bb7bea761 Filipe Manana   2021-07-27  9241  	 * logged before. This is because right after we checked for that, some
c8758bb7bea761 Filipe Manana   2021-07-27  9242  	 * other task fsyncing some other inode not involved with this rename
c8758bb7bea761 Filipe Manana   2021-07-27  9243  	 * operation could log that one of our inodes exists.
c8758bb7bea761 Filipe Manana   2021-07-27  9244  	 *
c8758bb7bea761 Filipe Manana   2021-07-27  9245  	 * We don't need to pin the logs before the above calls to
c8758bb7bea761 Filipe Manana   2021-07-27  9246  	 * btrfs_insert_inode_ref(), since those don't ever need to change a log.
c8758bb7bea761 Filipe Manana   2021-07-27  9247  	 */
c8758bb7bea761 Filipe Manana   2021-07-27  9248  	if (old_ino != BTRFS_FIRST_FREE_OBJECTID) {
c8758bb7bea761 Filipe Manana   2021-07-27  9249  		btrfs_pin_log_trans(root);
c8758bb7bea761 Filipe Manana   2021-07-27  9250  		root_log_pinned = true;
c8758bb7bea761 Filipe Manana   2021-07-27  9251  	}
c8758bb7bea761 Filipe Manana   2021-07-27  9252  	if (new_ino != BTRFS_FIRST_FREE_OBJECTID) {
c8758bb7bea761 Filipe Manana   2021-07-27  9253  		btrfs_pin_log_trans(dest);
c8758bb7bea761 Filipe Manana   2021-07-27  9254  		dest_log_pinned = true;
c8758bb7bea761 Filipe Manana   2021-07-27  9255  	}
c8758bb7bea761 Filipe Manana   2021-07-27  9256  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9257  	/* src is a subvolume */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9258  	if (old_ino == BTRFS_FIRST_FREE_OBJECTID) {
045d3967b6920b Josef Bacik     2019-12-18  9259  		ret = btrfs_unlink_subvol(trans, old_dir, old_dentry);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9260  	} else { /* src is an inode */
4ec5934e43cabd Nikolay Borisov 2017-01-18  9261  		ret = __btrfs_unlink_inode(trans, root, BTRFS_I(old_dir),
4ec5934e43cabd Nikolay Borisov 2017-01-18  9262  					   BTRFS_I(old_dentry->d_inode),
cdd1fedf8261cd Dan Fuhry       2016-03-17  9263  					   old_dentry->d_name.name,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9264  					   old_dentry->d_name.len);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9265  		if (!ret)
9a56fcd15a9c6b Nikolay Borisov 2020-11-02  9266  			ret = btrfs_update_inode(trans, root, BTRFS_I(old_inode));
cdd1fedf8261cd Dan Fuhry       2016-03-17  9267  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9268  	if (ret) {
66642832f06a43 Jeff Mahoney    2016-06-10  9269  		btrfs_abort_transaction(trans, ret);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9270  		goto out_fail;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9271  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9272  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9273  	/* dest is a subvolume */
cdd1fedf8261cd Dan Fuhry       2016-03-17  9274  	if (new_ino == BTRFS_FIRST_FREE_OBJECTID) {
045d3967b6920b Josef Bacik     2019-12-18  9275  		ret = btrfs_unlink_subvol(trans, new_dir, new_dentry);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9276  	} else { /* dest is an inode */
4ec5934e43cabd Nikolay Borisov 2017-01-18  9277  		ret = __btrfs_unlink_inode(trans, dest, BTRFS_I(new_dir),
4ec5934e43cabd Nikolay Borisov 2017-01-18  9278  					   BTRFS_I(new_dentry->d_inode),
cdd1fedf8261cd Dan Fuhry       2016-03-17  9279  					   new_dentry->d_name.name,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9280  					   new_dentry->d_name.len);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9281  		if (!ret)
9a56fcd15a9c6b Nikolay Borisov 2020-11-02  9282  			ret = btrfs_update_inode(trans, dest, BTRFS_I(new_inode));
cdd1fedf8261cd Dan Fuhry       2016-03-17  9283  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9284  	if (ret) {
66642832f06a43 Jeff Mahoney    2016-06-10  9285  		btrfs_abort_transaction(trans, ret);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9286  		goto out_fail;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9287  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9288  
db0a669fb00241 Nikolay Borisov 2017-02-20  9289  	ret = btrfs_add_link(trans, BTRFS_I(new_dir), BTRFS_I(old_inode),
cdd1fedf8261cd Dan Fuhry       2016-03-17  9290  			     new_dentry->d_name.name,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9291  			     new_dentry->d_name.len, 0, old_idx);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9292  	if (ret) {
66642832f06a43 Jeff Mahoney    2016-06-10  9293  		btrfs_abort_transaction(trans, ret);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9294  		goto out_fail;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9295  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9296  
db0a669fb00241 Nikolay Borisov 2017-02-20  9297  	ret = btrfs_add_link(trans, BTRFS_I(old_dir), BTRFS_I(new_inode),
cdd1fedf8261cd Dan Fuhry       2016-03-17  9298  			     old_dentry->d_name.name,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9299  			     old_dentry->d_name.len, 0, new_idx);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9300  	if (ret) {
66642832f06a43 Jeff Mahoney    2016-06-10  9301  		btrfs_abort_transaction(trans, ret);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9302  		goto out_fail;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9303  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9304  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9305  	if (old_inode->i_nlink == 1)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9306  		BTRFS_I(old_inode)->dir_index = old_idx;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9307  	if (new_inode->i_nlink == 1)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9308  		BTRFS_I(new_inode)->dir_index = new_idx;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9309  
86e8aa0e772cab Filipe Manana   2016-05-05  9310  	if (root_log_pinned) {
75b463d2b47aef Filipe Manana   2020-08-11  9311  		btrfs_log_new_name(trans, BTRFS_I(old_inode), BTRFS_I(old_dir),
75b463d2b47aef Filipe Manana   2020-08-11  9312  				   new_dentry->d_parent);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9313  		btrfs_end_log_trans(root);
86e8aa0e772cab Filipe Manana   2016-05-05  9314  		root_log_pinned = false;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9315  	}
86e8aa0e772cab Filipe Manana   2016-05-05  9316  	if (dest_log_pinned) {
75b463d2b47aef Filipe Manana   2020-08-11  9317  		btrfs_log_new_name(trans, BTRFS_I(new_inode), BTRFS_I(new_dir),
75b463d2b47aef Filipe Manana   2020-08-11  9318  				   old_dentry->d_parent);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9319  		btrfs_end_log_trans(dest);
86e8aa0e772cab Filipe Manana   2016-05-05  9320  		dest_log_pinned = false;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9321  	}
cdd1fedf8261cd Dan Fuhry       2016-03-17  9322  out_fail:
86e8aa0e772cab Filipe Manana   2016-05-05  9323  	/*
86e8aa0e772cab Filipe Manana   2016-05-05  9324  	 * If we have pinned a log and an error happened, we unpin tasks
86e8aa0e772cab Filipe Manana   2016-05-05  9325  	 * trying to sync the log and force them to fallback to a transaction
86e8aa0e772cab Filipe Manana   2016-05-05  9326  	 * commit if the log currently contains any of the inodes involved in
86e8aa0e772cab Filipe Manana   2016-05-05  9327  	 * this rename operation (to ensure we do not persist a log with an
86e8aa0e772cab Filipe Manana   2016-05-05  9328  	 * inconsistent state for any of these inodes or leading to any
86e8aa0e772cab Filipe Manana   2016-05-05  9329  	 * inconsistencies when replayed). If the transaction was aborted, the
86e8aa0e772cab Filipe Manana   2016-05-05  9330  	 * abortion reason is propagated to userspace when attempting to commit
86e8aa0e772cab Filipe Manana   2016-05-05  9331  	 * the transaction. If the log does not contain any of these inodes, we
86e8aa0e772cab Filipe Manana   2016-05-05  9332  	 * allow the tasks to sync it.
86e8aa0e772cab Filipe Manana   2016-05-05  9333  	 */
86e8aa0e772cab Filipe Manana   2016-05-05  9334  	if (ret && (root_log_pinned || dest_log_pinned)) {
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9335  		if (btrfs_inode_in_log(BTRFS_I(old_dir), fs_info->generation) ||
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9336  		    btrfs_inode_in_log(BTRFS_I(new_dir), fs_info->generation) ||
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9337  		    btrfs_inode_in_log(BTRFS_I(old_inode), fs_info->generation) ||
86e8aa0e772cab Filipe Manana   2016-05-05 @9338  		    (new_inode &&
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9339  		     btrfs_inode_in_log(BTRFS_I(new_inode), fs_info->generation)))
907877664e2d85 David Sterba    2019-03-20  9340  			btrfs_set_log_full_commit(trans);
86e8aa0e772cab Filipe Manana   2016-05-05  9341  
86e8aa0e772cab Filipe Manana   2016-05-05  9342  		if (root_log_pinned) {
86e8aa0e772cab Filipe Manana   2016-05-05  9343  			btrfs_end_log_trans(root);
86e8aa0e772cab Filipe Manana   2016-05-05  9344  			root_log_pinned = false;
86e8aa0e772cab Filipe Manana   2016-05-05  9345  		}
86e8aa0e772cab Filipe Manana   2016-05-05  9346  		if (dest_log_pinned) {
86e8aa0e772cab Filipe Manana   2016-05-05  9347  			btrfs_end_log_trans(dest);
86e8aa0e772cab Filipe Manana   2016-05-05  9348  			dest_log_pinned = false;
86e8aa0e772cab Filipe Manana   2016-05-05  9349  		}
86e8aa0e772cab Filipe Manana   2016-05-05  9350  	}
c5b4a50b74018b Filipe Manana   2018-06-11  9351  	ret2 = btrfs_end_transaction(trans);
c5b4a50b74018b Filipe Manana   2018-06-11  9352  	ret = ret ? ret : ret2;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9353  out_notrans:
943eb3bf25f4a7 Josef Bacik     2019-11-19  9354  	if (new_ino == BTRFS_FIRST_FREE_OBJECTID ||
943eb3bf25f4a7 Josef Bacik     2019-11-19  9355  	    old_ino == BTRFS_FIRST_FREE_OBJECTID)
0b246afa62b0cf Jeff Mahoney    2016-06-22  9356  		up_read(&fs_info->subvol_sem);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9357  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9358  	return ret;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9359  }
cdd1fedf8261cd Dan Fuhry       2016-03-17  9360  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37662 bytes --]

Re: [PATCH 3/3] btrfs: do not pin logs too early during renames

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 6208 bytes --]

[ I don't know why Smatch is suddenly complaining about this code when
  it's 5 years old.  But we may as well remove the NULL check. - dan]

Hi,

url:    https://github.com/0day-ci/linux/commits/fdmanana-kernel-org/btrfs-fsync-changes-a-bug-fix-and-a-couple-improvements/20210727-182628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: arm64-randconfig-m031-20210728 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 10.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
fs/btrfs/inode.c:9338 btrfs_rename_exchange() warn: variable dereferenced before check 'new_inode' (see line 9225)

Old smatch warnings:
fs/btrfs/inode.c:285 insert_inline_extent() error: we previously assumed 'compressed_pages' could be null (see line 254)

vim +/new_inode +9338 fs/btrfs/inode.c

cdd1fedf8261cd Dan Fuhry       2016-03-17  9118  static int btrfs_rename_exchange(struct inode *old_dir,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9119  			      struct dentry *old_dentry,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9120  			      struct inode *new_dir,
cdd1fedf8261cd Dan Fuhry       2016-03-17  9121  			      struct dentry *new_dentry)
cdd1fedf8261cd Dan Fuhry       2016-03-17  9122  {
0b246afa62b0cf Jeff Mahoney    2016-06-22  9123  	struct btrfs_fs_info *fs_info = btrfs_sb(old_dir->i_sb);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9124  	struct btrfs_trans_handle *trans;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9125  	struct btrfs_root *root = BTRFS_I(old_dir)->root;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9126  	struct btrfs_root *dest = BTRFS_I(new_dir)->root;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9127  	struct inode *new_inode = new_dentry->d_inode;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9128  	struct inode *old_inode = old_dentry->d_inode;
95582b00838837 Deepa Dinamani  2018-05-08  9129  	struct timespec64 ctime = current_time(old_inode);
4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10  9130  	u64 old_ino = btrfs_ino(BTRFS_I(old_inode));
4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10  9131  	u64 new_ino = btrfs_ino(BTRFS_I(new_inode));
                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"new_inode" can't be NULL.  Or more properly we are toasted if it is.

cdd1fedf8261cd Dan Fuhry       2016-03-17  9132  	u64 old_idx = 0;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9133  	u64 new_idx = 0;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9134  	int ret;

[ snip ]

cdd1fedf8261cd Dan Fuhry       2016-03-17  9322  out_fail:
86e8aa0e772cab Filipe Manana   2016-05-05  9323  	/*
86e8aa0e772cab Filipe Manana   2016-05-05  9324  	 * If we have pinned a log and an error happened, we unpin tasks
86e8aa0e772cab Filipe Manana   2016-05-05  9325  	 * trying to sync the log and force them to fallback to a transaction
86e8aa0e772cab Filipe Manana   2016-05-05  9326  	 * commit if the log currently contains any of the inodes involved in
86e8aa0e772cab Filipe Manana   2016-05-05  9327  	 * this rename operation (to ensure we do not persist a log with an
86e8aa0e772cab Filipe Manana   2016-05-05  9328  	 * inconsistent state for any of these inodes or leading to any
86e8aa0e772cab Filipe Manana   2016-05-05  9329  	 * inconsistencies when replayed). If the transaction was aborted, the
86e8aa0e772cab Filipe Manana   2016-05-05  9330  	 * abortion reason is propagated to userspace when attempting to commit
86e8aa0e772cab Filipe Manana   2016-05-05  9331  	 * the transaction. If the log does not contain any of these inodes, we
86e8aa0e772cab Filipe Manana   2016-05-05  9332  	 * allow the tasks to sync it.
86e8aa0e772cab Filipe Manana   2016-05-05  9333  	 */
86e8aa0e772cab Filipe Manana   2016-05-05  9334  	if (ret && (root_log_pinned || dest_log_pinned)) {
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9335  		if (btrfs_inode_in_log(BTRFS_I(old_dir), fs_info->generation) ||
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9336  		    btrfs_inode_in_log(BTRFS_I(new_dir), fs_info->generation) ||
0f8939b8ac8623 Nikolay Borisov 2017-01-18  9337  		    btrfs_inode_in_log(BTRFS_I(old_inode), fs_info->generation) ||
86e8aa0e772cab Filipe Manana   2016-05-05 @9338  		    (new_inode &&
                                                                     ^^^^^^^^^
This check is not required and only upsets the static checker.

0f8939b8ac8623 Nikolay Borisov 2017-01-18  9339  		     btrfs_inode_in_log(BTRFS_I(new_inode), fs_info->generation)))
907877664e2d85 David Sterba    2019-03-20  9340  			btrfs_set_log_full_commit(trans);
86e8aa0e772cab Filipe Manana   2016-05-05  9341  
86e8aa0e772cab Filipe Manana   2016-05-05  9342  		if (root_log_pinned) {
86e8aa0e772cab Filipe Manana   2016-05-05  9343  			btrfs_end_log_trans(root);
86e8aa0e772cab Filipe Manana   2016-05-05  9344  			root_log_pinned = false;
86e8aa0e772cab Filipe Manana   2016-05-05  9345  		}
86e8aa0e772cab Filipe Manana   2016-05-05  9346  		if (dest_log_pinned) {
86e8aa0e772cab Filipe Manana   2016-05-05  9347  			btrfs_end_log_trans(dest);
86e8aa0e772cab Filipe Manana   2016-05-05  9348  			dest_log_pinned = false;
86e8aa0e772cab Filipe Manana   2016-05-05  9349  		}
86e8aa0e772cab Filipe Manana   2016-05-05  9350  	}
c5b4a50b74018b Filipe Manana   2018-06-11  9351  	ret2 = btrfs_end_transaction(trans);
c5b4a50b74018b Filipe Manana   2018-06-11  9352  	ret = ret ? ret : ret2;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9353  out_notrans:
943eb3bf25f4a7 Josef Bacik     2019-11-19  9354  	if (new_ino == BTRFS_FIRST_FREE_OBJECTID ||
943eb3bf25f4a7 Josef Bacik     2019-11-19  9355  	    old_ino == BTRFS_FIRST_FREE_OBJECTID)
0b246afa62b0cf Jeff Mahoney    2016-06-22  9356  		up_read(&fs_info->subvol_sem);
cdd1fedf8261cd Dan Fuhry       2016-03-17  9357  
cdd1fedf8261cd Dan Fuhry       2016-03-17  9358  	return ret;
cdd1fedf8261cd Dan Fuhry       2016-03-17  9359  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 3/3] btrfs: do not pin logs too early during renames

[Filipe Manana]

[-- Attachment #1: Type: text/plain, Size: 7303 bytes --]

On Thu, Jul 29, 2021 at 10:27 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> [ I don't know why Smatch is suddenly complaining about this code when
>   it's 5 years old.  But we may as well remove the NULL check. - dan]
>
> Hi,
>
> url:    https://github.com/0day-ci/linux/commits/fdmanana-kernel-org/btrfs-fsync-changes-a-bug-fix-and-a-couple-improvements/20210727-182628
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: arm64-randconfig-m031-20210728 (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 10.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> New smatch warnings:
> fs/btrfs/inode.c:9338 btrfs_rename_exchange() warn: variable dereferenced before check 'new_inode' (see line 9225)
>
> Old smatch warnings:
> fs/btrfs/inode.c:285 insert_inline_extent() error: we previously assumed 'compressed_pages' could be null (see line 254)
>
> vim +/new_inode +9338 fs/btrfs/inode.c
>
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9118  static int btrfs_rename_exchange(struct inode *old_dir,
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9119                               struct dentry *old_dentry,
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9120                               struct inode *new_dir,
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9121                               struct dentry *new_dentry)
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9122  {
> 0b246afa62b0cf Jeff Mahoney    2016-06-22  9123         struct btrfs_fs_info *fs_info = btrfs_sb(old_dir->i_sb);
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9124         struct btrfs_trans_handle *trans;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9125         struct btrfs_root *root = BTRFS_I(old_dir)->root;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9126         struct btrfs_root *dest = BTRFS_I(new_dir)->root;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9127         struct inode *new_inode = new_dentry->d_inode;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9128         struct inode *old_inode = old_dentry->d_inode;
> 95582b00838837 Deepa Dinamani  2018-05-08  9129         struct timespec64 ctime = current_time(old_inode);
> 4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10  9130         u64 old_ino = btrfs_ino(BTRFS_I(old_inode));
> 4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10  9131         u64 new_ino = btrfs_ino(BTRFS_I(new_inode));
>                                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "new_inode" can't be NULL.  Or more properly we are toasted if it is.
>
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9132         u64 old_idx = 0;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9133         u64 new_idx = 0;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9134         int ret;
>
> [ snip ]
>
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9322  out_fail:
> 86e8aa0e772cab Filipe Manana   2016-05-05  9323         /*
> 86e8aa0e772cab Filipe Manana   2016-05-05  9324          * If we have pinned a log and an error happened, we unpin tasks
> 86e8aa0e772cab Filipe Manana   2016-05-05  9325          * trying to sync the log and force them to fallback to a transaction
> 86e8aa0e772cab Filipe Manana   2016-05-05  9326          * commit if the log currently contains any of the inodes involved in
> 86e8aa0e772cab Filipe Manana   2016-05-05  9327          * this rename operation (to ensure we do not persist a log with an
> 86e8aa0e772cab Filipe Manana   2016-05-05  9328          * inconsistent state for any of these inodes or leading to any
> 86e8aa0e772cab Filipe Manana   2016-05-05  9329          * inconsistencies when replayed). If the transaction was aborted, the
> 86e8aa0e772cab Filipe Manana   2016-05-05  9330          * abortion reason is propagated to userspace when attempting to commit
> 86e8aa0e772cab Filipe Manana   2016-05-05  9331          * the transaction. If the log does not contain any of these inodes, we
> 86e8aa0e772cab Filipe Manana   2016-05-05  9332          * allow the tasks to sync it.
> 86e8aa0e772cab Filipe Manana   2016-05-05  9333          */
> 86e8aa0e772cab Filipe Manana   2016-05-05  9334         if (ret && (root_log_pinned || dest_log_pinned)) {
> 0f8939b8ac8623 Nikolay Borisov 2017-01-18  9335                 if (btrfs_inode_in_log(BTRFS_I(old_dir), fs_info->generation) ||
> 0f8939b8ac8623 Nikolay Borisov 2017-01-18  9336                     btrfs_inode_in_log(BTRFS_I(new_dir), fs_info->generation) ||
> 0f8939b8ac8623 Nikolay Borisov 2017-01-18  9337                     btrfs_inode_in_log(BTRFS_I(old_inode), fs_info->generation) ||
> 86e8aa0e772cab Filipe Manana   2016-05-05 @9338                     (new_inode &&
>                                                                      ^^^^^^^^^
> This check is not required and only upsets the static checker.

Indeed, it's not needed as new_inode can never be NULL for a rename
exchange operation.
The code was likely copy-pasted from btrfs_rename(), used for regular
renames, where new_inode can be NULL.

I just sent a patch to remove the check:

https://lore.kernel.org/linux-btrfs/8dd8e8f3020a5bd13ae22a1f21b8328adc1f4636.1627568438.git.fdmanana(a)suse.com/

Thanks.

>
> 0f8939b8ac8623 Nikolay Borisov 2017-01-18  9339                      btrfs_inode_in_log(BTRFS_I(new_inode), fs_info->generation)))
> 907877664e2d85 David Sterba    2019-03-20  9340                         btrfs_set_log_full_commit(trans);
> 86e8aa0e772cab Filipe Manana   2016-05-05  9341
> 86e8aa0e772cab Filipe Manana   2016-05-05  9342                 if (root_log_pinned) {
> 86e8aa0e772cab Filipe Manana   2016-05-05  9343                         btrfs_end_log_trans(root);
> 86e8aa0e772cab Filipe Manana   2016-05-05  9344                         root_log_pinned = false;
> 86e8aa0e772cab Filipe Manana   2016-05-05  9345                 }
> 86e8aa0e772cab Filipe Manana   2016-05-05  9346                 if (dest_log_pinned) {
> 86e8aa0e772cab Filipe Manana   2016-05-05  9347                         btrfs_end_log_trans(dest);
> 86e8aa0e772cab Filipe Manana   2016-05-05  9348                         dest_log_pinned = false;
> 86e8aa0e772cab Filipe Manana   2016-05-05  9349                 }
> 86e8aa0e772cab Filipe Manana   2016-05-05  9350         }
> c5b4a50b74018b Filipe Manana   2018-06-11  9351         ret2 = btrfs_end_transaction(trans);
> c5b4a50b74018b Filipe Manana   2018-06-11  9352         ret = ret ? ret : ret2;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9353  out_notrans:
> 943eb3bf25f4a7 Josef Bacik     2019-11-19  9354         if (new_ino == BTRFS_FIRST_FREE_OBJECTID ||
> 943eb3bf25f4a7 Josef Bacik     2019-11-19  9355             old_ino == BTRFS_FIRST_FREE_OBJECTID)
> 0b246afa62b0cf Jeff Mahoney    2016-06-22  9356                 up_read(&fs_info->subvol_sem);
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9357
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9358         return ret;
> cdd1fedf8261cd Dan Fuhry       2016-03-17  9359  }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
>