xfs: allocate xattr buffer on demand

From: Dave Chinner <[email protected]>

When doing file lookups and checking for permissions, we end up in xfs_get_acl() to see if there are any ACLs on the inode. This requires and xattr lookup, and to do that we have to supply a buffer large enough to hold an maximum sized xattr.

On workloads were we are accessing a wide range of cache cold files under memory pressure (e.g. NFS fileservers) we end up spending a lot of time allocating the buffer. The buffer is 64k in length, so is a contiguous multi-page allocation, and if that then fails we fall back to vmalloc(). Hence the allocation here is /expensive/ when we are looking up hundreds of thousands of files a second.

Initial numbers from a bpf trace show average time in xfs_get_acl() is ~32us, with ~19us of that in the memory allocation. Note these are average times, so there are going to be affected by the worst case allocations more than the common fast case...

To avoid this, we could just do a "null" lookup to see if the ACL xattr exists and then only do the allocation if it exists. This, however, optimises the path for the "no ACL present" case at the expense of the "acl present" case. i.e. we can halve the time in xfs_get_acl() for the no acl case (i.e down to ~10-15us), but that then increases the ACL case by 30% (i.e. up to 40-45us).

To solve this and speed up both cases, drive the xattr buffer allocation into the attribute code once we know what the actual xattr length is. For the no-xattr case, we avoid the allocation completely, speeding up that case. For the common ACL case, we'll end up with a fast heap allocation (because it'll be smaller than a page), and only for the rarer "we have a remote xattr" will we have a multi-page allocation occur. Hence the common ACL case will be much faster, too.

The down side of this is the buffer allocation is now a GFP_NOFS allocation. This isn't a big deal for small allocations, though it might cause large xattrs (which are out of line and doing substantial GFP_NOFS allocations already) to have to do another large allocation in this context. I think, however, this is rare enough that it won't matter for the ACL path, and the common xattr paths can still provide and external buffer...

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c      | 23 ++++------
 fs/xfs/libxfs/xfs_attr.h      |  6 ++-
 fs/xfs/libxfs/xfs_attr_leaf.c | 85 +++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_da_btree.h  |  4 +-
 fs/xfs/xfs_acl.c              | 14 ++----
 fs/xfs/xfs_ioctl.c            |  2 +-
 fs/xfs/xfs_xattr.c            |  2 +-
 7 files changed, 72 insertions(+), 64 deletions(-)

fs/xfs/xfs_acl.c +0 -0

index cbda40d40326..cba1ea9d6d0a 100644

fs/xfs/xfs_acl.c +0 -3

@@ -130,17 +130,9 @@ xfs_get_acl(struct inode *inode, int type)
 		BUG();
 	}
 
-	/*
-	 * If we have a cached ACLs value just return it, not need to
-	 * go out to the disk.

fs/xfs/xfs_acl.c +0 -0

 	if (error) {
 		/*
 		 * If the attribute doesn't exist make sure we have a negative

fs/xfs/xfs_ioctl.c +1 -1

index 6f7848cd5527..5f73feb40384 100644
@@ -438,7 +438,7 @@ xfs_attrmulti_attr_get(
 	if (!kbuf)
 		return -ENOMEM;
 
-	error = xfs_attr_get(XFS_I(inode), name, kbuf, (int *)len, flags);
+	error = xfs_attr_get(XFS_I(inode), name, &kbuf, (int *)len, flags);
 	if (error)
 		goto out_kfree;
 

fs/xfs/xfs_xattr.c +1 -1

index 3123b5aaad2a..cb895b1df5e4 100644
@@ -30,7 +30,7 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
 		value = NULL;
 	}
 
-	error = xfs_attr_get(ip, (unsigned char *)name, value, &asize, xflags);
+	error = xfs_attr_get(ip, name, (unsigned char **)&value, &asize, xflags);
 	if (error)
 		return error;
 	return asize;

Signature · 1 line

2.22.0

Quoted 35 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When doing file lookups and checking for permissions, we end up in xfs_get_acl() to see if there are any ACLs on the inode. This requires and xattr lookup, and to do that we have to supply a buffer large enough to hold an maximum sized xattr.

On workloads were we are accessing a wide range of cache cold files under memory pressure (e.g. NFS fileservers) we end up spending a lot of time allocating the buffer. The buffer is 64k in length, so is a contiguous multi-page allocation, and if that then fails we fall back to vmalloc(). Hence the allocation here is /expensive/ when we are looking up hundreds of thousands of files a second.

Initial numbers from a bpf trace show average time in xfs_get_acl() is ~32us, with ~19us of that in the memory allocation. Note these are average times, so there are going to be affected by the worst case allocations more than the common fast case...

To avoid this, we could just do a "null" lookup to see if the ACL xattr exists and then only do the allocation if it exists. This, however, optimises the path for the "no ACL present" case at the expense of the "acl present" case. i.e. we can halve the time in xfs_get_acl() for the no acl case (i.e down to ~10-15us), but that then increases the ACL case by 30% (i.e. up to 40-45us).

To solve this and speed up both cases, drive the xattr buffer allocation into the attribute code once we know what the actual xattr length is. For the no-xattr case, we avoid the allocation completely, speeding up that case. For the common ACL case, we'll end up with a fast heap allocation (because it'll be smaller than a page), and only for the rarer "we have a remote xattr" will we have a multi-page allocation occur. Hence the common ACL case will be much faster, too.

Quoted 8 lines

The down side of this is the buffer allocation is now a GFP_NOFS allocation. This isn't a big deal for small allocations, though it might cause large xattrs (which are out of line and doing substantial GFP_NOFS allocations already) to have to do another large allocation in this context. I think, however, this is rare enough that it won't matter for the ACL path, and the common xattr paths can still provide and external buffer...

Where's the NOFS allocation coming from?

Quoted 12 lines

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c      | 23 ++++------
 fs/xfs/libxfs/xfs_attr.h      |  6 ++-
 fs/xfs/libxfs/xfs_attr_leaf.c | 85 +++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_da_btree.h  |  4 +-
 fs/xfs/xfs_acl.c              | 14 ++----
 fs/xfs/xfs_ioctl.c            |  2 +-
 fs/xfs/xfs_xattr.c            |  2 +-
 7 files changed, 72 insertions(+), 64 deletions(-)

...

Quoted 4 lines

fs/xfs/xfs_acl.c +0 -0

index cbda40d40326..cba1ea9d6d0a 100644

...

Quoted 13 lines

fs/xfs/xfs_acl.c +0 -8

@@ -130,17 +130,9 @@ xfs_get_acl(struct inode *inode, int type)
 		BUG();
 	}
 
-	/*
-	 * If we have a cached ACLs value just return it, not need to
-	 * go out to the disk.
-	 */
 	len = XFS_ACL_MAX_SIZE(ip->i_mount);
-	xfs_acl = kmem_zalloc_large(len, KM_SLEEP);
-	if (!xfs_acl)
-		return ERR_PTR(-ENOMEM);
-

fs/xfs/xfs_acl.c xfs_get_acl(struct inode *inode, int type) · line 135

Quoted 4 lines

fs/xfs/xfs_acl.c +2 -2

-	error = xfs_attr_get(ip, ea_name, (unsigned char *)xfs_acl,
-							&len, ATTR_ROOT);
+	error = xfs_attr_get(ip, ea_name, (unsigned char **)&xfs_acl, &len,
+				ATTR_ALLOC|ATTR_ROOT);

This looks mostly fine to me aside from an interface nit. I find it strange that we need to specify a max value in len for an xattr of unknown length.

Even if we wanted to provide the caller the option to specify a max, I think we should be able to pass zero here and not fail with -ERANGE down in xfs_attr_copy_value().

Of course, we can and should
still check the on-disk value length (against XFS_XATTR_SIZE_MAX for example) to protect against overtly large allocation attempts if on-disk data happens to bogus.

Somewhat related on the interface topic... should we ever return an allocated buffer on error, or should the xattr layer unwind such operations before it returns?

Quoted 45 lines · Brian Foster

Quoted 43 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When doing file lookups and checking for permissions, we end up in xfs_get_acl() to see if there are any ACLs on the inode. This requires and xattr lookup, and to do that we have to supply a buffer large enough to hold an maximum sized xattr.

On workloads were we are accessing a wide range of cache cold files under memory pressure (e.g. NFS fileservers) we end up spending a lot of time allocating the buffer. The buffer is 64k in length, so is a contiguous multi-page allocation, and if that then fails we fall back to vmalloc(). Hence the allocation here is /expensive/ when we are looking up hundreds of thousands of files a second.

Initial numbers from a bpf trace show average time in xfs_get_acl() is ~32us, with ~19us of that in the memory allocation. Note these are average times, so there are going to be affected by the worst case allocations more than the common fast case...

To avoid this, we could just do a "null" lookup to see if the ACL xattr exists and then only do the allocation if it exists. This, however, optimises the path for the "no ACL present" case at the expense of the "acl present" case. i.e. we can halve the time in xfs_get_acl() for the no acl case (i.e down to ~10-15us), but that then increases the ACL case by 30% (i.e. up to 40-45us).

To solve this and speed up both cases, drive the xattr buffer allocation into the attribute code once we know what the actual xattr length is. For the no-xattr case, we avoid the allocation completely, speeding up that case. For the common ACL case, we'll end up with a fast heap allocation (because it'll be smaller than a page), and only for the rarer "we have a remote xattr" will we have a multi-page allocation occur. Hence the common ACL case will be much faster, too.

The down side of this is the buffer allocation is now a GFP_NOFS allocation. This isn't a big deal for small allocations, though it might cause large xattrs (which are out of line and doing substantial GFP_NOFS allocations already) to have to do another large allocation in this context. I think, however, this is rare enough that it won't matter for the ACL path, and the common xattr paths can still provide and external buffer...

Quoted 1 line

Where's the NOFS allocation coming from?

As I mentioned to Darrick on #xfs, this is a stale comment from development of the original prototype. When I reworked the code this problem went away, but I forgot to remove it from the commit message.

Quoted 31 lines

Quoted 12 lines

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c      | 23 ++++------
 fs/xfs/libxfs/xfs_attr.h      |  6 ++-
 fs/xfs/libxfs/xfs_attr_leaf.c | 85 +++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_da_btree.h  |  4 +-
 fs/xfs/xfs_acl.c              | 14 ++----
 fs/xfs/xfs_ioctl.c            |  2 +-
 fs/xfs/xfs_xattr.c            |  2 +-
 7 files changed, 72 insertions(+), 64 deletions(-)

...

Quoted 4 lines

fs/xfs/xfs_acl.c +0 -0

index cbda40d40326..cba1ea9d6d0a 100644

...

Quoted 13 lines

fs/xfs/xfs_acl.c +0 -8

@@ -130,17 +130,9 @@ xfs_get_acl(struct inode *inode, int type)
 		BUG();
 	}
 
-	/*
-	 * If we have a cached ACLs value just return it, not need to
-	 * go out to the disk.
-	 */
 	len = XFS_ACL_MAX_SIZE(ip->i_mount);
-	xfs_acl = kmem_zalloc_large(len, KM_SLEEP);
-	if (!xfs_acl)
-		return ERR_PTR(-ENOMEM);
-

Quoted 8 lines

fs/xfs/xfs_acl.c xfs_get_acl(struct inode *inode, int type) · line 135

Quoted 4 lines

fs/xfs/xfs_acl.c +2 -2

-	error = xfs_attr_get(ip, ea_name, (unsigned char *)xfs_acl,
-							&len, ATTR_ROOT);
+	error = xfs_attr_get(ip, ea_name, (unsigned char **)&xfs_acl, &len,
+				ATTR_ALLOC|ATTR_ROOT);

This looks mostly fine to me aside from an interface nit. I find it strange that we need to specify a max value in len for an xattr of unknown length.

That's a quirk of the on-disk format. i.e. v4 filesystems only have 25 ACLs at most, so if we find an xattr that contains 200 ACLs we've got an oversized xattr and it should return an error. The existing code will return a -ERANGE for this case, and the size of the buffer required would be returned in &len. This patch does not change that behaviour.

Capping the maximum length to what is valid for the given context also prevents corruption from triggering a massive allocation size that might cause other problems....

Quoted 3 lines

Even if we wanted to provide the caller the option to specify a max, I think we should be able to pass zero here and not fail with -ERANGE down in xfs_attr_copy_value().

We can change this behaviour if we even need such functionality. I don't see any point in changing API behaviour for things we don't actually need or use right now.

Quoted 4 lines

Of course, we can and should
still check the on-disk value length (against XFS_XATTR_SIZE_MAX for example) to protect against overtly large allocation attempts if on-disk data happens to bogus.

The max size of the xattr is context dependent, so given we have always passed this down into xfs_attr_get() for it to determine validity of the xattr that is found, I just don't see any reason to change it...

Quoted 3 lines

Somewhat related on the interface topic... should we ever return an allocated buffer on error, or should the xattr layer unwind such operations before it returns?

Errors can occur in different layers, having every layer know how to handle unwinding/freeing adds unnecessary complexity to the error handling. Having the top level code direct that the buffer must be allocated, and that if it was allocated it will free it regardless of success/failure greatly simplifies the rest of the code....

It largely makes no difference if it's handled by xfs_attr_get() of the callers of that function. The only difference it makes if we push it into xfs_attr_get() is that if we specify XFS_DA_OP_ALLOCVAL the we have to unconditionally free anything in args.value. i.e. it just pushes the same semantics in one layer...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]