On 14.09.23 11:18, Qu Wenruo wrote:
>> + if (ret) {
>> + if (path->slots[0] != 0)
>> + path->slots[0]--;
>
> IIRC we have btrfs_previous_item() to do the forward search.
>
>> + }
>> +
>> + end = logical + *length;
>
> IMHO we can make it const and initialize it at the definition part.
Right.
>> +
>> + while (1) {
>
> Here we only can hit at most one RST item, thus I'd recommend to remove
> the while().
>
> Although this would mean we will need a if () to handle (ret > 0) case,
> but it may still be a little easier to read than a loop.
>
> You may want to refer to btrfs_lookup_csum() for the non-loop
> implementation.
Sure I'll look into it.
>> +
>> + if (encoding != btrfs_bg_type_to_raid_encoding(map_type)) {
>> + ret = -ENOENT;
>> + goto out;
>
> This looks like a very weird situation, we have a bg with a different type.
> Should we do some warning or is there some valid situation for this?
>
Yep and probably return -EUCLEAN and set the FS to r/o.
>> +out:
>> + if (ret > 0)
>> + ret = -ENOENT;
>> + if (ret && ret != -EIO) {
>> + /*
>> + * Check if the range we're looking for is actually backed by
>> + * an extent. This can happen, e.g. when scrub is running on a
>> + * block-group and the extent it is trying to scrub get's
>> + * deleted in the meantime. Although scrub is setting the
>> + * block-group to read-only, deletion of extents are still
>> + * allowed. If the extent is gone, simply return ENOENT and be
>> + * good.
>> + */
>
> As mentioned in the next patch (sorry for the reversed order), this
> should be handled in a different way (by only searching commit root for
> scrub usage).
Yep I already have a prototype for that, but it needs more testing.
>>
>> -static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *map,
>> - u32 stripe_index, u64 stripe_offset, u32 stripe_nr)
>> +static int set_io_stripe(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>> + u64 logical, u64 *length, struct btrfs_io_stripe *dst,
>> + struct map_lookup *map, u32 stripe_index,
>> + u64 stripe_offset, u64 stripe_nr)
> Do we need @length to be a pointer?
> IIRC we can return the number of bytes we mapped, or <0 for errors.
> Thus at least @length doesn't need to be a pointer.
Good point, I'll update.