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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox