From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, hch@infradead.org, david@fromorbit.com
Subject: Re: [PATCH 13/16] xfs: refactor inode ownership change transaction/inode/quota allocation idiom
Date: Tue, 2 Feb 2021 08:13:22 -0500 [thread overview]
Message-ID: <20210202131322.GC3336100@bfoster> (raw)
In-Reply-To: <161223147171.491593.1153393231638526420.stgit@magnolia>
On Mon, Feb 01, 2021 at 06:04:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> 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@kernel.org>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> 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__ */
>
next prev parent reply other threads:[~2021-02-02 13:19 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-02 2:03 [PATCHSET v6 00/16] xfs: minor cleanups of the quota functions Darrick J. Wong
2021-02-02 2:03 ` [PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails Darrick J. Wong
2021-02-02 13:13 ` Brian Foster
2021-02-02 17:47 ` Darrick J. Wong
2021-02-02 17:56 ` Christoph Hellwig
2021-02-02 18:34 ` Brian Foster
2021-02-02 19:38 ` [PATCH v6.1 " Darrick J. Wong
2021-02-02 2:03 ` [PATCH 02/16] xfs: clean up quota reservation callsites Darrick J. Wong
2021-02-02 2:03 ` [PATCH 03/16] xfs: create convenience wrappers for incore quota block reservations Darrick J. Wong
2021-02-02 2:03 ` [PATCH 04/16] xfs: remove xfs_trans_unreserve_quota_nblks completely Darrick J. Wong
2021-02-02 2:03 ` [PATCH 05/16] xfs: clean up icreate quota reservation calls Darrick J. Wong
2021-02-02 2:03 ` [PATCH 06/16] xfs: fix up build warnings when quotas are disabled Darrick J. Wong
2021-02-02 2:03 ` [PATCH 07/16] xfs: reduce quota reservation when doing a dax unwritten extent conversion Darrick J. Wong
2021-02-02 2:04 ` [PATCH 08/16] xfs: reserve data and rt quota at the same time Darrick J. Wong
2021-02-02 2:04 ` [PATCH 09/16] xfs: refactor common transaction/inode/quota allocation idiom Darrick J. Wong
2021-02-02 2:04 ` [PATCH 10/16] xfs: allow reservation of rtblocks with xfs_trans_alloc_inode Darrick J. Wong
2021-02-02 2:04 ` [PATCH 11/16] xfs: refactor reflink functions to use xfs_trans_alloc_inode Darrick J. Wong
2021-02-02 2:04 ` [PATCH 12/16] xfs: refactor inode creation transaction/inode/quota allocation idiom Darrick J. Wong
2021-02-02 2:04 ` [PATCH 13/16] xfs: refactor inode ownership change " Darrick J. Wong
2021-02-02 7:04 ` Christoph Hellwig
2021-02-02 13:13 ` Brian Foster [this message]
2021-02-02 2:04 ` [PATCH 14/16] xfs: remove xfs_qm_vop_chown_reserve Darrick J. Wong
2021-02-02 7:05 ` Christoph Hellwig
2021-02-02 13:13 ` Brian Foster
2021-02-02 2:04 ` [PATCH 15/16] xfs: rename code to error in xfs_ioctl_setattr Darrick J. Wong
2021-02-02 13:13 ` Brian Foster
2021-02-02 2:04 ` [PATCH 16/16] xfs: shut down the filesystem if we screw up quota errors Darrick J. Wong
2021-02-02 13:13 ` Brian Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210202131322.GC3336100@bfoster \
--to=bfoster@redhat.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=hch@infradead.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.