Re: [PATCH 01/10] btrfs: use precalculated sectorsize_bits from fs_info

[David Sterba <dsterba@suse.cz>]

On Mon, Nov 02, 2020 at 10:05:54PM +0800, Qu Wenruo wrote:
> On 2020/10/29 下午10:27, David Sterba wrote:
> > We do a lot of calculations where we divide or multiply by sectorsize.
> > We also know and make sure that sectorsize is a power of two, so this
> > means all divisions can be turned to shifts and avoid eg. expensive
> > u64/u32 divisions.
> > 
> > The type is u32 as it's more register friendly on x86_64 compared to u8
> > and the resulting assembly is smaller (movzbl vs movl).
> > 
> > There's also superblock s_blocksize_bits but it's usually one more
> > pointer dereference farther than fs_info.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/ctree.h           |  1 +
> >  fs/btrfs/disk-io.c         |  2 ++
> >  fs/btrfs/extent-tree.c     |  2 +-
> >  fs/btrfs/file-item.c       | 11 ++++++-----
> >  fs/btrfs/free-space-tree.c | 12 +++++++-----
> >  fs/btrfs/ordered-data.c    |  3 +--
> >  fs/btrfs/scrub.c           | 12 ++++++------
> >  fs/btrfs/tree-checker.c    |  3 ++-
> >  8 files changed, 26 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 8a83bce3225c..87c40cc5c42e 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -931,6 +931,7 @@ struct btrfs_fs_info {
> >  	/* Cached block sizes */
> >  	u32 nodesize;
> >  	u32 sectorsize;
> > +	u32 sectorsize_bits;
> 
> For the bit shift, it can alwasy be contained in one u8.
> Since one u32 is only to be at most 32 bits, it can be easily contained
> in u8 whose max value is 255.
> 
> This should allow us to pack several u8 together to reduce some memory
> usage.

As mentioned in the changelog, u32 generates slightly better assembly.
The size increase is minimal compared to overall fs_info size so I'd
become worried once we have a lot of u32 in place of u8 that would span
more cachelines than necessary. Reorganizing fs_info for better
cacheline access patterns would be interesting but at the moment we
leave it up to the CPU.