From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/6] xfs: CoW fork operations should only update quota reservations
Date: Fri, 26 Jan 2018 11:20:38 -0800 [thread overview]
Message-ID: <20180126192038.GC9068@magnolia> (raw)
In-Reply-To: <20180126190649.GB26910@bfoster.bfoster>
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@oracle.com>
> >
> > 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@oracle.com>
> > ---
>
> 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@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-01-26 19:20 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-26 2:04 [PATCH v2 0/6] xfs: reflink/scrub/quota fixes Darrick J. Wong
2018-01-26 2:04 ` [PATCH 1/6] xfs: refactor accounting updates out of xfs_bmap_btalloc Darrick J. Wong
2018-01-26 12:19 ` Christoph Hellwig
2018-01-26 19:06 ` Brian Foster
2018-01-26 2:05 ` [PATCH 2/6] xfs: CoW fork operations should only update quota reservations Darrick J. Wong
2018-01-26 19:06 ` Brian Foster
2018-01-26 19:20 ` Darrick J. Wong [this message]
2018-01-26 2:05 ` [PATCH 3/6] xfs: track CoW blocks separately in the inode Darrick J. Wong
2018-01-26 2:05 ` [PATCH 4/6] xfs: don't screw up direct writes when freesp is fragmented Darrick J. Wong
2018-01-26 19:24 ` Brian Foster
2018-01-26 19:49 ` Darrick J. Wong
2018-01-26 2:05 ` [PATCH 5/6] xfs: skip CoW writes past EOF when writeback races with truncate Darrick J. Wong
2018-01-26 12:21 ` Christoph Hellwig
2018-01-26 2:05 ` [PATCH 6/6] xfs: don't clobber inobt/finobt cursors when xref with rmap Darrick J. Wong
2018-01-26 12:18 ` [PATCH v2 0/6] xfs: reflink/scrub/quota fixes Christoph Hellwig
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=20180126192038.GC9068@magnolia \
--to=darrick.wong@oracle.com \
--cc=bfoster@redhat.com \
--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.