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: <0cd2a81f-00f0-4afa-ab24-448d89e1d4e3@wdc.com>
Date:   Thu, 14 Sep 2023 14:16:28 +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:
>> +
>> +	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.

Actually debug prints have shown that I do indeed sometimes hit the case 
where I need to call btrfs_next_item(). So the loop has to stay.

>> -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.

I thought about this a bit more. btrfs_map_block() also gets length 
passed in by reference, so it makes sense to do as well in 
set_io_stripe() and btrfs_get_raid_extent_offset() IMHO.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ