Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Mar 2023 09:55:42 -0700
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     "Colin King (gmail)" <colin.i.king@gmail.com>
Cc:     Chao Yu <chao@kernel.org>, linux-f2fs-devel@lists.sourceforge.net,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: f2fs: factor out discard_cmd usage from general rb_tree use
Message-ID: <ZB3WDpunfgJZhhQy@google.com>
References: <e50ebe1a-73a0-5800-71e3-0ddd366727ac@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e50ebe1a-73a0-5800-71e3-0ddd366727ac@gmail.com>
Precedence: bulk

On 03/24, Colin King (gmail) wrote:
> Hi,
> 
> static analysis with clang scan build has detected a potential issue
> introduced by the following commit:
> 
> commit 7e9775a516ff6c1e73ee2b42ec563cafee38f42f
> Author: Jaegeuk Kim <jaegeuk@kernel.org>
> Date:   Fri Mar 10 11:12:35 2023 -0800
> 
> f2fs: factor out discard_cmd usage from general rb_tree use

Good catch!
I found the bug and will post v2 soon.

> 
> 
> The warning is as follows:
> 
> fs/f2fs/segment.c:1425:4: warning: Value stored to 'tdc' is never read
> [deadcode.DeadStores]
> 
> The while loop in function __update_discard_tree_range is as follows (+ my
> annotations):
> 
> 
>         while (1) {
>                 struct rb_node *node;
>                 struct discard_cmd *tdc = NULL;
> 
> ### tdc is set to NULL
> 
>                 if (prev_dc) {
>                         di.lstart = prev_dc->di.lstart + prev_dc->di.len;
>                         if (di.lstart < lstart)
>                                 di.lstart = lstart;
>                         if (di.lstart >= end)
>                                 break;
> 
>                         if (!next_dc || next_dc->di.lstart > end)
>                                 di.len = end - di.lstart;
>                         else
>                                 di.len = next_dc->di.lstart - di.lstart;
>                         di.start = start + di.lstart - lstart;
>                 }
> 
>                 if (!di.len)
>                         goto next;
> 
>                 if (prev_dc && prev_dc->state == D_PREP &&
>                         prev_dc->bdev == bdev &&
>                         __is_discard_back_mergeable(&di, &prev_dc->di,
> 
> max_discard_blocks)) {
>                         prev_dc->di.len += di.len;
>                         dcc->undiscard_blks += di.len;
>                         __relocate_discard_cmd(dcc, prev_dc);
>                         di = prev_dc->di;
>                         tdc = prev_dc;
> 
> ### tdc is set to prev_dc, however, it is not not read any more with th
> introduction of the "goto next"" statement introduced in the commit
> mentioned earlier
> 
>                         goto next;
>                 }
> 
>                 if (next_dc && next_dc->state == D_PREP &&
>                         next_dc->bdev == bdev &&
>                         __is_discard_front_mergeable(&di, &next_dc->di,
> 
> max_discard_blocks)) {
>                         next_dc->di.lstart = di.lstart;
>                         next_dc->di.len += di.len;
>                         next_dc->di.start = di.start;
>                         dcc->undiscard_blks += di.len;
>                         __relocate_discard_cmd(dcc, next_dc);
> 
> ### tdc is always NULL, there is no path to this code where tdc is ever set
> to a non-NULL value.
> 
>                         if (tdc)
>                                 __remove_discard_cmd(sbi, tdc);
>                         goto next;
>                 }
> 
>                 __insert_discard_cmd(sbi, bdev, di.lstart, di.start,
> di.len);
>  next:
>                 prev_dc = next_dc;
>                 if (!prev_dc)
>                         break;
> 
>                 node = rb_next(&prev_dc->rb_node);
>                 next_dc = rb_entry_safe(node, struct discard_cmd, rb_node);
>         }
>