Re: [PATCH v1 01/17] xfs: Add larp state XFS_DAS_CREATE_FORK

[Dave Chinner <david@fromorbit.com>]

On Sat, Jun 11, 2022 at 02:41:44AM -0700, Allison Henderson wrote:
> Recent parent pointer testing has exposed a bug in the underlying
> larp state machine.  A replace operation may remove an old attr
> before adding the new one, but if it is the only attr in the fork,
> then the fork is removed.  This later causes a null pointer in
> xfs_attr_try_sf_addname which expects the fork present.  This
> patch adds an extra state to create the fork.

Hmmmm.

I thought I fixed those problems - in xfs_attr_sf_removename() there
is this code:

        if (totsize == sizeof(xfs_attr_sf_hdr_t) && xfs_has_attr2(mp) &&
            (dp->i_df.if_format != XFS_DINODE_FMT_BTREE) &&
            !(args->op_flags & (XFS_DA_OP_ADDNAME | XFS_DA_OP_REPLACE))) {
                xfs_attr_fork_remove(dp, args->trans);

A replace operation will have XFS_DA_OP_REPLACE set, and so the
final remove from a sf directory will not remove the attr fork in
this case. There is equivalent checks in the leaf/node remove name
paths to avoid removing the attr fork if the last attr is removed
while the attr fork is in those formats.

How do you reproduce this issue?

> Additionally the new state will be used by parent pointers which
> need to add attributes to newly created inodes that do not yet
> have a fork.

We already have the capability of doing that in xfs_init_new_inode()
by passing in init_xattrs == true. So when we are creating a new
inode with parent pointers enabled, we know that we are going to be
creating an xattr on the inode and so we should always set
init_xattrs in that case.

This should avoid the need for parent pointers to ever need to run
an extra transaction to create the attr fork. Hence, AFAICT, this
new state to handle attr fork creation shouldn't ever be needed for
parent pointers....

What am I missing?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com