Re: [PATCH 29/32] btrfs: scrub: introduce scrub_page::page_len for subpage support

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

[-- Attachment #1.1: Type: text/plain, Size: 3691 bytes --]



On 2020/11/10 上午2:17, David Sterba wrote:
> On Tue, Nov 03, 2020 at 09:31:05PM +0800, Qu Wenruo wrote:
>> --- a/fs/btrfs/scrub.c
>> +++ b/fs/btrfs/scrub.c
>> @@ -72,9 +72,15 @@ struct scrub_page {
>>  	u64			physical_for_dev_replace;
>>  	atomic_t		refs;
>>  	struct {
>> -		unsigned int	mirror_num:8;
>> -		unsigned int	have_csum:1;
>> -		unsigned int	io_error:1;
>> +		/*
>> +		 * For subpage case, where only part of the page is utilized
>> +		 * Note that 16 bits can only go 65535, not 65536, thus we have
>> +		 * to use 17 bits here.
>> +		 */
>> +		u32	page_len:17;
>> +		u32	mirror_num:8;
>> +		u32	have_csum:1;
>> +		u32	io_error:1;
>>  	};
> 
> The embedded struct is some relic so this can be cleaned up further.
> Mirror_num can become u8. The page length size is a bit awkward, 17 is
> the lowest number to contain the size up to 64k but there's still some
> space left so it can go up to 22 without increasing the structure size.
> 
> Source:
> 
> struct scrub_page {
>         struct scrub_block      *sblock;
>         struct page             *page;
>         struct btrfs_device     *dev;
>         struct list_head        list;
>         u64                     flags;  /* extent flags */
>         u64                     generation;
>         u64                     logical;
>         u64                     physical;
>         u64                     physical_for_dev_replace;
>         atomic_t                refs;
>         u8                      mirror_num;
>         /*
>          * For subpage case, where only part of the page is utilized Note that
>          * 16 bits can only go 65535, not 65536, thus we have to use 17 bits
>          * here.
>          */
>         u32     page_len:20;
>         u32     have_csum:1;
>         u32     io_error:1;
>         u8                      csum[BTRFS_CSUM_SIZE];
> 
>         struct scrub_recover    *recover;
> };
> 
> pahole:
> 
> struct scrub_page {
>         struct scrub_block *       sblock;               /*     0     8 */
>         struct page *              page;                 /*     8     8 */
>         struct btrfs_device *      dev;                  /*    16     8 */
>         struct list_head           list;                 /*    24    16 */
>         u64                        flags;                /*    40     8 */
>         u64                        generation;           /*    48     8 */
>         u64                        logical;              /*    56     8 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         u64                        physical;             /*    64     8 */
>         u64                        physical_for_dev_replace; /*    72     8 */
>         atomic_t                   refs;                 /*    80     4 */
>         u8                         mirror_num;           /*    84     1 */
> 
>         /* Bitfield combined with previous fields */
> 
>         u32                        page_len:20;          /*    84: 8  4 */
>         u32                        have_csum:1;          /*    84:28  4 */
>         u32                        io_error:1;           /*    84:29  4 */
> 
>         /* XXX 2 bits hole, try to pack */
> 
>         u8                         csum[32];             /*    88    32 */
>         struct scrub_recover *     recover;              /*   120     8 */
> 
>         /* size: 128, cachelines: 2, members: 16 */
>         /* sum members: 125 */
>         /* sum bitfield members: 22 bits, bit holes: 1, sum bit holes: 2 bits */
> };
> 
Thanks, looks indeed much better.

Would go this direction.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]