Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[Qu Wenruo <wqu@suse.com>]

在 2024/12/19 07:15, Filipe Manana 写道:
> On Wed, Dec 18, 2024 at 8:21 PM David Sterba <dsterba@suse.cz> wrote:
>>
>> On Mon, Dec 16, 2024 at 05:17:17PM +0000, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> The ctree module is about the implementation of the btree data structure
>>> and not a place holder for generic filesystem things like the csum
>>> algorithm details. Move the functions related to the csum algorithm
>>> details away from ctree.c and into fs.c, which is a far better place for
>>> them. Also fix missing punctuation in comments and change one multiline
>>> comment to a single line comment since everything fits in under 80
>>> characters.
>>>
>>> For some reason this also sligthly reduces the module's size.
>>>
>>> Before this change:
>>>
>>>    $ size fs/btrfs/btrfs.ko
>>>       text        data     bss     dec     hex filename
>>>    1782126      161045   16920 1960091  1de89b fs/btrfs/btrfs.ko
>>>
>>> After this change:
>>>
>>>    $ size fs/btrfs/btrfs.ko
>>>       text        data     bss     dec     hex filename
>>>    1782094      161045   16920 1960059  1de87b fs/btrfs/btrfs.ko
>>>
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>   fs/btrfs/ctree.c | 51 ------------------------------------------------
>>>   fs/btrfs/ctree.h |  6 ------
>>>   fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   fs/btrfs/fs.h    |  6 ++++++
>>
>> Can you please create a new file for checksums? Moving everything to
>> fs.c looks like we're going to have another ctree.c.
> 
> Is it really worth it? After this patchset fs.c is only 229 lines and
> the csum related functions are just a few and very short.
> My idea would be to do such a thing either when fs.c gets a lot larger
> or we get more csum functions (and/or they get larger).
> 
> But sure, why not, I can do that on top or send a new version of this patch.

Personally speaking, I'm not a huge fan of a lot of small files/headers.

It makes the include path less clear, and make the path completion 
easier to hit conflicts.
(That's also why I hate the "mode-" prefix in btrfs-progs check/ directory)

The current fs.[ch] is definitely acceptable, thus I'm pretty happy with 
the current move.

Thanks,
Qu

> 
> Thanks.
>