From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============3408111524546440247==" MIME-Version: 1.0 From: Filipe Manana To: kbuild-all@lists.01.org Subject: Re: [PATCH 3/3] btrfs: do not pin logs too early during renames Date: Thu, 29 Jul 2021 15:40:17 +0100 Message-ID: In-Reply-To: <202107290512.NuBwLok7-lkp@intel.com> List-Id: --===============3408111524546440247== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Thu, Jul 29, 2021 at 10:27 AM Dan Carpenter = 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/btrf= s-fsync-changes-a-bug-fix-and-a-couple-improvements/20210727-182628 > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git f= or-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 > Reported-by: Dan Carpenter > > 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_i= nfo *fs_info =3D btrfs_sb(old_dir->i_sb); > cdd1fedf8261cd Dan Fuhry 2016-03-17 9124 struct btrfs_tran= s_handle *trans; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9125 struct btrfs_root= *root =3D BTRFS_I(old_dir)->root; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9126 struct btrfs_root= *dest =3D BTRFS_I(new_dir)->root; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9127 struct inode *new= _inode =3D new_dentry->d_inode; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9128 struct inode *old= _inode =3D old_dentry->d_inode; > 95582b00838837 Deepa Dinamani 2018-05-08 9129 struct timespec64= ctime =3D current_time(old_inode); > 4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10 9130 u64 old_ino =3D b= trfs_ino(BTRFS_I(old_inode)); > 4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10 9131 u64 new_ino =3D b= trfs_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 =3D 0; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9133 u64 new_idx =3D 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 pin= ned 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 op= eration (to ensure we do not persist a log with an > 86e8aa0e772cab Filipe Manana 2016-05-05 9328 * inconsistent s= tate for any of these inodes or leading to any > 86e8aa0e772cab Filipe Manana 2016-05-05 9329 * inconsistencie= s when replayed). If the transaction was aborted, the > 86e8aa0e772cab Filipe Manana 2016-05-05 9330 * abortion reaso= n is propagated to userspace when attempting to commit > 86e8aa0e772cab Filipe Manana 2016-05-05 9331 * the transactio= n. If the log does not contain any of these inodes, we > 86e8aa0e772cab Filipe Manana 2016-05-05 9332 * allow the task= s 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/8dd8e8f3020a5bd13ae22a1f21b8328adc1f463= 6.1627568438.git.fdmanana(a)suse.com/ Thanks. > > 0f8939b8ac8623 Nikolay Borisov 2017-01-18 9339 btrf= s_inode_in_log(BTRFS_I(new_inode), fs_info->generation))) > 907877664e2d85 David Sterba 2019-03-20 9340 b= trfs_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 b= trfs_end_log_trans(root); > 86e8aa0e772cab Filipe Manana 2016-05-05 9344 r= oot_log_pinned =3D 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 b= trfs_end_log_trans(dest); > 86e8aa0e772cab Filipe Manana 2016-05-05 9348 d= est_log_pinned =3D false; > 86e8aa0e772cab Filipe Manana 2016-05-05 9349 } > 86e8aa0e772cab Filipe Manana 2016-05-05 9350 } > c5b4a50b74018b Filipe Manana 2018-06-11 9351 ret2 =3D btrfs_en= d_transaction(trans); > c5b4a50b74018b Filipe Manana 2018-06-11 9352 ret =3D ret ? ret= : ret2; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9353 out_notrans: > 943eb3bf25f4a7 Josef Bacik 2019-11-19 9354 if (new_ino =3D= =3D BTRFS_FIRST_FREE_OBJECTID || > 943eb3bf25f4a7 Josef Bacik 2019-11-19 9355 old_ino =3D= =3D 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 > --===============3408111524546440247==--