From: "Darrick J. Wong" <djwong@kernel.org>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org, hch@infradead.org, david@fromorbit.com
Subject: Re: [PATCH 06/13] xfs: reserve data and rt quota at the same time
Date: Thu, 28 Jan 2021 10:52:09 -0800 [thread overview]
Message-ID: <20210128185209.GX7698@magnolia> (raw)
In-Reply-To: <20210128181021.GE2619139@bfoster>
On Thu, Jan 28, 2021 at 01:10:21PM -0500, Brian Foster wrote:
> On Wed, Jan 27, 2021 at 10:01:38PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Modify xfs_trans_reserve_quota_nblks so that we can reserve data and
> > realtime blocks from the dquot at the same time. This change has the
> > theoretical side effect that for allocations to realtime files we will
> > reserve from the dquot both the number of rtblocks being allocated and
> > the number of bmbt blocks that might be needed to add the mapping.
> > However, since the mount code disables quota if it finds a realtime
> > device, this should not result in any behavior changes.
> >
> > This also replaces the flags argument with a force? boolean since we
>
> s/?//
>
> > don't need to distinguish between data and rt quota reservations any
> > more, and the only other flag being passed in was FORCE_RES.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 6 +-----
> > fs/xfs/libxfs/xfs_bmap.c | 4 +---
> > fs/xfs/xfs_bmap_util.c | 20 +++++++++-----------
> > fs/xfs/xfs_iomap.c | 26 +++++++++++++-------------
> > fs/xfs/xfs_quota.h | 10 +++++-----
> > fs/xfs/xfs_reflink.c | 6 ++----
> > fs/xfs/xfs_trans_dquot.c | 42 ++++++++++++++++++++++++++++--------------
> > 7 files changed, 59 insertions(+), 55 deletions(-)
> >
> >
> ...
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index f0a8f3377281..d54d9f02d3dd 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> ...
> > @@ -792,18 +792,17 @@ xfs_alloc_file_space(
> > if (unlikely(rt)) {
> > resrtextents = qblocks = resblks;
>
> This looks like the last usage of qblocks in the function.
Oops, yes, that can go away. Fixed.
> > resrtextents /= mp->m_sb.sb_rextsize;
> > - resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> > - quota_flag = XFS_QMOPT_RES_RTBLKS;
> > + dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> > + rblocks = resblks;
> > } else {
> > - resrtextents = 0;
> > - resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
> > - quota_flag = XFS_QMOPT_RES_REGBLKS;
> > + dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
> > + rblocks = 0;
> > }
> >
> > /*
> > * Allocate and setup the transaction.
> > */
> > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks,
> > + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, dblocks,
> > resrtextents, 0, &tp);
> >
> > /*
> ...
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index de0e371ba4dd..ef29d44c656a 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> ...
> > @@ -235,18 +235,19 @@ xfs_iomap_write_direct(
> > if (IS_DAX(VFS_I(ip))) {
> > bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
> > if (imap->br_state == XFS_EXT_UNWRITTEN) {
> > + force = true;
> > tflags |= XFS_TRANS_RESERVE;
> > - resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
> > + dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
>
> I'm a little confused what this hunk of logic is for in the first place,
I think this was to prevent racing write faults to an unwritten extent
in an S_DAX file from clobbering each other. Looking at the dax write
fault path (__xfs_filemap_fault), we only take MMAPLOCK_SHARED, so the
only way we can serialize the storage zeroing that must be done with
unwritten extent conversion is when we take the ILOCK to do the
conversion, aka BMAPI_ZERO.
xfs_direct_write_iomap_begin accomplishes this by (ab|re)using the
"allocation" path to do the extent conversion. The space is already
allocated, so we only have to reserve enough free space/quota to handle
the bmbt split.
> but doesn't this also adjust down the quota where it previously only
> affected the transaction reservation? Is that intentional?
So yes, it does adjust down the quota reservation for this one case, but
I don't think we needed it in the first place. Still, I'll cut out this
part and make it a separate fix patch so that the conversion is more
straightforward. Thanks for pointing that out.
--D
> Brian
>
> > }
> > }
> > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, resrtextents,
> > + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, dblocks, resrtextents,
> > tflags, &tp);
> > if (error)
> > return error;
> >
> > xfs_ilock(ip, XFS_ILOCK_EXCL);
> >
> > - error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
> > + error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
> > if (error)
> > goto out_trans_cancel;
> >
> > @@ -559,8 +560,7 @@ xfs_iomap_write_unwritten(
> > xfs_ilock(ip, XFS_ILOCK_EXCL);
> > xfs_trans_ijoin(tp, ip, 0);
> >
> > - error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
> > - XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES);
> > + error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, true);
> > if (error)
> > goto error_on_bmapi_transaction;
> >
> > diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> > index 72f4cfc49048..efd04f84d9b4 100644
> > --- a/fs/xfs/xfs_quota.h
> > +++ b/fs/xfs/xfs_quota.h
> > @@ -81,8 +81,8 @@ extern void xfs_trans_mod_dquot_byino(struct xfs_trans *, struct xfs_inode *,
> > uint, int64_t);
> > extern void xfs_trans_apply_dquot_deltas(struct xfs_trans *);
> > extern void xfs_trans_unreserve_and_mod_dquots(struct xfs_trans *);
> > -extern int xfs_trans_reserve_quota_nblks(struct xfs_trans *,
> > - struct xfs_inode *, int64_t, long, uint);
> > +int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp, struct xfs_inode *ip,
> > + int64_t dblocks, int64_t rblocks, bool force);
> > extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
> > struct xfs_mount *, struct xfs_dquot *,
> > struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
> > @@ -114,8 +114,7 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
> > static inline int
> > xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t dblocks)
> > {
> > - return xfs_trans_reserve_quota_nblks(NULL, ip, dblocks, 0,
> > - XFS_QMOPT_RES_REGBLKS);
> > + return xfs_trans_reserve_quota_nblks(NULL, ip, dblocks, 0, false);
> > }
> > #else
> > static inline int
> > @@ -134,7 +133,8 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
> > #define xfs_trans_apply_dquot_deltas(tp)
> > #define xfs_trans_unreserve_and_mod_dquots(tp)
> > static inline int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp,
> > - struct xfs_inode *ip, int64_t nblks, long ninos, uint flags)
> > + struct xfs_inode *ip, int64_t dblocks, int64_t rblocks,
> > + bool force)
> > {
> > return 0;
> > }
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 15435229bc1f..0778b5810c26 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -398,8 +398,7 @@ xfs_reflink_allocate_cow(
> > goto convert;
> > }
> >
> > - error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
> > - XFS_QMOPT_RES_REGBLKS);
> > + error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
> > if (error)
> > goto out_trans_cancel;
> >
> > @@ -1090,8 +1089,7 @@ xfs_reflink_remap_extent(
> > if (!smap_real && dmap_written)
> > qres += dmap->br_blockcount;
> > if (qres > 0) {
> > - error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0,
> > - XFS_QMOPT_RES_REGBLKS);
> > + error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0, false);
> > if (error)
> > goto out_cancel;
> > }
> > diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> > index 22aa875b84f7..a1a72b7900c5 100644
> > --- a/fs/xfs/xfs_trans_dquot.c
> > +++ b/fs/xfs/xfs_trans_dquot.c
> > @@ -780,28 +780,42 @@ int
> > xfs_trans_reserve_quota_nblks(
> > struct xfs_trans *tp,
> > struct xfs_inode *ip,
> > - int64_t nblks,
> > - long ninos,
> > - uint flags)
> > + int64_t dblocks,
> > + int64_t rblocks,
> > + bool force)
> > {
> > struct xfs_mount *mp = ip->i_mount;
> > + unsigned int qflags = 0;
> > + int error;
> >
> > if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
> > return 0;
> >
> > ASSERT(!xfs_is_quota_inode(&mp->m_sb, ip->i_ino));
> > -
> > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > - ASSERT((flags & ~(XFS_QMOPT_FORCE_RES)) == XFS_TRANS_DQ_RES_RTBLKS ||
> > - (flags & ~(XFS_QMOPT_FORCE_RES)) == XFS_TRANS_DQ_RES_BLKS);
> > -
> > - /*
> > - * Reserve nblks against these dquots, with trans as the mediator.
> > - */
> > - return xfs_trans_reserve_quota_bydquots(tp, mp,
> > - ip->i_udquot, ip->i_gdquot,
> > - ip->i_pdquot,
> > - nblks, ninos, flags);
> > +
> > + if (force)
> > + qflags |= XFS_QMOPT_FORCE_RES;
> > +
> > + /* Reserve data device quota against the inode's dquots. */
> > + error = xfs_trans_reserve_quota_bydquots(tp, mp, ip->i_udquot,
> > + ip->i_gdquot, ip->i_pdquot, dblocks, 0,
> > + XFS_QMOPT_RES_REGBLKS | qflags);
> > + if (error)
> > + return error;
> > +
> > + /* Do the same but for realtime blocks. */
> > + error = xfs_trans_reserve_quota_bydquots(tp, mp, ip->i_udquot,
> > + ip->i_gdquot, ip->i_pdquot, rblocks, 0,
> > + XFS_QMOPT_RES_RTBLKS | qflags);
> > + if (error) {
> > + xfs_trans_reserve_quota_bydquots(tp, mp, ip->i_udquot,
> > + ip->i_gdquot, ip->i_pdquot, -dblocks, 0,
> > + XFS_QMOPT_RES_REGBLKS);
> > + return error;
> > + }
> > +
> > + return 0;
> > }
> >
> > /* Change the quota reservations for an inode creation activity. */
> >
>
next prev parent reply other threads:[~2021-01-28 18:55 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-28 6:01 [PATCHSET v3 00/13] xfs: minor cleanups of the quota functions Darrick J. Wong
2021-01-28 6:01 ` [PATCH 01/13] xfs: clean up quota reservation callsites Darrick J. Wong
2021-01-28 6:01 ` [PATCH 02/13] xfs: create convenience wrappers for incore quota block reservations Darrick J. Wong
2021-01-28 18:08 ` Brian Foster
2021-01-28 6:01 ` [PATCH 03/13] xfs: remove xfs_trans_unreserve_quota_nblks completely Darrick J. Wong
2021-01-28 9:46 ` Christoph Hellwig
2021-01-28 18:08 ` Brian Foster
2021-01-28 6:01 ` [PATCH 04/13] xfs: clean up icreate quota reservation calls Darrick J. Wong
2021-01-28 18:09 ` Brian Foster
2021-01-28 6:01 ` [PATCH 05/13] xfs: fix up build warnings when quotas are disabled Darrick J. Wong
2021-01-28 18:09 ` Brian Foster
2021-01-28 18:22 ` Darrick J. Wong
2021-01-28 18:23 ` Christoph Hellwig
2021-01-28 6:01 ` [PATCH 06/13] xfs: reserve data and rt quota at the same time Darrick J. Wong
2021-01-28 9:49 ` Christoph Hellwig
2021-01-28 18:01 ` Darrick J. Wong
2021-01-28 18:10 ` Brian Foster
2021-01-28 18:52 ` Darrick J. Wong [this message]
2021-01-28 6:01 ` [PATCH 07/13] xfs: refactor common transaction/inode/quota allocation idiom Darrick J. Wong
2021-01-28 9:50 ` Christoph Hellwig
2021-01-28 18:22 ` Brian Foster
2021-01-28 6:01 ` [PATCH 08/13] xfs: allow reservation of rtblocks with xfs_trans_alloc_inode Darrick J. Wong
2021-01-28 9:51 ` Christoph Hellwig
2021-01-28 18:22 ` Brian Foster
2021-01-28 6:01 ` [PATCH 09/13] xfs: refactor reflink functions to use xfs_trans_alloc_inode Darrick J. Wong
2021-01-28 9:53 ` Christoph Hellwig
2021-01-28 18:06 ` Darrick J. Wong
2021-01-28 18:23 ` Brian Foster
2021-01-28 6:02 ` [PATCH 10/13] xfs: try worst case space reservation upfront in xfs_reflink_remap_extent Darrick J. Wong
2021-01-28 9:55 ` Christoph Hellwig
2021-01-28 18:23 ` Brian Foster
2021-01-28 6:02 ` [PATCH 11/13] xfs: refactor inode creation transaction/inode/quota allocation idiom Darrick J. Wong
2021-01-28 9:57 ` Christoph Hellwig
2021-01-28 18:18 ` Darrick J. Wong
2021-01-28 18:23 ` Brian Foster
2021-01-28 6:02 ` [PATCH 12/13] xfs: move xfs_qm_vop_chown_reserve to xfs_trans_dquot.c Darrick J. Wong
2021-01-28 9:58 ` Christoph Hellwig
2021-01-28 18:23 ` Brian Foster
2021-01-28 6:02 ` [PATCH 13/13] xfs: clean up xfs_trans_reserve_quota_chown a bit Darrick J. Wong
2021-01-28 10:00 ` Christoph Hellwig
2021-01-28 18:23 ` 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=20210128185209.GX7698@magnolia \
--to=djwong@kernel.org \
--cc=bfoster@redhat.com \
--cc=david@fromorbit.com \
--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