Re: [PATCH 1/2] btrfs: introduce btrfs_subpage_bitmap_info

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/8/16 下午5:28, Nikolay Borisov wrote:
>
>
[...]
>>
>>   	if (sectorsize != PAGE_SIZE) {
>> +		struct btrfs_subpage_info *subpage_info;
>> +
>> +		ASSERT(sectorsize < PAGE_SIZE);
>
> nit: Simply make the check sectorsize < PAGE_SIZE and that renders the
> assert redundant.

I guess that's the way to go.

Since I don't believe we will support multi-page sectorsize anyway, we
don't need to bother sectorsize > PAGE_SIZE forever.
Thus we can forget the sectorsize > PAGE_SIZE case.

[...]
>> +
>> +	subpage_info->total_nr_bits = cur;
>
> So those values are really consts, however due to them being allocated
> on the heap you can't simply define them as const and initialize them.
> What a bummer...

Yeah, that's the limit of C.

>
>
> On the other hand the namings are a bit generic, those are really
> offsets into subpage->bitmaps. As such I'd prefer something along the
> lines of
> writeback_bitmap_offset
> ordered_bitmap_offset etc.

The *_bitmap_offset seems a little long, especially in the 2nd patch
we're already introducing too many new lines just to handle the *_start
naming.

Especially all these members are used with thing like
fs_info->subpage_info->uptodate_start.

What about the name "uptodate_offset"?

>
> Also I believe a graphical representation is in order i.e
>
>
> [u][u][u][u][e][e][e][e][e]
> ^			  ^
> |-uptodate_start	  |- error_start etc

That looks awesome.

>
> Since it's a bit unexpected to have multiple, logically independent
> bitmaps be tracked in the same physical location.

That's also I'm concerning of.

I haven't seen other code sides doing the same behavior.
IOmap just waste all the memory by going full u32 bitmap even for case
like 16K page size and 4K sectorsize, exactly I want to avoid.

Thanks,
Qu

>> +}
>> +
>>   int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
>>   			 struct page *page, enum btrfs_subpage_type type)
>>   {
>> diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
>> index 9aa40d795ba9..ea90ba42c97b 100644
>> --- a/fs/btrfs/subpage.h
>> +++ b/fs/btrfs/subpage.h
>> @@ -11,6 +11,32 @@
>>    */
>>   #define BTRFS_SUBPAGE_BITMAP_SIZE	16
>>
>> +/*
>> + * Extra info for subpapge bitmap.
>> + *
>> + * For subpage we integrate all uptodate/error/dirty/writeback/ordered
>> + * bitmaps into one larger bitmap.
>> + * This structure records the basic info.
>> + */
>> +struct btrfs_subpage_info {
>> +	/* Number of bits for each bitmap*/
>> +	unsigned int bitmap_nr_bits;
>> +
>> +	/* Total number of bits for the whole bitmap */
>> +	unsigned int total_nr_bits;
>> +
>> +	/*
>> +	 * *_start indicates where the bitmap starts, the length
>> +	 * is always @bitmap_size, which is calculated from
>> +	 * PAGE_SIZE / sectorsize.
>> +	 */
>> +	unsigned int uptodate_start;
>> +	unsigned int error_start;
>> +	unsigned int dirty_start;
>> +	unsigned int writeback_start;
>> +	unsigned int ordered_start;
>> +};
>> +
>>   /*
>>    * Structure to trace status of each sector inside a page, attached to
>>    * page::private for both data and metadata inodes.
>> @@ -53,6 +79,8 @@ enum btrfs_subpage_type {
>>   	BTRFS_SUBPAGE_DATA,
>>   };
>>
>> +void btrfs_init_subpage_info(struct btrfs_subpage_info *subpage_info,
>> +			     u32 sectorsize);
>>   int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
>>   			 struct page *page, enum btrfs_subpage_type type);
>>   void btrfs_detach_subpage(const struct btrfs_fs_info *fs_info,
>>