Re: [PATCH v2 15/16] btrfs: reserve correct number of items for inode creation

[Omar Sandoval <osandov@osandov.com>]

On Fri, Mar 11, 2022 at 12:56:43PM -0500, Sweet Tea Dorminy wrote:
> 
> On 3/9/22 20:31, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > The various inode creation code paths do not account for the compression
> > property, POSIX ACLs, or the parent inode item when starting a
> > transaction. Fix it by refactoring all of these code paths to use a new
> > function, btrfs_new_inode_prepare(), which computes the correct number
> > of items. To do so, it needs to know whether POSIX ACLs will be created,
> > so move the ACL creation into that function. To reduce the number of
> > arguments that need to be passed around for inode creation, define
> > struct btrfs_new_inode_args containing all of the relevant information.
> > 
> > btrfs_new_inode_prepare() will also be a good place to set up the
> > fscrypt context and encrypted filename in the future.
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >   fs/btrfs/acl.c   |  36 +------
> >   fs/btrfs/ctree.h |  34 +++++--
> >   fs/btrfs/inode.c | 256 ++++++++++++++++++++++++++++++++++-------------
> >   fs/btrfs/ioctl.c |  83 ++++++++++-----
> >   4 files changed, 277 insertions(+), 132 deletions(-)
> > 
> > diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> > index a6909ec9bc38..548d6a5477b4 100644
> > --- a/fs/btrfs/acl.c
> > +++ b/fs/btrfs/acl.c
> > @@ -55,8 +55,8 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type, bool rcu)
> >   	return acl;
> >   }
> > -static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
> > -			   struct inode *inode, struct posix_acl *acl, int type)
> > +int __btrfs_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
> > +		    struct posix_acl *acl, int type)
> >   {
> >   	int ret, size = 0;
> >   	const char *name;
> > @@ -127,35 +127,3 @@ int btrfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >   		inode->i_mode = old_mode;
> >   	return ret;
> >   }
> > -
> > -int btrfs_init_acl(struct btrfs_trans_handle *trans,
> > -		   struct inode *inode, struct inode *dir)
> > -{
> > -	struct posix_acl *default_acl, *acl;
> > -	int ret = 0;
> > -
> > -	/* this happens with subvols */
> > -	if (!dir)
> > -		return 0;
> > -
> > -	ret = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (default_acl) {
> > -		ret = __btrfs_set_acl(trans, inode, default_acl,
> > -				      ACL_TYPE_DEFAULT);
> > -		posix_acl_release(default_acl);
> > -	}
> > -
> > -	if (acl) {
> > -		if (!ret)
> > -			ret = __btrfs_set_acl(trans, inode, acl,
> > -					      ACL_TYPE_ACCESS);
> > -		posix_acl_release(acl);
> > -	}
> > -
> > -	if (!default_acl && !acl)
> > -		cache_no_acl(inode);
> > -	return ret;
> > -}
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index f39730420e8a..322c02610e9e 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -3254,11 +3254,32 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, long nr,
> >   int btrfs_set_extent_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
> >   			      unsigned int extra_bits,
> >   			      struct extent_state **cached_state);
> > +struct btrfs_new_inode_args {
> > +	/* Input */
> > +	struct inode *dir;
> > +	struct dentry *dentry;
> > +	struct inode *inode;
> > +	bool orphan;
> > +	bool subvol;
> > +
> > +	/*
> > +	 * Output from btrfs_new_inode_prepare(), input to
> > +	 * btrfs_create_new_inode().
> > +	 */
> > +	struct posix_acl *default_acl;
> > +	struct posix_acl *acl;
> > +};
> > +int btrfs_new_inode_prepare(struct btrfs_new_inode_args *args,
> > +			    unsigned int *trans_num_items);
> > +int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
> > +			   struct btrfs_new_inode_args *args,
> > +			   u64 *index);
> > +void btrfs_new_inode_args_destroy(struct btrfs_new_inode_args *args);
> >   struct inode *btrfs_new_subvol_inode(struct user_namespace *mnt_userns,
> >   				     struct inode *dir);
> >   int btrfs_create_subvol_root(struct btrfs_trans_handle *trans,
> >   			     struct btrfs_root *parent_root,
> > -			     struct inode *inode);
> > +			     struct btrfs_new_inode_args *args);
> >    void btrfs_set_delalloc_extent(struct inode *inode, struct extent_state *state,
> >   			       unsigned *bits);
> >   void btrfs_clear_delalloc_extent(struct inode *inode,
> > @@ -3816,15 +3837,16 @@ static inline int __btrfs_fs_compat_ro(struct btrfs_fs_info *fs_info, u64 flag)
> >   struct posix_acl *btrfs_get_acl(struct inode *inode, int type, bool rcu);
> >   int btrfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >   		  struct posix_acl *acl, int type);
> > -int btrfs_init_acl(struct btrfs_trans_handle *trans,
> > -		   struct inode *inode, struct inode *dir);
> > +int __btrfs_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
> > +		    struct posix_acl *acl, int type);
> >   #else
> >   #define btrfs_get_acl NULL
> >   #define btrfs_set_acl NULL
> > -static inline int btrfs_init_acl(struct btrfs_trans_handle *trans,
> > -				 struct inode *inode, struct inode *dir)
> > +static inline int __btrfs_set_acl(struct btrfs_trans_handle *trans,
> > +				  struct inode *inode, struct posix_acl *acl,
> > +				  int type)
> >   {
> > -	return 0;
> > +	return -EOPNOTSUPP;
> >   }
> >   #endif
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index bea2cb2d90a5..e2b1b1969d0b 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -223,14 +223,26 @@ static int btrfs_dirty_inode(struct inode *inode);
> >   static int btrfs_init_inode_security(struct btrfs_trans_handle *trans,
> >   				     struct inode *inode,  struct inode *dir,
> > -				     const struct qstr *qstr)
> > +				     const struct qstr *qstr,
> > +				     struct posix_acl *default_acl,
> > +				     struct posix_acl *acl)
> >   {
> >   	int err;
> > -	err = btrfs_init_acl(trans, inode, dir);
> > -	if (!err)
> > -		err = btrfs_xattr_security_init(trans, inode, dir, qstr);
> > -	return err;
> > +	if (default_acl) {
> > +		err = __btrfs_set_acl(trans, inode, default_acl,
> > +				      ACL_TYPE_DEFAULT);
> > +		if (err)
> > +			return err;
> > +	}
> > +	if (acl) {
> > +		err = __btrfs_set_acl(trans, inode, acl, ACL_TYPE_ACCESS);
> > +		if (err)
> > +			return err;
> > +	}
> > +	if (!default_acl && !acl)
> > +		cache_no_acl(inode);
> > +	return btrfs_xattr_security_init(trans, inode, dir, qstr);
> >   }
> 
> 
> Would it be worth making this take a btrfs_new_inode_args also, since
> basically everything it needs is contained therein? I think the only place
> calling btrfs_init_inode_security() with params not just pulled out of the
> btrfs_new_inode_args is btrfs_tempfile, which is passing a NULL name instead
> of &dentry->d_name; I'm not clear on why in that case it's different...

That's a good idea, I'll do that.