Re: [PATCH] btrfs: replace manual next item search with btrfs_get_next_valid_item()

[Sun YangKai <sunk67188@gmail.com>]

> 在 2025/7/31 00:23, Sun YangKai 写道:
> > The function btrfs_get_next_valid_item() was introduced in
> > commit 62142be363ae9("btrfs: introduce btrfs_for_each_slot iterator
> > macro")
> > and has never been used in other places other than
> > the btrfs_for_each_slot macro.
> 
> That's because the function name is not that straight-forward.
> 
> The name btrfs_get_"next"_valid_item() implies it would move to the next
> slot, but it's not the case.
> 
> Thus this can lead to quite some confusion and we're not that excited to
> use it.

Great point, and I also find the name is kind of confusing. Maybe we can have a 
better name like btrfs_ensure_valid_item() or something similar.

> One example inlined below:
> 
> [...]
> 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index fa7a929a0461..70e73cbf6b16 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1723,18 +1723,13 @@ static int find_free_dev_extent(struct
> > btrfs_device *device, u64 num_bytes,> 
> >   		goto out;
> >   	
> >   	while (search_start < search_end) {
> > 
> > +		ret = btrfs_get_next_valid_item(root, &key, path);
> > +		if (ret < 0)
> > +			goto out;
> > +		if (ret > 0)
> > +			break;
> 
> Here just by the name, it looks like it will move to the next slot thus
> can be problematic (as it will skip the current slot).
> 
> But it's not the case, as if the current slot is not beyond the current
> leaf, we just return with the current item key stored into @key.
> 
> 
> Another thing is, we may not need to bother the @key, or the caller will
> manually check the item key anyway. Compared to the confusing name, this
> is just a minor problem though.
> 
> But still it modifies the @key, which may or may not be what we want.

If I recall correctly, there's only one case that we don't want to bother with 
the @key in this patch, and I'll discard that change in the future versions  
if necessary. In all other cases,  we just read the key according to the path, 
which is part of the pattern I want to replace with this helper.

> That's why we are not excited to use the new helper.
> 
> Thanks,
> Qu
> 
> >   		l = path->nodes[0];
> >   		slot = path->slots[0];
> > 
> > -		if (slot >= btrfs_header_nritems(l)) {
> > -			ret = btrfs_next_leaf(root, path);
> > -			if (ret == 0)
> > -				continue;
> > -			if (ret < 0)
> > -				goto out;
> > -
> > -			break;
> > -		}
> > -		btrfs_item_key_to_cpu(l, &key, slot);
> > 
> >   		if (key.objectid < device->devid)
> >   		
> >   			goto next;
> > 

Thanks,
YangKai