lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 25 Nov 2019 13:35:36 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Theodore Ts'o <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: Unnecessarily bad cache behavior for ext4_getattr()

On Nov 24, 2019, at 5:19 PM, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> It looks from profiles like ext4_getattr() is fairly expensive,
> because it unnecessarily accesses the extended inode information and
> causes extra cache misses.
> 
> On an empty kernel allmodconfig build (which is a lot of "stat()"
> calls by Make, and a lot of silly string stuff in user space due to
> all the make variable games we play), ext4_getattr() was something
> like 1% of the time according to the profile I gathered. It might be
> bogus - maybe the cacheline ends up being accessed later anyway, but
> it _looked_ like it was the whole "i_extra_isize" access that missed
> in the cache.
> 
> That's all for gathering the STATX_BTIME information, that the caller
> doesn't even *want*.
> 
> How about a patch like the attached?

I think that looks quite reasonable.  I was going to comment that the
nanosecond timestamps for [amc]time are also stored in the "extra_isize"
part of the inode, but in this callpath they are already stored in the
VFS inode and do not need to be extracted each time.

So I'd think your patch should be good, modulo 80-column line wrap.

Reviewed-by: Andreas Dilger <adilger@...ger.ca>


>  fs/ext4/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 516faa280ced..617dc8835f5f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5700,7 +5700,7 @@ int ext4_getattr(const struct path *path, struct kstat *stat,
>  	struct ext4_inode_info *ei = EXT4_I(inode);
>  	unsigned int flags;
> 
> -	if (EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
> +	if ((query_flags & STATX_BTIME) && EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
>  		stat->result_mask |= STATX_BTIME;
>  		stat->btime.tv_sec = ei->i_crtime.tv_sec;
>  		stat->btime.tv_nsec = ei->i_crtime.tv_nsec;

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists