Re: [PATCH 13/16] xfs: refactor inode ownership change transaction/inode/quota allocation idiom

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 01, 2021 at 06:04:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
> 
> For file ownership (uid, gid, prid) changes, create a new helper
> xfs_trans_alloc_ichange that allocates a transaction and reserves the
> appropriate amount of quota against that transction in preparation for a
> change of user, group, or project id.  Replace all the open-coded idioms
> with a single call to this helper so that we can contain the retry loops
> in the next patchset.
> 
> This changes the locking behavior for ichange transactions slightly.
> Since tr_ichange does not have a permanent reservation and cannot roll,
> we pass XFS_ILOCK_EXCL to ijoin so that the inode will be unlocked
> automatically at commit time.
> 
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

>  fs/xfs/xfs_ioctl.c |   29 ++++++++----------------
>  fs/xfs/xfs_iops.c  |   26 ++--------------------
>  fs/xfs/xfs_trans.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_trans.h |    3 +++
>  4 files changed, 77 insertions(+), 43 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 3fbd98f61ea5..78ee201eb7cb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1275,24 +1275,23 @@ xfs_ioctl_setattr_prepare_dax(
>   */
>  static struct xfs_trans *
>  xfs_ioctl_setattr_get_trans(
> -	struct xfs_inode	*ip)
> +	struct xfs_inode	*ip,
> +	struct xfs_dquot	*pdqp)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_trans	*tp;
>  	int			error = -EROFS;
>  
>  	if (mp->m_flags & XFS_MOUNT_RDONLY)
> -		goto out_unlock;
> +		goto out_error;
>  	error = -EIO;
>  	if (XFS_FORCED_SHUTDOWN(mp))
> -		goto out_unlock;
> +		goto out_error;
>  
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> +			capable(CAP_FOWNER), &tp);
>  	if (error)
> -		goto out_unlock;
> -
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +		goto out_error;
>  
>  	/*
>  	 * CAP_FOWNER overrides the following restrictions:
> @@ -1312,7 +1311,7 @@ xfs_ioctl_setattr_get_trans(
>  
>  out_cancel:
>  	xfs_trans_cancel(tp);
> -out_unlock:
> +out_error:
>  	return ERR_PTR(error);
>  }
>  
> @@ -1462,20 +1461,12 @@ xfs_ioctl_setattr(
>  
>  	xfs_ioctl_setattr_prepare_dax(ip, fa);
>  
> -	tp = xfs_ioctl_setattr_get_trans(ip);
> +	tp = xfs_ioctl_setattr_get_trans(ip, pdqp);
>  	if (IS_ERR(tp)) {
>  		code = PTR_ERR(tp);
>  		goto error_free_dquots;
>  	}
>  
> -	if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
> -	    ip->i_d.di_projid != fa->fsx_projid) {
> -		code = xfs_qm_vop_chown_reserve(tp, ip, NULL, NULL, pdqp,
> -				capable(CAP_FOWNER) ?  XFS_QMOPT_FORCE_RES : 0);
> -		if (code)	/* out of quota */
> -			goto error_trans_cancel;
> -	}
> -
>  	xfs_fill_fsxattr(ip, false, &old_fa);
>  	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
>  	if (code)
> @@ -1608,7 +1599,7 @@ xfs_ioc_setxflags(
>  
>  	xfs_ioctl_setattr_prepare_dax(ip, &fa);
>  
> -	tp = xfs_ioctl_setattr_get_trans(ip);
> +	tp = xfs_ioctl_setattr_get_trans(ip, NULL);
>  	if (IS_ERR(tp)) {
>  		error = PTR_ERR(tp);
>  		goto out_drop_write;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f1e21b6cfa48..00369502fe25 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -700,13 +700,11 @@ xfs_setattr_nonsize(
>  			return error;
>  	}
>  
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> +			capable(CAP_FOWNER), &tp);
>  	if (error)
>  		goto out_dqrele;
>  
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, 0);
> -
>  	/*
>  	 * Change file ownership.  Must be the owner or privileged.
>  	 */
> @@ -722,21 +720,6 @@ xfs_setattr_nonsize(
>  		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
>  		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
>  
> -		/*
> -		 * Do a quota reservation only if uid/gid is actually
> -		 * going to change.
> -		 */
> -		if (XFS_IS_QUOTA_RUNNING(mp) &&
> -		    ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
> -		     (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
> -			ASSERT(tp);
> -			error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> -						NULL, capable(CAP_FOWNER) ?
> -						XFS_QMOPT_FORCE_RES : 0);
> -			if (error)	/* out of quota */
> -				goto out_cancel;
> -		}
> -
>  		/*
>  		 * CAP_FSETID overrides the following restrictions:
>  		 *
> @@ -786,8 +769,6 @@ xfs_setattr_nonsize(
>  		xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp);
>  
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -
>  	/*
>  	 * Release any dquot(s) the inode had kept before chown.
>  	 */
> @@ -814,9 +795,6 @@ xfs_setattr_nonsize(
>  
>  	return 0;
>  
> -out_cancel:
> -	xfs_trans_cancel(tp);
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out_dqrele:
>  	xfs_qm_dqrele(udqp);
>  	xfs_qm_dqrele(gdqp);
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 6c68635cc6ac..60672b5545c9 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1107,3 +1107,65 @@ xfs_trans_alloc_icreate(
>  	*tpp = tp;
>  	return 0;
>  }
> +
> +/*
> + * Allocate an transaction, lock and join the inode to it, and reserve quota
> + * in preparation for inode attribute changes that include uid, gid, or prid
> + * changes.
> + *
> + * The caller must ensure that the on-disk dquots attached to this inode have
> + * already been allocated and initialized.  The ILOCK will be dropped when the
> + * transaction is committed or cancelled.
> + */
> +int
> +xfs_trans_alloc_ichange(
> +	struct xfs_inode	*ip,
> +	struct xfs_dquot	*udqp,
> +	struct xfs_dquot	*gdqp,
> +	struct xfs_dquot	*pdqp,
> +	bool			force,
> +	struct xfs_trans	**tpp)
> +{
> +	struct xfs_trans	*tp;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	int			error;
> +
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +
> +	error = xfs_qm_dqattach_locked(ip, false);
> +	if (error) {
> +		/* Caller should have allocated the dquots! */
> +		ASSERT(error != -ENOENT);
> +		goto out_cancel;
> +	}
> +
> +	/*
> +	 * For each quota type, skip quota reservations if the inode's dquots
> +	 * now match the ones that came from the caller, or the caller didn't
> +	 * pass one in.
> +	 */
> +	if (udqp == ip->i_udquot)
> +		udqp = NULL;
> +	if (gdqp == ip->i_gdquot)
> +		gdqp = NULL;
> +	if (pdqp == ip->i_pdquot)
> +		pdqp = NULL;
> +	if (udqp || gdqp || pdqp) {
> +		error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp, pdqp,
> +				force ? XFS_QMOPT_FORCE_RES : 0);
> +		if (error)
> +			goto out_cancel;
> +	}
> +
> +	*tpp = tp;
> +	return 0;
> +
> +out_cancel:
> +	xfs_trans_cancel(tp);
> +	return error;
> +}
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 04c132c55e9b..8b03fbfe9a1b 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -277,5 +277,8 @@ int xfs_trans_alloc_icreate(struct xfs_mount *mp, struct xfs_trans_res *resv,
>  		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
>  		struct xfs_dquot *pdqp, unsigned int dblocks,
>  		struct xfs_trans **tpp);
> +int xfs_trans_alloc_ichange(struct xfs_inode *ip, struct xfs_dquot *udqp,
> +		struct xfs_dquot *gdqp, struct xfs_dquot *pdqp, bool force,
> +		struct xfs_trans **tpp);
>  
>  #endif	/* __XFS_TRANS_H__ */
> 




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux