linux-kernel - Re: [PATCH v8 05/11] btrfs: lookup physical address from stripe extent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d77c2b4-c4f6-4f51-b578-9e38730b11fc@wdc.com>
Date:   Thu, 14 Sep 2023 09:45:02 +0000
From:   Johannes Thumshirn <Johannes.Thumshirn@....com>
To:     Qu Wenruo <quwenruo.btrfs@....com>, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>
CC:     Christoph Hellwig <hch@....de>,
        Naohiro Aota <Naohiro.Aota@....com>, Qu Wenruo <wqu@...e.com>,
        Damien Le Moal <dlemoal@...nel.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 05/11] btrfs: lookup physical address from stripe
 extent

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.