On Fri, Jan 26, 2018 at 02:06:49PM -0500, Brian Foster wrote:
> On Thu, Jan 25, 2018 at 06:05:00PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Since the CoW fork only exists in memory, it is incorrect to update the
> > on-disk quota block counts when we modify the CoW fork. Unlike the data
> > fork, even real extents in the CoW fork are only reservations (on-disk
> > they're owned by the refcountbt) so they must not be tracked in the on
> > disk quota info.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
>
> Looks sane.. just a couple comments from my braindump on the previous
> patch and a few comment suggestions that help me understand this better
> (feel free to tweak, rewrite, etc.).
>
> > fs/xfs/libxfs/xfs_bmap.c | 33 ++++++++++++++++++++++++++++++---
> > fs/xfs/libxfs/xfs_bmap.h | 2 +-
> > fs/xfs/xfs_reflink.c | 12 ++++++------
> > 3 files changed, 37 insertions(+), 10 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 6ad79ea..a59d5be 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3343,8 +3343,26 @@ xfs_bmap_btalloc_accounting(
> > struct xfs_bmalloca *ap,
> > struct xfs_alloc_arg *args)
> > {
> > - if (!(ap->flags & XFS_BMAPI_COWFORK))
> > - ap->ip->i_d.di_nblocks += args->len;
> > + if (ap->flags & XFS_BMAPI_COWFORK) {
>
> /*
> * COW fork blocks are in-core only and thus are treated as in-core
> * quota reservation (like delalloc blocks) even when converted to real
> * blocks. The quota reservation is not accounted to disk until blocks
> * are remapped to the data fork. So if these blocks were previously
> * delalloc, we already have quota reservation and there's nothing to do
> * yet.
> */
Ok.
>
> > + /* Filling a previously reserved extent; nothing to do here. */
> > + if (ap->wasdel)
> > + return;
> > +
> > + /*
> > + * If we get here, we're filling a CoW hole with a real
> > + * (non-delalloc) CoW extent having reserved enough blocks
> > + * from both q_res_bcount and qt_blk_res to guarantee that we
> > + * won't run out of space. The unused qt_blk_res is given
> > + * back to q_res_bcount when the transaction commits, so we
> > + * must decrease qt_blk_res without decreasing q_res_bcount.
> > + */
>
> /*
> * Otherwise, we've allocated blocks in a hole. The transaction has
> * acquired in-core quota reservation for this extent. Rather than
> * account these as real blocks, however, we reduce the transaction
> * quota reservation based on the allocation. This essentially transfers
> * the transaction quota reservation to that of a delalloc extent.
> */
Ok.
>
> > + xfs_trans_mod_dquot_byino(ap->tp, ap->ip, XFS_TRANS_DQ_RES_BLKS,
> > + -(long)args->len);
> > + return;
> > + }
> > +
> > + /* data/attr fork only */
> > + ap->ip->i_d.di_nblocks += args->len;
> > xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
> > if (ap->wasdel)
> > ap->ip->i_delayed_blks -= args->len;
> > @@ -4761,13 +4779,15 @@ xfs_bmap_del_extent_cow(
> > struct xfs_inode *ip,
> > struct xfs_iext_cursor *icur,
> > struct xfs_bmbt_irec *got,
> > - struct xfs_bmbt_irec *del)
> > + struct xfs_bmbt_irec *del,
> > + bool free_quotares)
> > {
> > struct xfs_mount *mp = ip->i_mount;
> > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> > struct xfs_bmbt_irec new;
> > xfs_fileoff_t del_endoff, got_endoff;
> > int state = BMAP_COWFORK;
> > + int error;
> >
> > XFS_STATS_INC(mp, xs_del_exlist);
> >
> > @@ -4824,6 +4844,13 @@ xfs_bmap_del_extent_cow(
> > xfs_iext_insert(ip, icur, &new, state);
> > break;
> > }
> > +
> > + /* Remove the quota reservation */
> > + if (!free_quotares)
> > + return;
> > + error = xfs_trans_reserve_quota_nblks(NULL, ip,
> > + -(long)del->br_blockcount, 0, XFS_QMOPT_RES_REGBLKS);
> > + ASSERT(error == 0);
>
> Might as well pull this into the only free_quotares = true caller.
Done. Thx for the review!
--D
> Brian
>
> > }
> >
> > /*
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index e36d757..e99f28f 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -224,7 +224,7 @@ int xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
> > struct xfs_bmbt_irec *del);
> > void xfs_bmap_del_extent_cow(struct xfs_inode *ip,
> > struct xfs_iext_cursor *cur, struct xfs_bmbt_irec *got,
> > - struct xfs_bmbt_irec *del);
> > + struct xfs_bmbt_irec *del, bool free_quotares);
> > uint xfs_default_attroffset(struct xfs_inode *ip);
> > int xfs_bmap_collapse_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> > xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 82abff6..3644a08 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -599,10 +599,6 @@ xfs_reflink_cancel_cow_blocks(
> > del.br_startblock, del.br_blockcount,
> > NULL);
> >
> > - /* Update quota accounting */
> > - xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
> > - -(long)del.br_blockcount);
> > -
> > /* Roll the transaction */
> > xfs_defer_ijoin(&dfops, ip);
> > error = xfs_defer_finish(tpp, &dfops);
> > @@ -612,7 +608,7 @@ xfs_reflink_cancel_cow_blocks(
> > }
> >
> > /* Remove the mapping from the CoW fork. */
> > - xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
> > + xfs_bmap_del_extent_cow(ip, &icur, &got, &del, true);
> > } else {
> > /* Didn't do anything, push cursor back. */
> > xfs_iext_prev(ifp, &icur);
> > @@ -795,8 +791,12 @@ xfs_reflink_end_cow(
> > if (error)
> > goto out_defer;
> >
> > + /* Charge this new data fork mapping to the on-disk quota. */
> > + xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_DELBCOUNT,
> > + (long)del.br_blockcount);
> > +
> > /* Remove the mapping from the CoW fork. */
> > - xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
> > + xfs_bmap_del_extent_cow(ip, &icur, &got, &del, false);
> >
> > xfs_defer_ijoin(&dfops, ip);
> > error = xfs_defer_finish(&tp, &dfops);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
