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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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. */
> >
>
