On Tue, Jan 23, 2018 at 07:05:57AM -0500, Brian Foster wrote:
> On Sat, Jan 20, 2018 at 09:34:10PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Ensure that we've attached all the necessary dquots before performing
> > reflink operations so that quota accounting is accurate.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_reflink.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> >
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 5d1ff5a..947d0637 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -54,6 +54,7 @@
> > #include "xfs_rmap_btree.h"
> > #include "xfs_sb.h"
> > #include "xfs_ag_resv.h"
> > +#include "xfs_qm.h"
> >
> > /*
> > * Copy on Write of Shared Blocks
> > @@ -282,6 +283,10 @@ xfs_reflink_reserve_cow(
> > * tree.
> > */
> >
> > + error = xfs_qm_dqattach_locked(ip, 0);
> > + if (error)
> > + return error;
> > +
>
> The same call exists further down in the function. Was the intent to
> move it? I suspect we don't need it twice, at least.
Drat, I don't know why either of these are there, I think I got paste-happy?
OH, yuck, this is the debug patch from an earlier revision. The only
dqattach we actually need is...
> > if (!xfs_iext_lookup_extent(ip, ifp, imap->br_startoff, &icur, &got))
> > eof = true;
> > if (!eof && got.br_startoff <= imap->br_startoff) {
> > @@ -396,6 +401,10 @@ xfs_reflink_allocate_cow(
> > ASSERT(xfs_is_reflink_inode(ip));
> > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
> >
> > + error = xfs_qm_dqattach_locked(ip, 0);
> > + if (error)
> > + return error;
> > +
>
> Similar pattern here, but for this one the assert above suggests we
> could have the shared lock. xfs_qm_dqattach_locked() looks like it
> expects the exclusive lock (and that's what it looks like the second
> call deals with). Hm?
>
> Brian
>
> > /*
> > * Even if the extent is not shared we might have a preallocation for
> > * it in the COW fork. If so use it.
> > @@ -1356,6 +1365,14 @@ xfs_reflink_remap_range(
> > if (IS_DAX(inode_in) || IS_DAX(inode_out))
> > goto out_unlock;
> >
> > + /* Attach dquots to both inodes */
> > + ret = xfs_qm_dqattach(src, 0);
> > + if (ret)
> > + goto out_unlock;
> > + ret = xfs_qm_dqattach(dest, 0);
...this one, because we're not changing the src file's block allocations,
but we /are/ forgetting to ensure they're attached to dest.
Sorry for the noise, I'll send out the proper patch later today.
--D
> > + if (ret)
> > + goto out_unlock;
> > +
> > ret = vfs_clone_file_prep_inodes(inode_in, pos_in, inode_out, pos_out,
> > &len, is_dedupe);
> > if (ret <= 0)
> >
> > --
> > 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
