Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* Unnecessarily bad cache behavior for ext4_getattr()
@ 2019-11-25  0:19 Linus Torvalds
  2019-11-25 20:35 ` Andreas Dilger
  2019-11-30  0:49 ` Theodore Y. Ts'o
  0 siblings, 2 replies; 3+ messages in thread
From: Linus Torvalds @ 2019-11-25  0:19 UTC (permalink / raw)
  To: Theodore Ts'o, Andreas Dilger; +Cc: Ext4 Developers List

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

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?

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 623 bytes --]

 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;

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Unnecessarily bad cache behavior for ext4_getattr()
  2019-11-25  0:19 Unnecessarily bad cache behavior for ext4_getattr() Linus Torvalds
@ 2019-11-25 20:35 ` Andreas Dilger
  2019-11-30  0:49 ` Theodore Y. Ts'o
  1 sibling, 0 replies; 3+ messages in thread
From: Andreas Dilger @ 2019-11-25 20:35 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Theodore Ts'o, Ext4 Developers List

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

On Nov 24, 2019, at 5:19 PM, Linus Torvalds <torvalds@linux-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@dilger.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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Unnecessarily bad cache behavior for ext4_getattr()
  2019-11-25  0:19 Unnecessarily bad cache behavior for ext4_getattr() Linus Torvalds
  2019-11-25 20:35 ` Andreas Dilger
@ 2019-11-30  0:49 ` Theodore Y. Ts'o
  1 sibling, 0 replies; 3+ messages in thread
From: Theodore Y. Ts'o @ 2019-11-30  0:49 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Andreas Dilger, Ext4 Developers List

On Sun, Nov 24, 2019 at 04:19:16PM -0800, Linus Torvalds 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?

Looks good, thanks, I've applied it to the ext4 tree.

I'm a bit surprised a cache line miss rated that high on a kernel
build, but that probably says a lot about how efficient the rest of
the kernel was (and I assume Make didn't need to rebuild most of the
object files).

					- Ted

P.S.  Did you see the ext4 pull request?  I wasn't sure if you haven't
gotten to it yet due to being distracted by Turkey day or not...


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-25  0:19 Unnecessarily bad cache behavior for ext4_getattr() Linus Torvalds
2019-11-25 20:35 ` Andreas Dilger
2019-11-30  0:49 ` Theodore Y. Ts'o

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git