Re: [PATCH v1 10/17] xfs: parent pointer attribute creation

[Alli <allison.henderson@oracle.com>]

On Thu, 2022-06-16 at 15:49 +1000, Dave Chinner wrote:
> On Sat, Jun 11, 2022 at 02:41:53AM -0700, Allison Henderson wrote:
> > Add parent pointer attribute during xfs_create, and subroutines to
> > initialize attributes
> > 
> > [bfoster: rebase, use VFS inode generation]
> > [achender: rebased, changed __unint32_t to xfs_dir2_dataptr_t,
> >            fixed some null pointer bugs,
> >            merged error handling patch,
> >            added subroutines to handle attribute initialization,
> >            remove unnecessary ENOSPC handling in
> > xfs_attr_set_first_parent]
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > ---
> >  fs/xfs/Makefile            |  1 +
> >  fs/xfs/libxfs/xfs_attr.c   |  2 +-
> >  fs/xfs/libxfs/xfs_attr.h   |  1 +
> >  fs/xfs/libxfs/xfs_parent.c | 77 +++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_parent.h | 31 ++++++++++++++
> >  fs/xfs/xfs_inode.c         | 88 +++++++++++++++++++++++++++-------
> > ----
> >  fs/xfs/xfs_xattr.c         |  2 +-
> >  fs/xfs/xfs_xattr.h         |  1 +
> >  8 files changed, 177 insertions(+), 26 deletions(-)
> ......
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index b2dfd84e1f62..6b1e4cb11b5c 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -36,6 +36,8 @@
> >  #include "xfs_reflink.h"
> >  #include "xfs_ag.h"
> >  #include "xfs_log_priv.h"
> > +#include "xfs_parent.h"
> > +#include "xfs_xattr.h"
> >  
> >  struct kmem_cache *xfs_inode_cache;
> >  
> > @@ -962,27 +964,40 @@ xfs_bumplink(
> >  
> >  int
> >  xfs_create(
> > -	struct user_namespace	*mnt_userns,
> > -	xfs_inode_t		*dp,
> > -	struct xfs_name		*name,
> > -	umode_t			mode,
> > -	dev_t			rdev,
> > -	bool			init_xattrs,
> > -	xfs_inode_t		**ipp)
> > -{
> > -	int			is_dir = S_ISDIR(mode);
> > -	struct xfs_mount	*mp = dp->i_mount;
> > -	struct xfs_inode	*ip = NULL;
> > -	struct xfs_trans	*tp = NULL;
> > -	int			error;
> > -	bool                    unlock_dp_on_error = false;
> > -	prid_t			prid;
> > -	struct xfs_dquot	*udqp = NULL;
> > -	struct xfs_dquot	*gdqp = NULL;
> > -	struct xfs_dquot	*pdqp = NULL;
> > -	struct xfs_trans_res	*tres;
> > -	uint			resblks;
> > -	xfs_ino_t		ino;
> > +	struct user_namespace		*mnt_userns,
> > +	xfs_inode_t			*dp,
> > +	struct xfs_name			*name,
> > +	umode_t				mode,
> > +	dev_t				rdev,
> > +	bool				init_xattrs,
> > +	xfs_inode_t			**ipp)
> > +{
> > +	int				is_dir = S_ISDIR(mode);
> > +	struct xfs_mount		*mp = dp->i_mount;
> > +	struct xfs_inode		*ip = NULL;
> > +	struct xfs_trans		*tp = NULL;
> > +	int				error;
> > +	bool				unlock_dp_on_error = false;
> > +	prid_t				prid;
> > +	struct xfs_dquot		*udqp = NULL;
> > +	struct xfs_dquot		*gdqp = NULL;
> > +	struct xfs_dquot		*pdqp = NULL;
> > +	struct xfs_trans_res		*tres;
> > +	uint				resblks;
> > +	xfs_ino_t			ino;
> > +	xfs_dir2_dataptr_t		diroffset;
> > +	struct xfs_parent_name_rec	rec;
> > +	struct xfs_da_args		args = {
> > +		.dp		= dp,
> > +		.geo		= mp->m_attr_geo,
> > +		.whichfork	= XFS_ATTR_FORK,
> > +		.attr_filter	= XFS_ATTR_PARENT,
> > +		.op_flags	= XFS_DA_OP_OKNOENT,
> > +		.name		= (const uint8_t *)&rec,
> > +		.namelen	= sizeof(rec),
> > +		.value		= (void *)name->name,
> 
> Why the cast to void?
.value is a void*, but name is a const char *, so we get a compiler
warning with out the cast

> 
> > +		.valuelen	= name->len,
> > +	};
> >  
> >  	trace_xfs_create(dp, name);
> >  
> > @@ -1009,6 +1024,12 @@ xfs_create(
> >  		tres = &M_RES(mp)->tr_create;
> >  	}
> >  
> > +	if (xfs_has_larp(mp)) {
> > +		error = xfs_attr_grab_log_assist(mp);
> > +		if (error)
> > +			goto out_release_dquots;
> > +	}
> 
> Parent pointers can only use logged attributes - so this check
> should actually be:
> 
> 	if (xfs_has_parent_pointers(mp)) {
> 		.....
> 	}
> 
> i.e. having the parent pointer feature bit present on disk turns on
> LARP mode unconditionally for that filesystem.
> 
> This means you are probably going to have to add a LARP mount
> feature bit and xfs_has_larp(mp) should be converted to it. i.e. the
> sysfs debug knob should go away once parent pointers are merged
> because LARP will be turned on by PP and we won't need a debug mode
> for testing LARP anymore...Ok, that makes sense 

Ok, as I recall initially larp was a mount option.  Then later in the
reviews we decided to make it a debug option.  I /think/ the reason was
that debug options are easier to deprecate than mount options?  Since
you dont have to deal with user space changes or breaking peoples mount
commands?  And the sysfs knob can be toggled without a remount.

Also, I seem to recall the reason pptrs was stuck as a mkfs option was
because it cant be toggled right?  Or we end up with some inodes that
have it, and some that don't.  So to be clear: we're proposing a mkfs
option that turns on a mount option.

I guess I'm struggling with why we have to loose the debug option?  It
doesnt seem inappropriate to me to have a mkfs option that alters a
debug option.  Other file systems that dont have pptrs on can still use
the larp knob.  And then the ones that do can just assume it on and
ignore the knob.  Maybe print a warning that clarifies that pptr
enabled file systems cannot disable larp.  Doesnt that retain all the
reasons we put the debug knob in?  With less surgery to larp, userspace
and the test cases?


> 
> > +
> >  	/*
> >  	 * Initially assume that the file does not exist and
> >  	 * reserve the resources for that case.  If that is not
> > @@ -1024,7 +1045,7 @@ xfs_create(
> >  				resblks, &tp);
> >  	}
> >  	if (error)
> > -		goto out_release_dquots;
> > +		goto drop_incompat;
> >  
> >  	xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
> >  	unlock_dp_on_error = true;
> > @@ -1048,11 +1069,12 @@ xfs_create(
> >  	 * the transaction cancel unlocking dp so don't do it
> > explicitly in the
> >  	 * error path.
> >  	 */
> > -	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
> > +	xfs_trans_ijoin(tp, dp, 0);
> >  	unlock_dp_on_error = false;
> >  
> >  	error = xfs_dir_createname(tp, dp, name, ip->i_ino,
> > -				   resblks - XFS_IALLOC_SPACE_RES(mp),
> > NULL);
> > +				   resblks - XFS_IALLOC_SPACE_RES(mp),
> > +				   &diroffset);
> >  	if (error) {
> >  		ASSERT(error != -ENOSPC);
> >  		goto out_trans_cancel;
> > @@ -1068,6 +1090,20 @@ xfs_create(
> >  		xfs_bumplink(tp, dp);
> >  	}
> >  
> > +	/*
> > +	 * If we have parent pointers, we need to add the attribute
> > containing
> > +	 * the parent information now.
> > +	 */
> > +	if (xfs_sb_version_hasparent(&mp->m_sb)) {
> > +		xfs_init_parent_name_rec(&rec, dp, diroffset);
> > +		args.dp	= ip;
> > +		args.trans = tp;
> > +		args.hashval = xfs_da_hashname(args.name,
> > args.namelen);
> > +		error =  xfs_attr_defer_add(&args);
> 
> White space.
will clean out

> 
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> >  	/*
> >  	 * If this is a synchronous mount, make sure that the
> >  	 * create transaction goes to disk before returning to
> > @@ -1093,6 +1129,7 @@ xfs_create(
> >  
> >  	*ipp = ip;
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +	xfs_iunlock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
> >  	return 0;
> >  
> >   out_trans_cancel:
> > @@ -1107,6 +1144,9 @@ xfs_create(
> >  		xfs_finish_inode_setup(ip);
> >  		xfs_irele(ip);
> >  	}
> > + drop_incompat:
> > +	if (xfs_has_larp(mp))
> > +		xlog_drop_incompat_feat(mp->m_log);
> 
> 	if (xfs_has_parent_pointers(mp))
Will update

> 
> >   out_release_dquots:
> >  	xfs_qm_dqrele(udqp);
> >  	xfs_qm_dqrele(gdqp);
> > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> > index 35e13e125ec6..6012a6ba512c 100644
> > --- a/fs/xfs/xfs_xattr.c
> > +++ b/fs/xfs/xfs_xattr.c
> > @@ -27,7 +27,7 @@
> >   * they must release the permission by calling
> > xlog_drop_incompat_feat
> >   * when they're done.
> >   */
> > -static inline int
> > +inline int
> >  xfs_attr_grab_log_assist(
> >  	struct xfs_mount	*mp)
> >  {
> 
> Drop the inline, too.
> 
Alrighty, thx for the reviews!

Allison

> Cheers,
> 
> Dave.
>