From: Qu Wenruo <wqu@suse.com>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>, Filipe Manana <fdmanana@kernel.org>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH RFC 2/8] btrfs: rename members of can_nocow_file_extent_args
Date: Sat, 13 Apr 2024 07:42:44 +0930 [thread overview]
Message-ID: <adb96734-6521-4c13-ae86-f70a5f8848fe@suse.com> (raw)
In-Reply-To: <766b8e1e-0c04-4fdb-ae76-b92cd8f85bc3@gmx.com>
在 2024/4/13 07:30, Qu Wenruo 写道:
>
>
> 在 2024/4/12 22:51, Filipe Manana 写道:
>> On Thu, Apr 11, 2024 at 11:03 PM Qu Wenruo <quwenruo.btrfs@gmx.com>
>> wrote:
> [...]
>>>
>>> Well, the new @block_start matches the old extent_map::block_start.
>>
>> So it becomes a single exception, different from everywhere else.
>> Doesn't seem like a good thing in general.
>
> OK, I can get rid of the @block_start name.
>
>>
>>>
>>> I have to say, we do not have a solid definition on "disk_bytenr" in the
>>> first place.
>>
>> Well I find the name clear, it is a disk location measured by a byte
>> address.
>> block_start is not so clear for anyone not familiar with btrfs'
>> internals, it makes me think of a block number and wonder what's the
>> block size, etc.
>>
>>>
>>> Should it always match ondisk file_extent_item::disk_bytenr, or should
>>> it act like "block_start" of the old extent_map?
>>
>> It's always about a range of a file extent item, be it the whole range
>> or just a part of it.
>> I don't see why it's confusing to use disk_bytenr, etc.
>> I find it more confusing to use something else, or at least what's
>> being proposed in this patch.
>
> Well, IMHO since we take the name @disk_bytenr from btrfs file extent
> item, and btrfs file extent uses @disk_bytenr to uniquely locate a data
> extent, then we should also follow it to use @disk_bytenr for the same
> purpose.
>
> So that every time we see the name @disk_bytenr, we know it can be used
> to locate a data extent, without any need for weird offset calculation.
>
> That's why I'm strongly against adding any offset into @disk_bytenr.
> And I believe that's the biggest difference in our points of view.
>
> Although in this particular case, I can use some extra prefixs like
> "orig_" or "fe_" (for file extent), so that those members can be later
> directly passed to create_io_em() without extra offset calculation.
>
> Would that be a acceptable trade-off?
>
>
> Another solution would be just drop this patch, and do extra calulation
> resulting something like this:
>
> create_io_em(...,
> disk_bytenr - whatever_offset, /* disk_bytenr */
> offset - whatever_offset, /* offset */
> PREALLOC, ...);
>
> At least that does not sound sane to me, and can be bug prune.
> You won't believe how many different crashes I hit just due to the weird
> disk_bytenr calculation here, and that's the biggest reason I have
the extra sanity checks.
>
> Thanks,
> Qu
>
next prev parent reply other threads:[~2024-04-12 22:12 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-08 22:33 [PATCH 0/8] btrfs: extent-map: use disk_bytenr/offset to replace block_start/block_len/orig_start Qu Wenruo
2024-04-08 22:33 ` [PATCH RFC 1/8] btrfs: rename extent_map::orig_block_len to disk_num_bytes Qu Wenruo
2024-04-09 14:58 ` David Sterba
2024-04-09 21:38 ` Qu Wenruo
2024-04-08 22:33 ` [PATCH RFC 2/8] btrfs: rename members of can_nocow_file_extent_args Qu Wenruo
2024-04-11 14:46 ` Filipe Manana
2024-04-11 22:03 ` Qu Wenruo
2024-04-12 13:21 ` Filipe Manana
2024-04-12 22:00 ` Qu Wenruo
2024-04-12 22:12 ` Qu Wenruo [this message]
2024-04-08 22:33 ` [PATCH RFC 3/8] btrfs: introduce new members for extent_map Qu Wenruo
2024-04-11 14:56 ` Filipe Manana
2024-04-11 21:52 ` Qu Wenruo
2024-04-08 22:33 ` [PATCH RFC 4/8] btrfs: introduce extra sanity checks for extent maps Qu Wenruo
2024-04-08 22:33 ` [PATCH RFC 5/8] btrfs: remove extent_map::orig_start member Qu Wenruo
2024-04-09 14:59 ` David Sterba
2024-04-08 22:33 ` [PATCH RFC 6/8] btrfs: remove extent_map::block_len member Qu Wenruo
2024-04-08 22:33 ` [PATCH RFC 7/8] btrfs: remove extent_map::block_start member Qu Wenruo
2024-04-08 22:33 ` [PATCH RFC 8/8] btrfs: reorder disk_bytenr/disk_num_bytes/ram_bytes/offset parameters Qu Wenruo
2024-04-09 14:57 ` [PATCH 0/8] btrfs: extent-map: use disk_bytenr/offset to replace block_start/block_len/orig_start David Sterba
2024-04-09 21:40 ` Qu Wenruo
2024-04-09 22:18 ` David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=adb96734-6521-4c13-ae86-f70a5f8848fe@suse.com \
--to=wqu@suse.com \
--cc=fdmanana@kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=quwenruo.btrfs@gmx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).