From: Filipe Manana <fdmanana@kernel.org>
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 [thread overview]
Message-ID: <CAL3q7H6UothiZYqYaRTKN0T_oyg8X6YAepH-t-rOi_3AnX-LmQ@mail.gmail.com> (raw)
In-Reply-To: <202107290512.NuBwLok7-lkp@intel.com>
[-- 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
>
next prev parent reply other threads:[~2021-07-29 14:40 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-28 22:04 [PATCH 3/3] btrfs: do not pin logs too early during renames kernel test robot
2021-07-29 9:27 ` Dan Carpenter
2021-07-29 14:40 ` Filipe Manana [this message]
-- strict thread matches above, loose matches on Subject: below --
2021-07-27 10:24 [PATCH 0/3] btrfs: fsync changes, a bug fix and a couple improvements fdmanana
2021-07-27 10:24 ` [PATCH 1/3] btrfs: fix lost inode on log replay after mix of fsync, rename and inode eviction fdmanana
2021-07-27 10:24 ` [PATCH 2/3] btrfs: eliminate some false positives when checking if inode was logged fdmanana
2021-07-27 10:24 ` [PATCH 3/3] btrfs: do not pin logs too early during renames fdmanana
2021-07-28 12:41 ` [PATCH 0/3] btrfs: fsync changes, a bug fix and a couple improvements David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAL3q7H6UothiZYqYaRTKN0T_oyg8X6YAepH-t-rOi_3AnX-LmQ@mail.gmail.com \
--to=fdmanana@kernel.org \
--cc=kbuild-all@lists.01.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.