[PATCH] [RFC] xfs: initialise attr fork on inode create

[PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When we allocate a new inode, we often need to add an attribute to
the inode as part of the create. This can happen as a result of
needing to add default ACLs or security labels before the inode is
made visible to userspace.

This is highly inefficient right now. We do the create transaction
to allocate the inode, then we do an "add attr fork" transaction to
modify the just created empty inode to set the inode fork offset to
allow attributes to be stored, then we go and do the attribute
creation.

This means 3 transactions instead of 1 to allocate an inode, and
this greatly increases the load on the CIL commit code, resulting in
excessive contention on the CIL spin locks and performance
degradation:

 18.99%  [kernel]                [k] __pv_queued_spin_lock_slowpath
  3.57%  [kernel]                [k] do_raw_spin_lock
  2.51%  [kernel]                [k] __raw_callee_save___pv_queued_spin_unlock
  2.48%  [kernel]                [k] memcpy
  2.34%  [kernel]                [k] xfs_log_commit_cil

The typical profile resulting from running fsmark on a selinux enabled
filesytem is adds this overhead to the create path:

  - 15.30% xfs_init_security
     - 15.23% security_inode_init_security
	- 13.05% xfs_initxattrs
	   - 12.94% xfs_attr_set
	      - 6.75% xfs_bmap_add_attrfork
		 - 5.51% xfs_trans_commit
		    - 5.48% __xfs_trans_commit
		       - 5.35% xfs_log_commit_cil
			  - 3.86% _raw_spin_lock
			     - do_raw_spin_lock
				  __pv_queued_spin_lock_slowpath
		 - 0.70% xfs_trans_alloc
		      0.52% xfs_trans_reserve
	      - 5.41% xfs_attr_set_args
		 - 5.39% xfs_attr_set_shortform.constprop.0
		    - 4.46% xfs_trans_commit
		       - 4.46% __xfs_trans_commit
			  - 4.33% xfs_log_commit_cil
			     - 2.74% _raw_spin_lock
				- do_raw_spin_lock
				     __pv_queued_spin_lock_slowpath
			       0.60% xfs_inode_item_format
		      0.90% xfs_attr_try_sf_addname
	- 1.99% selinux_inode_init_security
	   - 1.02% security_sid_to_context_force
	      - 1.00% security_sid_to_context_core
		 - 0.92% sidtab_entry_to_string
		    - 0.90% sidtab_sid2str_get
			 0.59% sidtab_sid2str_put.part.0
	   - 0.82% selinux_determine_inode_label
	      - 0.77% security_transition_sid
		   0.70% security_compute_sid.part.0

And fsmark creation rate performance drops by ~25%. The key point to
note here is that half the additional overhead comes from adding the
attribute fork to the newly created inode. That's crazy, considering
we can do this same thing at inode create time with a couple of
lines of code and no extra overhead.

So, if we know we are going to add an attribute immediately after
creating the inode, let's just initialise the attribute fork inside
the create transaction and chop that whole chunk of code out of
the create fast path. This completely removes the performance
drop caused by enabling SELinux, and the profile looks like:

     - 8.99% xfs_init_security
         - 9.00% security_inode_init_security
            - 6.43% xfs_initxattrs
               - 6.37% xfs_attr_set
                  - 5.45% xfs_attr_set_args
                     - 5.42% xfs_attr_set_shortform.constprop.0
                        - 4.51% xfs_trans_commit
                           - 4.54% __xfs_trans_commit
                              - 4.59% xfs_log_commit_cil
                                 - 2.67% _raw_spin_lock
                                    - 3.28% do_raw_spin_lock
                                         3.08% __pv_queued_spin_lock_slowpath
                                   0.66% xfs_inode_item_format
                        - 0.90% xfs_attr_try_sf_addname
                  - 0.60% xfs_trans_alloc
            - 2.35% selinux_inode_init_security
               - 1.25% security_sid_to_context_force
                  - 1.21% security_sid_to_context_core
                     - 1.19% sidtab_entry_to_string
                        - 1.20% sidtab_sid2str_get
                           - 0.86% sidtab_sid2str_put.part.0
                              - 0.62% _raw_spin_lock_irqsave
                                 - 0.77% do_raw_spin_lock
                                      __pv_queued_spin_lock_slowpath
               - 0.84% selinux_determine_inode_label
                  - 0.83% security_transition_sid
                       0.86% security_compute_sid.part.0

Which indicates the XFS overhead of creating the selinux xattr has
been halved. This doesn't fix the CIL lock contention problem, just
means it's not a limiting factor for this workload. Lock contention
in the security subsystems is going to be an issue soon, though...

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
 fs/xfs/libxfs/xfs_inode_fork.h |  1 +
 fs/xfs/xfs_inode.c             | 24 ++++++++++++++++++++----
 fs/xfs/xfs_inode.h             |  5 +++--
 fs/xfs/xfs_iops.c              | 10 +++++++++-
 fs/xfs/xfs_qm.c                |  2 +-
 fs/xfs/xfs_symlink.c           |  2 +-
 7 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 7575de5cecb1..5d5b466bd787 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -280,6 +280,19 @@ xfs_dfork_attr_shortform_size(
 	return be16_to_cpu(atp->hdr.totsize);
 }
 
+struct xfs_ifork *
+xfs_ifork_alloc(
+	int8_t			format,
+	xfs_extnum_t		nextents)
+{
+	struct xfs_ifork	*ifp;
+
+	ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
+	ifp->if_format = format;
+	ifp->if_nextents = nextents;
+	return ifp;
+}
+
 int
 xfs_iformat_attr_fork(
 	struct xfs_inode	*ip,
@@ -291,11 +304,8 @@ xfs_iformat_attr_fork(
 	 * Initialize the extent count early, as the per-format routines may
 	 * depend on it.
 	 */
-	ip->i_afp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
-	ip->i_afp->if_format = dip->di_aformat;
-	if (unlikely(ip->i_afp->if_format == 0)) /* pre IRIX 6.2 file system */
-		ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
-	ip->i_afp->if_nextents = be16_to_cpu(dip->di_anextents);
+	ip->i_afp = xfs_ifork_alloc(dip->di_aformat,
+				be16_to_cpu(dip->di_anextents));
 
 	switch (ip->i_afp->if_format) {
 	case XFS_DINODE_FMT_LOCAL:
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index a4953e95c4f3..3ad088c6a9d2 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -80,6 +80,7 @@ static inline int8_t xfs_ifork_format(struct xfs_ifork *ifp)
 	return ifp->if_format;
 }
 
+struct xfs_ifork *xfs_ifork_alloc(int8_t format, xfs_extnum_t nextents);
 struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
 
 int		xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -800,6 +800,7 @@ xfs_ialloc(
 	dev_t		rdev,
 	prid_t		prid,
 	xfs_buf_t	**ialloc_context,
+	bool		init_attrs,
 	xfs_inode_t	**ipp)
 {
 	struct xfs_mount *mp = tp->t_mountp;
@@ -918,6 +919,18 @@ xfs_ialloc(
 		ASSERT(0);
 	}
 
+	/*
+	 * If we need to create attributes immediately after allocating the
+	 * inode, initialise an empty attribute fork right now. We use the
+	 * default fork offset for attributes here as we don't know exactly what
+	 * size or how many attributes we might be adding. We can do this safely
+	 * here because we know the data fork is completely empty right now.
+	 */
+	if (init_attrs) {
+		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
+		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
+	}
+
 	/*
 	 * Log the new values stuffed into the inode.
 	 */
@@ -951,6 +964,7 @@ xfs_dir_ialloc(
 	xfs_nlink_t	nlink,
 	dev_t		rdev,
 	prid_t		prid,		/* project id */
+	bool		init_attrs,
 	xfs_inode_t	**ipp)		/* pointer to inode; it will be
 					   locked. */
 {
@@ -980,7 +994,7 @@ xfs_dir_ialloc(
 	 * the inode(s) that we've just allocated.
 	 */
 	code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
-			&ip);
+			init_attrs, &ip);
 
 	/*
 	 * Return an error if we were unable to allocate a new inode.
@@ -1050,7 +1064,7 @@ xfs_dir_ialloc(
 		 * this call should always succeed.
 		 */
 		code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
-				  &ialloc_context, &ip);
+				  &ialloc_context, init_attrs, &ip);
 
 		/*
 		 * If we get an error at this point, return to the caller
@@ -1112,6 +1126,7 @@ xfs_create(
 	struct xfs_name		*name,
 	umode_t			mode,
 	dev_t			rdev,
+	bool			init_attrs,
 	xfs_inode_t		**ipp)
 {
 	int			is_dir = S_ISDIR(mode);
@@ -1182,7 +1197,8 @@ xfs_create(
 	 * entry pointing to them, but a directory also the "." entry
 	 * pointing to itself.
 	 */
-	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip);
+	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid,
+				init_attrs, &ip);
 	if (error)
 		goto out_trans_cancel;
 
@@ -1304,7 +1320,7 @@ xfs_create_tmpfile(
 	if (error)
 		goto out_trans_cancel;
 
-	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, &ip);
+	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, false, &ip);
 	if (error)
 		goto out_trans_cancel;
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 751a3d1d7d84..670dfab334de 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -370,7 +370,8 @@ void		xfs_inactive(struct xfs_inode *ip);
 int		xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
 			   struct xfs_inode **ipp, struct xfs_name *ci_name);
 int		xfs_create(struct xfs_inode *dp, struct xfs_name *name,
-			   umode_t mode, dev_t rdev, struct xfs_inode **ipp);
+			   umode_t mode, dev_t rdev, bool need_xattr,
+			   struct xfs_inode **ipp);
 int		xfs_create_tmpfile(struct xfs_inode *dp, umode_t mode,
 			   struct xfs_inode **ipp);
 int		xfs_remove(struct xfs_inode *dp, struct xfs_name *name,
@@ -408,7 +409,7 @@ xfs_extlen_t	xfs_get_extsz_hint(struct xfs_inode *ip);
 xfs_extlen_t	xfs_get_cowextsz_hint(struct xfs_inode *ip);
 
 int		xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
-			       xfs_nlink_t, dev_t, prid_t,
+			       xfs_nlink_t, dev_t, prid_t, bool need_xattr,
 			       struct xfs_inode **);
 
 static inline int
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 1414ab79eacf..75b44b82ad1f 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -126,6 +126,7 @@ xfs_cleanup_inode(
 	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
 }
 
+
 STATIC int
 xfs_generic_create(
 	struct inode	*dir,
@@ -161,7 +162,14 @@ xfs_generic_create(
 		goto out_free_acl;
 
 	if (!tmpfile) {
-		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
+		bool need_xattr = false;
+
+		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
+		    default_acl || acl)
+			need_xattr = true;
+
+		error = xfs_create(XFS_I(dir), &name, mode, rdev,
+					need_xattr, &ip);
 	} else {
 		error = xfs_create_tmpfile(XFS_I(dir), mode, &ip);
 	}
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index b2a9abee8b2b..45faddfb4234 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -787,7 +787,7 @@ xfs_qm_qino_alloc(
 		return error;
 
 	if (need_alloc) {
-		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip);
+		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, false, ip);
 		if (error) {
 			xfs_trans_cancel(tp);
 			return error;
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 8e88a7ca387e..20919aaea981 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -224,7 +224,7 @@ xfs_symlink(
 	 * Allocate an inode for the symlink.
 	 */
 	error = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0,
-			       prid, &ip);
+			       prid, false, &ip);
 	if (error)
 		goto out_trans_cancel;
 
-- 
2.28.0

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Christoph Hellwig]

This looks pretty sensible, and pretty simple.  Why the RFC?

This looks good to me modulo a few tiny nitpicks below:

> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 1414ab79eacf..75b44b82ad1f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -126,6 +126,7 @@ xfs_cleanup_inode(
>  	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
>  }
>  
> +
>  STATIC int
>  xfs_generic_create(
>  	struct inode	*dir,

Nit: this adds a spuurious empty line.

> @@ -161,7 +162,14 @@ xfs_generic_create(
>  		goto out_free_acl;
>  
>  	if (!tmpfile) {
> -		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
> +		bool need_xattr = false;
> +
> +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
> +		    default_acl || acl)
> +			need_xattr = true;
> +
> +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
> +					need_xattr, &ip);

It might be wort to factor the condition into a little helper.  Also
I think we also have security labels for O_TMPFILE inodes, so it might
be worth plugging into that path as well.

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner]

On Thu, Dec 03, 2020 at 08:40:12AM +0000, Christoph Hellwig wrote:
> This looks pretty sensible, and pretty simple.  Why the RFC?
> 
> This looks good to me modulo a few tiny nitpicks below:
> 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 1414ab79eacf..75b44b82ad1f 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -126,6 +126,7 @@ xfs_cleanup_inode(
> >  	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
> >  }
> >  
> > +
> >  STATIC int
> >  xfs_generic_create(
> >  	struct inode	*dir,
> 
> Nit: this adds a spuurious empty line.

Fixed.

> > @@ -161,7 +162,14 @@ xfs_generic_create(
> >  		goto out_free_acl;
> >  
> >  	if (!tmpfile) {
> > -		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
> > +		bool need_xattr = false;
> > +
> > +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
> > +		    default_acl || acl)
> > +			need_xattr = true;
> > +
> > +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
> > +					need_xattr, &ip);
> 
> It might be wort to factor the condition into a little helper.  Also
> I think we also have security labels for O_TMPFILE inodes, so it might
> be worth plugging into that path as well.

Yeah, a helper is a good idea - I just wanted to get some feedback
first on whether it's a good idea to peek directly at
i_sb->s_security or whether there is some other way of knowing ahead
of time that a security xattr is going to be created. I couldn't
find one, but that doesn't mean such an interface doesn't exist in
all the twisty passages of the LSM layers...

You didn't shout and run screaming, so that's a positive :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Christoph Hellwig]

On Fri, Dec 04, 2020 at 08:44:26AM +1100, Dave Chinner wrote:
> > > +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
> > > +		    default_acl || acl)
> > > +			need_xattr = true;
> > > +
> > > +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
> > > +					need_xattr, &ip);
> > 
> > It might be wort to factor the condition into a little helper.  Also
> > I think we also have security labels for O_TMPFILE inodes, so it might
> > be worth plugging into that path as well.
> 
> Yeah, a helper is a good idea - I just wanted to get some feedback
> first on whether it's a good idea to peek directly at
> i_sb->s_security or whether there is some other way of knowing ahead
> of time that a security xattr is going to be created. I couldn't
> find one, but that doesn't mean such an interface doesn't exist in
> all the twisty passages of the LSM layers...

I've added the relevant list, maybe someone there has an opinion.

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Casey Schaufler]

On 12/3/2020 11:54 PM, Christoph Hellwig wrote:
> On Fri, Dec 04, 2020 at 08:44:26AM +1100, Dave Chinner wrote:
>>>> +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
>>>> +		    default_acl || acl)
>>>> +			need_xattr = true;
>>>> +
>>>> +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
>>>> +					need_xattr, &ip);
>>> It might be wort to factor the condition into a little helper.  Also
>>> I think we also have security labels for O_TMPFILE inodes, so it might
>>> be worth plugging into that path as well.
>> Yeah, a helper is a good idea - I just wanted to get some feedback
>> first on whether it's a good idea to peek directly at
>> i_sb->s_security

Only security modules should ever look at what's in the security blob.
In fact, you can't assume that the presence of a security blob
(i.e. ...->s_security != NULL) implies "need_xattr", or any other
state for the superblock.

>>  or whether there is some other way of knowing ahead
>> of time that a security xattr is going to be created. I couldn't
>> find one, but that doesn't mean such an interface doesn't exist in
>> all the twisty passages of the LSM layers...
> I've added the relevant list, maybe someone there has an opinion.

How is what you're looking for different from security_ismaclabel() ?

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Christoph Hellwig]

On Mon, Dec 07, 2020 at 09:22:13AM -0800, Casey Schaufler wrote:
> Only security modules should ever look at what's in the security blob.
> In fact, you can't assume that the presence of a security blob
> (i.e. ...->s_security != NULL) implies "need_xattr", or any other
> state for the superblock.

Maybe "strongly suggests that an xattr will be added" is the better
wording.

> 
> >>  or whether there is some other way of knowing ahead
> >> of time that a security xattr is going to be created. I couldn't
> >> find one, but that doesn't mean such an interface doesn't exist in
> >> all the twisty passages of the LSM layers...
> > I've added the relevant list, maybe someone there has an opinion.
> 
> How is what you're looking for different from security_ismaclabel() ?

Not at all.  What this needs is a guestimate (which doesn't have
to be 100% reliable) that a new inode created by ->create, ->mknod,
or ->mkdir will have an xattr set on it during the creation syscall.

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner]

On Mon, Dec 07, 2020 at 05:25:45PM +0000, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 09:22:13AM -0800, Casey Schaufler wrote:
> > Only security modules should ever look at what's in the security blob.
> > In fact, you can't assume that the presence of a security blob
> > (i.e. ...->s_security != NULL) implies "need_xattr", or any other
> > state for the superblock.
> 
> Maybe "strongly suggests that an xattr will be added" is the better
> wording.

Right, I did this knowing that only selinux and smack actually use
sb->s_security so it's not 100% reliable. However, these are also
the only two security modules that hook inode_init_security and
create xattrs.

So it seems like peeking at ->s_security here gives us a fairly
reliable indicator that we're going to have to create xattrs on this
new inode before we complete the create process...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Brian Foster]

On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When we allocate a new inode, we often need to add an attribute to
> the inode as part of the create. This can happen as a result of
> needing to add default ACLs or security labels before the inode is
> made visible to userspace.
> 
> This is highly inefficient right now. We do the create transaction
> to allocate the inode, then we do an "add attr fork" transaction to
> modify the just created empty inode to set the inode fork offset to
> allow attributes to be stored, then we go and do the attribute
> creation.
> 
> This means 3 transactions instead of 1 to allocate an inode, and
> this greatly increases the load on the CIL commit code, resulting in
> excessive contention on the CIL spin locks and performance
> degradation:
> 
>  18.99%  [kernel]                [k] __pv_queued_spin_lock_slowpath
>   3.57%  [kernel]                [k] do_raw_spin_lock
>   2.51%  [kernel]                [k] __raw_callee_save___pv_queued_spin_unlock
>   2.48%  [kernel]                [k] memcpy
>   2.34%  [kernel]                [k] xfs_log_commit_cil
> 
> The typical profile resulting from running fsmark on a selinux enabled
> filesytem is adds this overhead to the create path:
> 
...
> 
> And fsmark creation rate performance drops by ~25%. The key point to
> note here is that half the additional overhead comes from adding the
> attribute fork to the newly created inode. That's crazy, considering
> we can do this same thing at inode create time with a couple of
> lines of code and no extra overhead.
> 
> So, if we know we are going to add an attribute immediately after
> creating the inode, let's just initialise the attribute fork inside
> the create transaction and chop that whole chunk of code out of
> the create fast path. This completely removes the performance
> drop caused by enabling SELinux, and the profile looks like:
> 
...
> 
> Which indicates the XFS overhead of creating the selinux xattr has
> been halved. This doesn't fix the CIL lock contention problem, just
> means it's not a limiting factor for this workload. Lock contention
> in the security subsystems is going to be an issue soon, though...
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
>  fs/xfs/libxfs/xfs_inode_fork.h |  1 +
>  fs/xfs/xfs_inode.c             | 24 ++++++++++++++++++++----
>  fs/xfs/xfs_inode.h             |  5 +++--
>  fs/xfs/xfs_iops.c              | 10 +++++++++-
>  fs/xfs/xfs_qm.c                |  2 +-
>  fs/xfs/xfs_symlink.c           |  2 +-
>  7 files changed, 50 insertions(+), 14 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
...
> @@ -918,6 +919,18 @@ xfs_ialloc(
>  		ASSERT(0);
>  	}
>  
> +	/*
> +	 * If we need to create attributes immediately after allocating the
> +	 * inode, initialise an empty attribute fork right now. We use the
> +	 * default fork offset for attributes here as we don't know exactly what
> +	 * size or how many attributes we might be adding. We can do this safely
> +	 * here because we know the data fork is completely empty right now.
> +	 */
> +	if (init_attrs) {
> +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> +	}
> +

Seems reasonable in principle, but why not refactor
xfs_bmap_add_attrfork() such that the internals (i.e. everything within
the transaction/ilock code) can be properly reused in both contexts
rather than open-coding (and thus duplicating) a somewhat stripped down
version? At a glance, it looks like there are some subtle differences in
the initial setup of the attr fork for a device node inode, for example.

Brian

>  	/*
>  	 * Log the new values stuffed into the inode.
>  	 */
> @@ -951,6 +964,7 @@ xfs_dir_ialloc(
>  	xfs_nlink_t	nlink,
>  	dev_t		rdev,
>  	prid_t		prid,		/* project id */
> +	bool		init_attrs,
>  	xfs_inode_t	**ipp)		/* pointer to inode; it will be
>  					   locked. */
>  {
> @@ -980,7 +994,7 @@ xfs_dir_ialloc(
>  	 * the inode(s) that we've just allocated.
>  	 */
>  	code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
> -			&ip);
> +			init_attrs, &ip);
>  
>  	/*
>  	 * Return an error if we were unable to allocate a new inode.
> @@ -1050,7 +1064,7 @@ xfs_dir_ialloc(
>  		 * this call should always succeed.
>  		 */
>  		code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
> -				  &ialloc_context, &ip);
> +				  &ialloc_context, init_attrs, &ip);
>  
>  		/*
>  		 * If we get an error at this point, return to the caller
> @@ -1112,6 +1126,7 @@ xfs_create(
>  	struct xfs_name		*name,
>  	umode_t			mode,
>  	dev_t			rdev,
> +	bool			init_attrs,
>  	xfs_inode_t		**ipp)
>  {
>  	int			is_dir = S_ISDIR(mode);
> @@ -1182,7 +1197,8 @@ xfs_create(
>  	 * entry pointing to them, but a directory also the "." entry
>  	 * pointing to itself.
>  	 */
> -	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip);
> +	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid,
> +				init_attrs, &ip);
>  	if (error)
>  		goto out_trans_cancel;
>  
> @@ -1304,7 +1320,7 @@ xfs_create_tmpfile(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, &ip);
> +	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, false, &ip);
>  	if (error)
>  		goto out_trans_cancel;
>  
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 751a3d1d7d84..670dfab334de 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -370,7 +370,8 @@ void		xfs_inactive(struct xfs_inode *ip);
>  int		xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
>  			   struct xfs_inode **ipp, struct xfs_name *ci_name);
>  int		xfs_create(struct xfs_inode *dp, struct xfs_name *name,
> -			   umode_t mode, dev_t rdev, struct xfs_inode **ipp);
> +			   umode_t mode, dev_t rdev, bool need_xattr,
> +			   struct xfs_inode **ipp);
>  int		xfs_create_tmpfile(struct xfs_inode *dp, umode_t mode,
>  			   struct xfs_inode **ipp);
>  int		xfs_remove(struct xfs_inode *dp, struct xfs_name *name,
> @@ -408,7 +409,7 @@ xfs_extlen_t	xfs_get_extsz_hint(struct xfs_inode *ip);
>  xfs_extlen_t	xfs_get_cowextsz_hint(struct xfs_inode *ip);
>  
>  int		xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
> -			       xfs_nlink_t, dev_t, prid_t,
> +			       xfs_nlink_t, dev_t, prid_t, bool need_xattr,
>  			       struct xfs_inode **);
>  
>  static inline int
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 1414ab79eacf..75b44b82ad1f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -126,6 +126,7 @@ xfs_cleanup_inode(
>  	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
>  }
>  
> +
>  STATIC int
>  xfs_generic_create(
>  	struct inode	*dir,
> @@ -161,7 +162,14 @@ xfs_generic_create(
>  		goto out_free_acl;
>  
>  	if (!tmpfile) {
> -		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
> +		bool need_xattr = false;
> +
> +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
> +		    default_acl || acl)
> +			need_xattr = true;
> +
> +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
> +					need_xattr, &ip);
>  	} else {
>  		error = xfs_create_tmpfile(XFS_I(dir), mode, &ip);
>  	}
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index b2a9abee8b2b..45faddfb4234 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -787,7 +787,7 @@ xfs_qm_qino_alloc(
>  		return error;
>  
>  	if (need_alloc) {
> -		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip);
> +		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, false, ip);
>  		if (error) {
>  			xfs_trans_cancel(tp);
>  			return error;
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 8e88a7ca387e..20919aaea981 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -224,7 +224,7 @@ xfs_symlink(
>  	 * Allocate an inode for the symlink.
>  	 */
>  	error = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0,
> -			       prid, &ip);
> +			       prid, false, &ip);
>  	if (error)
>  		goto out_trans_cancel;
>  
> -- 
> 2.28.0
> 

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner]

On Fri, Dec 04, 2020 at 07:31:37AM -0500, Brian Foster wrote:
> On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When we allocate a new inode, we often need to add an attribute to
> > the inode as part of the create. This can happen as a result of
> > needing to add default ACLs or security labels before the inode is
> > made visible to userspace.
> > 
> > This is highly inefficient right now. We do the create transaction
> > to allocate the inode, then we do an "add attr fork" transaction to
> > modify the just created empty inode to set the inode fork offset to
> > allow attributes to be stored, then we go and do the attribute
> > creation.
> > 
> > This means 3 transactions instead of 1 to allocate an inode, and
> > this greatly increases the load on the CIL commit code, resulting in
> > excessive contention on the CIL spin locks and performance
> > degradation:
> > 
> >  18.99%  [kernel]                [k] __pv_queued_spin_lock_slowpath
> >   3.57%  [kernel]                [k] do_raw_spin_lock
> >   2.51%  [kernel]                [k] __raw_callee_save___pv_queued_spin_unlock
> >   2.48%  [kernel]                [k] memcpy
> >   2.34%  [kernel]                [k] xfs_log_commit_cil
> > 
> > The typical profile resulting from running fsmark on a selinux enabled
> > filesytem is adds this overhead to the create path:
> > 
> ...
> > 
> > And fsmark creation rate performance drops by ~25%. The key point to
> > note here is that half the additional overhead comes from adding the
> > attribute fork to the newly created inode. That's crazy, considering
> > we can do this same thing at inode create time with a couple of
> > lines of code and no extra overhead.
> > 
> > So, if we know we are going to add an attribute immediately after
> > creating the inode, let's just initialise the attribute fork inside
> > the create transaction and chop that whole chunk of code out of
> > the create fast path. This completely removes the performance
> > drop caused by enabling SELinux, and the profile looks like:
> > 
> ...
> > 
> > Which indicates the XFS overhead of creating the selinux xattr has
> > been halved. This doesn't fix the CIL lock contention problem, just
> > means it's not a limiting factor for this workload. Lock contention
> > in the security subsystems is going to be an issue soon, though...
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
> >  fs/xfs/libxfs/xfs_inode_fork.h |  1 +
> >  fs/xfs/xfs_inode.c             | 24 ++++++++++++++++++++----
> >  fs/xfs/xfs_inode.h             |  5 +++--
> >  fs/xfs/xfs_iops.c              | 10 +++++++++-
> >  fs/xfs/xfs_qm.c                |  2 +-
> >  fs/xfs/xfs_symlink.c           |  2 +-
> >  7 files changed, 50 insertions(+), 14 deletions(-)
> > 
> ...
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> ...
> > @@ -918,6 +919,18 @@ xfs_ialloc(
> >  		ASSERT(0);
> >  	}
> >  
> > +	/*
> > +	 * If we need to create attributes immediately after allocating the
> > +	 * inode, initialise an empty attribute fork right now. We use the
> > +	 * default fork offset for attributes here as we don't know exactly what
> > +	 * size or how many attributes we might be adding. We can do this safely
> > +	 * here because we know the data fork is completely empty right now.
> > +	 */
> > +	if (init_attrs) {
> > +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> > +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > +	}
> > +
> 
> Seems reasonable in principle, but why not refactor
> xfs_bmap_add_attrfork() such that the internals (i.e. everything within
> the transaction/ilock code) can be properly reused in both contexts
> rather than open-coding (and thus duplicating) a somewhat stripped down
> version?

We don't know the size of the attribute that is being created, so
the attr size dependent parts of it can't be used.

> At a glance, it looks like there are some subtle differences in
> the initial setup of the attr fork for a device node inode, for example.

Yes, there's a difference, but it's largely irrelevant as adding
the first attribute to a device format inode will reset the
forkoffset to the min via xfs_attr_shortform_bytesfit().

And if the attribute is larger than will fit in the default fork
offset space, but can fit the attr in shrotform by shrinking the
empty data fork space, xfs_attr_shortform_bytesfit() will do that as
well. IOWs, we only need to set a non-zero fork offset here and init
the ip->i_afp pointer - immediately setting an attribute on the
empty inode literal area will do the rest for the fork offset setup
for us...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Brian Foster]

On Sat, Dec 05, 2020 at 08:22:22AM +1100, Dave Chinner wrote:
> On Fri, Dec 04, 2020 at 07:31:37AM -0500, Brian Foster wrote:
> > On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > When we allocate a new inode, we often need to add an attribute to
> > > the inode as part of the create. This can happen as a result of
> > > needing to add default ACLs or security labels before the inode is
> > > made visible to userspace.
> > > 
> > > This is highly inefficient right now. We do the create transaction
> > > to allocate the inode, then we do an "add attr fork" transaction to
> > > modify the just created empty inode to set the inode fork offset to
> > > allow attributes to be stored, then we go and do the attribute
> > > creation.
> > > 
> > > This means 3 transactions instead of 1 to allocate an inode, and
> > > this greatly increases the load on the CIL commit code, resulting in
> > > excessive contention on the CIL spin locks and performance
> > > degradation:
> > > 
> > >  18.99%  [kernel]                [k] __pv_queued_spin_lock_slowpath
> > >   3.57%  [kernel]                [k] do_raw_spin_lock
> > >   2.51%  [kernel]                [k] __raw_callee_save___pv_queued_spin_unlock
> > >   2.48%  [kernel]                [k] memcpy
> > >   2.34%  [kernel]                [k] xfs_log_commit_cil
> > > 
> > > The typical profile resulting from running fsmark on a selinux enabled
> > > filesytem is adds this overhead to the create path:
> > > 
> > ...
> > > 
> > > And fsmark creation rate performance drops by ~25%. The key point to
> > > note here is that half the additional overhead comes from adding the
> > > attribute fork to the newly created inode. That's crazy, considering
> > > we can do this same thing at inode create time with a couple of
> > > lines of code and no extra overhead.
> > > 
> > > So, if we know we are going to add an attribute immediately after
> > > creating the inode, let's just initialise the attribute fork inside
> > > the create transaction and chop that whole chunk of code out of
> > > the create fast path. This completely removes the performance
> > > drop caused by enabling SELinux, and the profile looks like:
> > > 
> > ...
> > > 
> > > Which indicates the XFS overhead of creating the selinux xattr has
> > > been halved. This doesn't fix the CIL lock contention problem, just
> > > means it's not a limiting factor for this workload. Lock contention
> > > in the security subsystems is going to be an issue soon, though...
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
> > >  fs/xfs/libxfs/xfs_inode_fork.h |  1 +
> > >  fs/xfs/xfs_inode.c             | 24 ++++++++++++++++++++----
> > >  fs/xfs/xfs_inode.h             |  5 +++--
> > >  fs/xfs/xfs_iops.c              | 10 +++++++++-
> > >  fs/xfs/xfs_qm.c                |  2 +-
> > >  fs/xfs/xfs_symlink.c           |  2 +-
> > >  7 files changed, 50 insertions(+), 14 deletions(-)
> > > 
> > ...
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > ...
> > > @@ -918,6 +919,18 @@ xfs_ialloc(
> > >  		ASSERT(0);
> > >  	}
> > >  
> > > +	/*
> > > +	 * If we need to create attributes immediately after allocating the
> > > +	 * inode, initialise an empty attribute fork right now. We use the
> > > +	 * default fork offset for attributes here as we don't know exactly what
> > > +	 * size or how many attributes we might be adding. We can do this safely
> > > +	 * here because we know the data fork is completely empty right now.
> > > +	 */
> > > +	if (init_attrs) {
> > > +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> > > +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > > +	}
> > > +
> > 
> > Seems reasonable in principle, but why not refactor
> > xfs_bmap_add_attrfork() such that the internals (i.e. everything within
> > the transaction/ilock code) can be properly reused in both contexts
> > rather than open-coding (and thus duplicating) a somewhat stripped down
> > version?
> 
> We don't know the size of the attribute that is being created, so
> the attr size dependent parts of it can't be used.
> 

Not sure I see the problem here. It looks to me that
xfs_bmap_add_attrfork() would do the right thing if we just passed a
size of zero. The only place the size value is actually used is down in
xfs_attr_shortform_bytesfit(), and I'd expect that to identify that the
requested size is <= than the current afork size (also zero for a newly
allocated inode..?) and bail out.

That said, I wouldn't be opposed to tweaking xfs_bmap_set_attrforkoff()
by a line or two to just skip the shortform call if size == 0. Then we
can be more explicit about the "size == 0 means preemptive fork alloc,
use the default offset" use case and perhaps actually document it with
some comments as well.

Brian

> > At a glance, it looks like there are some subtle differences in
> > the initial setup of the attr fork for a device node inode, for example.
> 
> Yes, there's a difference, but it's largely irrelevant as adding
> the first attribute to a device format inode will reset the
> forkoffset to the min via xfs_attr_shortform_bytesfit().
> 
> And if the attribute is larger than will fit in the default fork
> offset space, but can fit the attr in shrotform by shrinking the
> empty data fork space, xfs_attr_shortform_bytesfit() will do that as
> well. IOWs, we only need to set a non-zero fork offset here and init
> the ip->i_afp pointer - immediately setting an attribute on the
> empty inode literal area will do the rest for the fork offset setup
> for us...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner]

On Sat, Dec 05, 2020 at 06:34:44AM -0500, Brian Foster wrote:
> On Sat, Dec 05, 2020 at 08:22:22AM +1100, Dave Chinner wrote:
> > On Fri, Dec 04, 2020 at 07:31:37AM -0500, Brian Foster wrote:
> > > On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > ...
> > > > @@ -918,6 +919,18 @@ xfs_ialloc(
> > > >  		ASSERT(0);
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * If we need to create attributes immediately after allocating the
> > > > +	 * inode, initialise an empty attribute fork right now. We use the
> > > > +	 * default fork offset for attributes here as we don't know exactly what
> > > > +	 * size or how many attributes we might be adding. We can do this safely
> > > > +	 * here because we know the data fork is completely empty right now.
> > > > +	 */
> > > > +	if (init_attrs) {
> > > > +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> > > > +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > > > +	}
> > > > +
> > > 
> > > Seems reasonable in principle, but why not refactor
> > > xfs_bmap_add_attrfork() such that the internals (i.e. everything within
> > > the transaction/ilock code) can be properly reused in both contexts
> > > rather than open-coding (and thus duplicating) a somewhat stripped down
> > > version?
> > 
> > We don't know the size of the attribute that is being created, so
> > the attr size dependent parts of it can't be used.
> 
> Not sure I see the problem here. It looks to me that
> xfs_bmap_add_attrfork() would do the right thing if we just passed a
> size of zero.

Yes, but it also does an awful lot that we do not need.

> The only place the size value is actually used is down in
> xfs_attr_shortform_bytesfit(), and I'd expect that to identify that the
> requested size is <= than the current afork size (also zero for a newly
> allocated inode..?) and bail out.

RIght it ends up doing that because an uninitialised inode fork
(di_forkoff = 0) is the same size as the requested size of zero, and
then it does ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;

But that's decided another two function calls deep, after a lot of
branches and shifting and comparisons to determine that the attr
fork is empty. Yet we already know that the attr fork is empty here
so all that extra CPU work is completely unnecessary.

Keep in mind we do exactly the same thing in
xfs_bmap_forkoff_reset(). We don't care about all the setup stuff in
xfs_bmap_add_attrfork(), we just reset the attr fork offset to the
default if the attr fork had grown larger than the default offset.

> That said, I wouldn't be opposed to tweaking xfs_bmap_set_attrforkoff()
> by a line or two to just skip the shortform call if size == 0. Then we
> can be more explicit about the "size == 0 means preemptive fork alloc,
> use the default offset" use case and perhaps actually document it with
> some comments as well.

It just seems wrong to me to code a special case into some function
to optimise that special case when the code that needs the special
case has no need to call that function in the first place.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Brian Foster]

On Mon, Dec 07, 2020 at 10:33:22AM +1100, Dave Chinner wrote:
> On Sat, Dec 05, 2020 at 06:34:44AM -0500, Brian Foster wrote:
> > On Sat, Dec 05, 2020 at 08:22:22AM +1100, Dave Chinner wrote:
> > > On Fri, Dec 04, 2020 at 07:31:37AM -0500, Brian Foster wrote:
> > > > On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > > index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> > > > > --- a/fs/xfs/xfs_inode.c
> > > > > +++ b/fs/xfs/xfs_inode.c
> > > > ...
> > > > > @@ -918,6 +919,18 @@ xfs_ialloc(
> > > > >  		ASSERT(0);
> > > > >  	}
> > > > >  
> > > > > +	/*
> > > > > +	 * If we need to create attributes immediately after allocating the
> > > > > +	 * inode, initialise an empty attribute fork right now. We use the
> > > > > +	 * default fork offset for attributes here as we don't know exactly what
> > > > > +	 * size or how many attributes we might be adding. We can do this safely
> > > > > +	 * here because we know the data fork is completely empty right now.
> > > > > +	 */
> > > > > +	if (init_attrs) {
> > > > > +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> > > > > +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > > > > +	}
> > > > > +
> > > > 
> > > > Seems reasonable in principle, but why not refactor
> > > > xfs_bmap_add_attrfork() such that the internals (i.e. everything within
> > > > the transaction/ilock code) can be properly reused in both contexts
> > > > rather than open-coding (and thus duplicating) a somewhat stripped down
> > > > version?
> > > 
> > > We don't know the size of the attribute that is being created, so
> > > the attr size dependent parts of it can't be used.
> > 
> > Not sure I see the problem here. It looks to me that
> > xfs_bmap_add_attrfork() would do the right thing if we just passed a
> > size of zero.
> 
> Yes, but it also does an awful lot that we do not need.
> 

Hence the suggestion to refactor it..

> > The only place the size value is actually used is down in
> > xfs_attr_shortform_bytesfit(), and I'd expect that to identify that the
> > requested size is <= than the current afork size (also zero for a newly
> > allocated inode..?) and bail out.
> 
> RIght it ends up doing that because an uninitialised inode fork
> (di_forkoff = 0) is the same size as the requested size of zero, and
> then it does ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> 
> But that's decided another two function calls deep, after a lot of
> branches and shifting and comparisons to determine that the attr
> fork is empty. Yet we already know that the attr fork is empty here
> so all that extra CPU work is completely unnecessary.
> 

xfs_bmap_add_attrfork() already asserts that the attr fork is
nonexistent at the very top of the function, for one. The 25-30 lines of
that function that we need can be trivially lifted out into a new helper
that can equally as trivially accommodate the size == 0 case and skip
all those shortform calculations.

> Keep in mind we do exactly the same thing in
> xfs_bmap_forkoff_reset(). We don't care about all the setup stuff in
> xfs_bmap_add_attrfork(), we just reset the attr fork offset to the
> default if the attr fork had grown larger than the default offset.
> 

I'm not arguing that the attr fork needs to be set up in a particular
way on initial creation. I'm arguing that we don't need yet a third
unique code path to set/initialize a default/empty attr fork. We can
slowly normalize them all to the _reset() technique you're effectively
reimplementing here if that works well enough and is preferred...

> > That said, I wouldn't be opposed to tweaking xfs_bmap_set_attrforkoff()
> > by a line or two to just skip the shortform call if size == 0. Then we
> > can be more explicit about the "size == 0 means preemptive fork alloc,
> > use the default offset" use case and perhaps actually document it with
> > some comments as well.
> 
> It just seems wrong to me to code a special case into some function
> to optimise that special case when the code that needs the special
> case has no need to call that function in the first place.....
> 

I'm not sure what's so odd or controversial about refactoring and
reusing an existing operational (i.e. add fork) function to facilitate
review and future maintenance of that particular operation being
performed from new and various contexts. And speaking in generalities
like this just obfuscates and overcomplicates the argument. Let's be
clear, we're literally arguing over a delta that would look something
like this:

xfs_bmap_set_attrforkoff()
{
...
+		if (size)
-               ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
+			ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
                if (!ip->i_d.di_forkoff)
                        ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
		...
}

Given all of that, I'm not convinced this is nearly the problem you seem
to insinuate, yet I also don't think I'll convince you otherwise so it's
probably not worth continuing to debate. You have my feedback, I'll let
others determine how this patch comes together from here...

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Darrick J. Wong]

On Mon, Dec 07, 2020 at 11:30:18AM -0500, Brian Foster wrote:
> On Mon, Dec 07, 2020 at 10:33:22AM +1100, Dave Chinner wrote:
> > On Sat, Dec 05, 2020 at 06:34:44AM -0500, Brian Foster wrote:
> > > On Sat, Dec 05, 2020 at 08:22:22AM +1100, Dave Chinner wrote:
> > > > On Fri, Dec 04, 2020 at 07:31:37AM -0500, Brian Foster wrote:
> > > > > On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> > > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > > > index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> > > > > > --- a/fs/xfs/xfs_inode.c
> > > > > > +++ b/fs/xfs/xfs_inode.c
> > > > > ...
> > > > > > @@ -918,6 +919,18 @@ xfs_ialloc(
> > > > > >  		ASSERT(0);
> > > > > >  	}
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * If we need to create attributes immediately after allocating the
> > > > > > +	 * inode, initialise an empty attribute fork right now. We use the
> > > > > > +	 * default fork offset for attributes here as we don't know exactly what
> > > > > > +	 * size or how many attributes we might be adding. We can do this safely
> > > > > > +	 * here because we know the data fork is completely empty right now.
> > > > > > +	 */
> > > > > > +	if (init_attrs) {
> > > > > > +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> > > > > > +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > > > > > +	}
> > > > > > +
> > > > > 
> > > > > Seems reasonable in principle, but why not refactor
> > > > > xfs_bmap_add_attrfork() such that the internals (i.e. everything within
> > > > > the transaction/ilock code) can be properly reused in both contexts
> > > > > rather than open-coding (and thus duplicating) a somewhat stripped down
> > > > > version?
> > > > 
> > > > We don't know the size of the attribute that is being created, so
> > > > the attr size dependent parts of it can't be used.
> > > 
> > > Not sure I see the problem here. It looks to me that
> > > xfs_bmap_add_attrfork() would do the right thing if we just passed a
> > > size of zero.
> > 
> > Yes, but it also does an awful lot that we do not need.
> > 
> 
> Hence the suggestion to refactor it..
> 
> > > The only place the size value is actually used is down in
> > > xfs_attr_shortform_bytesfit(), and I'd expect that to identify that the
> > > requested size is <= than the current afork size (also zero for a newly
> > > allocated inode..?) and bail out.
> > 
> > RIght it ends up doing that because an uninitialised inode fork
> > (di_forkoff = 0) is the same size as the requested size of zero, and
> > then it does ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > 
> > But that's decided another two function calls deep, after a lot of
> > branches and shifting and comparisons to determine that the attr
> > fork is empty. Yet we already know that the attr fork is empty here
> > so all that extra CPU work is completely unnecessary.
> > 
> 
> xfs_bmap_add_attrfork() already asserts that the attr fork is
> nonexistent at the very top of the function, for one. The 25-30 lines of
> that function that we need can be trivially lifted out into a new helper
> that can equally as trivially accommodate the size == 0 case and skip
> all those shortform calculations.
> 
> > Keep in mind we do exactly the same thing in
> > xfs_bmap_forkoff_reset(). We don't care about all the setup stuff in
> > xfs_bmap_add_attrfork(), we just reset the attr fork offset to the
> > default if the attr fork had grown larger than the default offset.
> > 
> 
> I'm not arguing that the attr fork needs to be set up in a particular
> way on initial creation. I'm arguing that we don't need yet a third
> unique code path to set/initialize a default/empty attr fork. We can
> slowly normalize them all to the _reset() technique you're effectively
> reimplementing here if that works well enough and is preferred...
> 
> > > That said, I wouldn't be opposed to tweaking xfs_bmap_set_attrforkoff()
> > > by a line or two to just skip the shortform call if size == 0. Then we
> > > can be more explicit about the "size == 0 means preemptive fork alloc,
> > > use the default offset" use case and perhaps actually document it with
> > > some comments as well.
> > 
> > It just seems wrong to me to code a special case into some function
> > to optimise that special case when the code that needs the special
> > case has no need to call that function in the first place.....
> > 
> 
> I'm not sure what's so odd or controversial about refactoring and
> reusing an existing operational (i.e. add fork) function to facilitate
> review and future maintenance of that particular operation being
> performed from new and various contexts. And speaking in generalities
> like this just obfuscates and overcomplicates the argument. Let's be
> clear, we're literally arguing over a delta that would look something
> like this:
> 
> xfs_bmap_set_attrforkoff()
> {
> ...
> +		if (size)
> -               ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
> +			ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
>                 if (!ip->i_d.di_forkoff)
>                         ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> 		...
> }
> 
> Given all of that, I'm not convinced this is nearly the problem you seem
> to insinuate, yet I also don't think I'll convince you otherwise so it's
> probably not worth continuing to debate. You have my feedback, I'll let
> others determine how this patch comes together from here...

Alternate take: If you /are/ going to have a "init empty attr fork"
shortcut function, can it at least be placed next to the regular one in
xfs_bmap.c so that the di_forkoff setters are only split across three
source files instead of four?

--D

> Brian
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
> 

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Darrick J. Wong]

On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When we allocate a new inode, we often need to add an attribute to
> the inode as part of the create. This can happen as a result of
> needing to add default ACLs or security labels before the inode is
> made visible to userspace.
> 
> This is highly inefficient right now. We do the create transaction
> to allocate the inode, then we do an "add attr fork" transaction to
> modify the just created empty inode to set the inode fork offset to
> allow attributes to be stored, then we go and do the attribute
> creation.
> 
> This means 3 transactions instead of 1 to allocate an inode, and
> this greatly increases the load on the CIL commit code, resulting in
> excessive contention on the CIL spin locks and performance
> degradation:
> 
>  18.99%  [kernel]                [k] __pv_queued_spin_lock_slowpath
>   3.57%  [kernel]                [k] do_raw_spin_lock
>   2.51%  [kernel]                [k] __raw_callee_save___pv_queued_spin_unlock
>   2.48%  [kernel]                [k] memcpy
>   2.34%  [kernel]                [k] xfs_log_commit_cil
> 
> The typical profile resulting from running fsmark on a selinux enabled
> filesytem is adds this overhead to the create path:
> 
>   - 15.30% xfs_init_security
>      - 15.23% security_inode_init_security
> 	- 13.05% xfs_initxattrs
> 	   - 12.94% xfs_attr_set
> 	      - 6.75% xfs_bmap_add_attrfork
> 		 - 5.51% xfs_trans_commit
> 		    - 5.48% __xfs_trans_commit
> 		       - 5.35% xfs_log_commit_cil
> 			  - 3.86% _raw_spin_lock
> 			     - do_raw_spin_lock
> 				  __pv_queued_spin_lock_slowpath
> 		 - 0.70% xfs_trans_alloc
> 		      0.52% xfs_trans_reserve
> 	      - 5.41% xfs_attr_set_args
> 		 - 5.39% xfs_attr_set_shortform.constprop.0
> 		    - 4.46% xfs_trans_commit
> 		       - 4.46% __xfs_trans_commit
> 			  - 4.33% xfs_log_commit_cil
> 			     - 2.74% _raw_spin_lock
> 				- do_raw_spin_lock
> 				     __pv_queued_spin_lock_slowpath
> 			       0.60% xfs_inode_item_format
> 		      0.90% xfs_attr_try_sf_addname
> 	- 1.99% selinux_inode_init_security
> 	   - 1.02% security_sid_to_context_force
> 	      - 1.00% security_sid_to_context_core
> 		 - 0.92% sidtab_entry_to_string
> 		    - 0.90% sidtab_sid2str_get
> 			 0.59% sidtab_sid2str_put.part.0
> 	   - 0.82% selinux_determine_inode_label
> 	      - 0.77% security_transition_sid
> 		   0.70% security_compute_sid.part.0
> 
> And fsmark creation rate performance drops by ~25%. The key point to
> note here is that half the additional overhead comes from adding the
> attribute fork to the newly created inode. That's crazy, considering
> we can do this same thing at inode create time with a couple of
> lines of code and no extra overhead.
> 
> So, if we know we are going to add an attribute immediately after
> creating the inode, let's just initialise the attribute fork inside
> the create transaction and chop that whole chunk of code out of
> the create fast path. This completely removes the performance
> drop caused by enabling SELinux, and the profile looks like:
> 
>      - 8.99% xfs_init_security
>          - 9.00% security_inode_init_security
>             - 6.43% xfs_initxattrs
>                - 6.37% xfs_attr_set
>                   - 5.45% xfs_attr_set_args
>                      - 5.42% xfs_attr_set_shortform.constprop.0
>                         - 4.51% xfs_trans_commit
>                            - 4.54% __xfs_trans_commit
>                               - 4.59% xfs_log_commit_cil
>                                  - 2.67% _raw_spin_lock
>                                     - 3.28% do_raw_spin_lock
>                                          3.08% __pv_queued_spin_lock_slowpath
>                                    0.66% xfs_inode_item_format
>                         - 0.90% xfs_attr_try_sf_addname
>                   - 0.60% xfs_trans_alloc
>             - 2.35% selinux_inode_init_security
>                - 1.25% security_sid_to_context_force
>                   - 1.21% security_sid_to_context_core
>                      - 1.19% sidtab_entry_to_string
>                         - 1.20% sidtab_sid2str_get
>                            - 0.86% sidtab_sid2str_put.part.0
>                               - 0.62% _raw_spin_lock_irqsave
>                                  - 0.77% do_raw_spin_lock
>                                       __pv_queued_spin_lock_slowpath
>                - 0.84% selinux_determine_inode_label
>                   - 0.83% security_transition_sid
>                        0.86% security_compute_sid.part.0
> 
> Which indicates the XFS overhead of creating the selinux xattr has
> been halved. This doesn't fix the CIL lock contention problem, just
> means it's not a limiting factor for this workload. Lock contention
> in the security subsystems is going to be an issue soon, though...
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
>  fs/xfs/libxfs/xfs_inode_fork.h |  1 +
>  fs/xfs/xfs_inode.c             | 24 ++++++++++++++++++++----
>  fs/xfs/xfs_inode.h             |  5 +++--
>  fs/xfs/xfs_iops.c              | 10 +++++++++-
>  fs/xfs/xfs_qm.c                |  2 +-
>  fs/xfs/xfs_symlink.c           |  2 +-
>  7 files changed, 50 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 7575de5cecb1..5d5b466bd787 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -280,6 +280,19 @@ xfs_dfork_attr_shortform_size(
>  	return be16_to_cpu(atp->hdr.totsize);
>  }
>  
> +struct xfs_ifork *
> +xfs_ifork_alloc(
> +	int8_t			format,

enum xfs_dinode_fmt	format, ?

> +	xfs_extnum_t		nextents)
> +{
> +	struct xfs_ifork	*ifp;
> +
> +	ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
> +	ifp->if_format = format;
> +	ifp->if_nextents = nextents;
> +	return ifp;
> +}
> +
>  int
>  xfs_iformat_attr_fork(
>  	struct xfs_inode	*ip,
> @@ -291,11 +304,8 @@ xfs_iformat_attr_fork(
>  	 * Initialize the extent count early, as the per-format routines may
>  	 * depend on it.
>  	 */
> -	ip->i_afp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
> -	ip->i_afp->if_format = dip->di_aformat;
> -	if (unlikely(ip->i_afp->if_format == 0)) /* pre IRIX 6.2 file system */
> -		ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;

Can we still mount an irix 6.1 filesystem?  I would guess not, but
removing old cruft is a separate patch.  Granted you did say RFC...

> -	ip->i_afp->if_nextents = be16_to_cpu(dip->di_anextents);
> +	ip->i_afp = xfs_ifork_alloc(dip->di_aformat,
> +				be16_to_cpu(dip->di_anextents));
>  
>  	switch (ip->i_afp->if_format) {
>  	case XFS_DINODE_FMT_LOCAL:
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index a4953e95c4f3..3ad088c6a9d2 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -80,6 +80,7 @@ static inline int8_t xfs_ifork_format(struct xfs_ifork *ifp)
>  	return ifp->if_format;
>  }
>  
> +struct xfs_ifork *xfs_ifork_alloc(int8_t format, xfs_extnum_t nextents);
>  struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
>  
>  int		xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -800,6 +800,7 @@ xfs_ialloc(
>  	dev_t		rdev,
>  	prid_t		prid,
>  	xfs_buf_t	**ialloc_context,
> +	bool		init_attrs,

init_xattrs ?  Just to distinguish them from regular file attrs like
mode/size/owner/etc.

>  	xfs_inode_t	**ipp)
>  {
>  	struct xfs_mount *mp = tp->t_mountp;
> @@ -918,6 +919,18 @@ xfs_ialloc(
>  		ASSERT(0);
>  	}
>  
> +	/*
> +	 * If we need to create attributes immediately after allocating the
> +	 * inode, initialise an empty attribute fork right now. We use the
> +	 * default fork offset for attributes here as we don't know exactly what
> +	 * size or how many attributes we might be adding. We can do this safely
> +	 * here because we know the data fork is completely empty right now.
> +	 */
> +	if (init_attrs) {
> +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> +	}
> +
>  	/*
>  	 * Log the new values stuffed into the inode.
>  	 */
> @@ -951,6 +964,7 @@ xfs_dir_ialloc(
>  	xfs_nlink_t	nlink,
>  	dev_t		rdev,
>  	prid_t		prid,		/* project id */
> +	bool		init_attrs,
>  	xfs_inode_t	**ipp)		/* pointer to inode; it will be
>  					   locked. */
>  {
> @@ -980,7 +994,7 @@ xfs_dir_ialloc(
>  	 * the inode(s) that we've just allocated.
>  	 */
>  	code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
> -			&ip);
> +			init_attrs, &ip);
>  
>  	/*
>  	 * Return an error if we were unable to allocate a new inode.
> @@ -1050,7 +1064,7 @@ xfs_dir_ialloc(
>  		 * this call should always succeed.
>  		 */
>  		code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
> -				  &ialloc_context, &ip);
> +				  &ialloc_context, init_attrs, &ip);
>  
>  		/*
>  		 * If we get an error at this point, return to the caller
> @@ -1112,6 +1126,7 @@ xfs_create(
>  	struct xfs_name		*name,
>  	umode_t			mode,
>  	dev_t			rdev,
> +	bool			init_attrs,
>  	xfs_inode_t		**ipp)
>  {
>  	int			is_dir = S_ISDIR(mode);
> @@ -1182,7 +1197,8 @@ xfs_create(
>  	 * entry pointing to them, but a directory also the "." entry
>  	 * pointing to itself.
>  	 */
> -	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip);
> +	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid,
> +				init_attrs, &ip);
>  	if (error)
>  		goto out_trans_cancel;
>  
> @@ -1304,7 +1320,7 @@ xfs_create_tmpfile(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, &ip);
> +	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, false, &ip);
>  	if (error)
>  		goto out_trans_cancel;
>  
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 751a3d1d7d84..670dfab334de 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -370,7 +370,8 @@ void		xfs_inactive(struct xfs_inode *ip);
>  int		xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
>  			   struct xfs_inode **ipp, struct xfs_name *ci_name);
>  int		xfs_create(struct xfs_inode *dp, struct xfs_name *name,
> -			   umode_t mode, dev_t rdev, struct xfs_inode **ipp);
> +			   umode_t mode, dev_t rdev, bool need_xattr,
> +			   struct xfs_inode **ipp);
>  int		xfs_create_tmpfile(struct xfs_inode *dp, umode_t mode,
>  			   struct xfs_inode **ipp);
>  int		xfs_remove(struct xfs_inode *dp, struct xfs_name *name,
> @@ -408,7 +409,7 @@ xfs_extlen_t	xfs_get_extsz_hint(struct xfs_inode *ip);
>  xfs_extlen_t	xfs_get_cowextsz_hint(struct xfs_inode *ip);
>  
>  int		xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
> -			       xfs_nlink_t, dev_t, prid_t,
> +			       xfs_nlink_t, dev_t, prid_t, bool need_xattr,
>  			       struct xfs_inode **);
>  
>  static inline int
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 1414ab79eacf..75b44b82ad1f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -126,6 +126,7 @@ xfs_cleanup_inode(
>  	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
>  }
>  
> +
>  STATIC int
>  xfs_generic_create(
>  	struct inode	*dir,
> @@ -161,7 +162,14 @@ xfs_generic_create(
>  		goto out_free_acl;
>  
>  	if (!tmpfile) {
> -		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
> +		bool need_xattr = false;
> +
> +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
> +		    default_acl || acl)
> +			need_xattr = true;

This should be a separate predicate in case the logic gets more twisty
later.

> +
> +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
> +					need_xattr, &ip);
>  	} else {
>  		error = xfs_create_tmpfile(XFS_I(dir), mode, &ip);

/me bets O_TMPFILE files can have xattr-based security labels applied to
them too, right?

>  	}
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index b2a9abee8b2b..45faddfb4234 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -787,7 +787,7 @@ xfs_qm_qino_alloc(
>  		return error;
>  
>  	if (need_alloc) {
> -		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip);
> +		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, false, ip);
>  		if (error) {
>  			xfs_trans_cancel(tp);
>  			return error;
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 8e88a7ca387e..20919aaea981 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -224,7 +224,7 @@ xfs_symlink(
>  	 * Allocate an inode for the symlink.
>  	 */
>  	error = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0,
> -			       prid, &ip);
> +			       prid, false, &ip);
>  	if (error)
>  		goto out_trans_cancel;
>  
> -- 
> 2.28.0
> 

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Christoph Hellwig]

Btw, while looking at the code before replying to Casey I noticed
something else in this area of code which we should probably fix
if we touch all this.  We are really supposed to create the ACLs
and security labels atomically with the actual inode creation.  And
I think we have all the infrastructure to do this without too much
pain now for ACLs.  Security labels with the weird
security_inode_init_security interface might be a little harder but
not impossible.

And I suspect security_inode_init_security might be right thing
to reuse for the helper to figure out what attrs would be set.  If
security_inode_init_security with an idempotent callback is
idempotent itself we might be able to use it directly, but all the
weird hooking makes it rather hard to read.

Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner]

On Mon, Dec 07, 2020 at 05:31:11PM +0000, Christoph Hellwig wrote:
> Btw, while looking at the code before replying to Casey I noticed
> something else in this area of code which we should probably fix
> if we touch all this.  We are really supposed to create the ACLs
> and security labels atomically with the actual inode creation.  And
> I think we have all the infrastructure to do this without too much
> pain now for ACLs.  Security labels with the weird
> security_inode_init_security interface might be a little harder but
> not impossible.

Yes, that's long been a known problem - it was a problem way back in
the day for DMF and the DMAPI attributes that needed to be added at
create time. That's where XFS_TRANS_RESERVE originally came from, so
that ENOSPC didn't prevent the dmapi xattr from being added to a
newly created inode.

This atomicity problem is one of the things that Allison's attribute
defer-op rework is intended to address.  i.e. being able to
atomically create xattrs at inode create time and have them fully
recoverable by intent replay if the initial inode allocation and
directory linkage transaction succeeds.

Essential the transaction context would start in
xfs_generic_create(), not xfs_create(), and roll through to the
xattr creations for ACLs and security contexts...

> And I suspect security_inode_init_security might be right thing
> to reuse for the helper to figure out what attrs would be set.

Problem is that it appears to need an inode to already be allocated
and instantiated to work....

> If
> security_inode_init_security with an idempotent callback is
> idempotent itself we might be able to use it directly, but all the
> weird hooking makes it rather hard to read.

Yeah, it's like a layer of inscrutible obscurity has been added to a
simple ops table... :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com