Re: [PATCH v2] btrfs: fix nonzero lowest level handling in btrfs_search_forward()

[Sun YangKai <sunk67188@gmail.com>]

> > On Tue, Apr 29, 2025 at 02:57:02PM +0800, Sun YangKai wrote:
> > > Hi maintainers and community,
> > > 
> > > I'd like to request some feedback on this issue.
> > > 
> > > Is this behavior not considered a bug, or does it require further work?
> > > 
> > > Given that this issue never seems to be triggered in the current code
> > > base,
> > > perhaps we could consider cleaning up and removing the lowest_level
> > > related
> > > logic altogether?
> > 
> > I've looked at this a few times and I'm not decided what to do. It's an
> > old code and works given that we haven't seen any problems so removing
> > dead code makes sense. OTOH that one function does not pass some
> > values of parameters does not mean want to remove the implementation
> > completely. At least some assertions should be added to handle the
> > case(s) for the removed code if there's a new use.
> 
> Thank you for your kind reply.
> 
> The current implementation contains misleading functionality regarding non-
> zero lowest_level values. While the code appears to support customized path-
> >lowest_level cases, the actual handling of non-zero lowest_level values
> 
> contains critical flaws. This creates a false impression that the function
> can operate correctly under these conditions when in reality the
> implementation is invalid. The presence of this obsolete code path risks
> user confusion and potential misuse.
> 
> So I come up with some options about what we can do:
> 
> 1. (this is what this patch trying to do) Try to fix the wrong code and make
> this function work well on non-zero `path->lowest_level`. But since the
> non- zero lowest_level codepath is never used, maybe this just changes some
> dead code to prevent potential errors that may occur in the future.
> 
> 2. Since this function has never been called with non-zero
> path->lowest_level, we can safely eliminate the associated validation,
> permanently disregard the parameter, and explicitly document this behavior
> (always searching down to level 0) in comments. This simplifies the logic
> by removing obsolete dead code.
> 
> 3. Remove the dead code like 2, but add assertions when this function is
> called with a non-zero `path->lowest_path`. This can also prevent potential
> errors.
> 
> 4. Leave the code untouched, add a comment saying that never call this with
> a non-zero `path->lowest_level`.
> 
> Given its legacy status, there's no foreseeable requirement to retain
> non-zero level search capabilities in this code. So maybe option 2 is good
> enough.

Sorry I just found that I forget to cc the mail list in the last reply.

I will send a new patch later, maybe this version is better.

YangKai