Re: [PATCH] xfs: flush CoW fork reservations before processing quota get request

[Chandan Rajendra <chandan@linux.vnet.ibm.com>]

On Wednesday, October 31, 2018 9:03:05 PM IST Darrick J. Wong wrote:
> On Tue, Oct 23, 2018 at 12:18:08PM +0530, Chandan Rajendra wrote:
> > generic/305 fails on a 64k block sized filesystem due to the following
> > interaction,
> > 
> > 1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
> > 2. XFS reserves 32 blocks of space in the CoW fork.
> >    xfs_bmap_extsize_align() calculates XFS_DEFAULT_COWEXTSZ_HINT (32
> >    blocks) as the number of blocks to be reserved.
> > 3. The reserved space in the range [1M(i.e. i_size), 1M + 16
> >    blocks] is  freed by __fput(). This corresponds to freeing "eof
> >    blocks" i.e. space reserved beyond EOF of a file.
> > 
> > The reserved space to which data was never written i.e. [9th block,
> > 1M(EOF)], remains reserved in the CoW fork until either the CoW block
> > reservation trimming worker gets invoked or the filesystem is
> > unmounted.
> > 
> > This commit fixes the issue by freeing unused CoW block reservations
> > whenever quota numbers are requested by userspace application.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > ---
> > 
> > PS: With the above patch, the tests xfs/214 & xfs/440 fail because the
> > value passed to xfs_io's cowextsize does not have any effect when CoW
> > fork reservations are flushed before querying for quota usage numbers.
> 
> Hmmm.  I restarted looking into all the weird quota count mismatches in
> xfstests and noticed (with a generous amount of trace_printks) that most
> of the discrepancies can be traced to speculative preallocations in the
> cow fork that don't get cleaned out.  So we're on the same page. :)
> 
> I thought about enhancing the XFS_IOC_FREE_EOFBLOCKS ioctl with a new
> mode to clean out CoW stuff too, but then I started thinking about what
> _check_quota_usage is actually looking for, and realized that (for xfs
> anyway) it compares an aged quota report (reflective of thousands of
> individual fs ops) against a freshly quotacheck'd quota report to look
> for accounting leaks.
> 
> Then I tried replacing the $XFS_SPACEMAN_PROG -c 'prealloc -s' call in
> _check_quota_usage with a umount/mount cycle so that we know we've
> cleaned out all the reservations and *poof* the discrepancies all went
> away.  The test is still useful since we're comparing the accumulated
> quota counts against freshly computed counts, but now we know that we've
> cleaned out any speculative preallocations that xfs might have decided
> to try (assuming xfs never changes behavior to speculate on a fresh
> mount).
> 
> It's awfully tempting to just leave it that way... but what do you
> think?  I think it's a better solution than forcing /every/ quota
> report to iterate the in-core inodes looking for cow blocks to dump.
> 
> Granted maybe we still want the ioctl to do it for us?  Though that
> could get tricky since written extents in the cow fork represent writes
> in progress and can't ever be removed except by xfs_inactive.

Hmm. W.r.t Preallocated EOF blocks, it is easy to identify the blocks to be
removed by the ioctl i.e. blocks which are present beyond inode->i_size.

You are right about the inability to do so for CoW blocks since some of the
unused CoW blocks fall within inode->i_size. Hence I agree with your approach
of replacing "$XFS_SPACEMAN_PROG -c 'prealloc -s' call' in _check_quota_usage
with umount/mount.

If you are fine with it, I can fix _check_quota_usage() and also the relevant
tests.

> 
> > fs/xfs/xfs_quotaops.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> > index a7c0c65..9236a38 100644
> > --- a/fs/xfs/xfs_quotaops.c
> > +++ b/fs/xfs/xfs_quotaops.c
> > @@ -218,14 +218,21 @@ xfs_fs_get_dqblk(
> >  	struct kqid		qid,
> >  	struct qc_dqblk		*qdq)
> >  {
> > +	int			ret;
> >  	struct xfs_mount	*mp = XFS_M(sb);
> >  	xfs_dqid_t		id;
> > +	struct xfs_eofblocks	eofb = { 0 };
> >  
> >  	if (!XFS_IS_QUOTA_RUNNING(mp))
> >  		return -ENOSYS;
> >  	if (!XFS_IS_QUOTA_ON(mp))
> >  		return -ESRCH;
> >  
> > +	eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
> > +	ret = xfs_icache_free_cowblocks(mp, &eofb);
> > +	if (ret)
> > +		return ret;
> > +
> >  	id = from_kqid(&init_user_ns, qid);
> >  	return xfs_qm_scall_getquota(mp, id, xfs_quota_type(qid.type), qdq);
> >  }
> > @@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(
> >  	int			ret;
> >  	struct xfs_mount	*mp = XFS_M(sb);
> >  	xfs_dqid_t		id;
> > +	struct xfs_eofblocks	eofb = { 0 };
> >  
> >  	if (!XFS_IS_QUOTA_RUNNING(mp))
> >  		return -ENOSYS;
> >  	if (!XFS_IS_QUOTA_ON(mp))
> >  		return -ESRCH;
> >  
> > +	eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
> > +	ret = xfs_icache_free_cowblocks(mp, &eofb);
> > +	if (ret)
> > +		return ret;
> > +
> >  	id = from_kqid(&init_user_ns, *qid);
> >  	ret = xfs_qm_scall_getquota_next(mp, &id, xfs_quota_type(qid->type),
> >  			qdq);
> 
> 


-- 
chandan