Re: [PATCH 06/11] xfs: flush eof/cowblocks if we can't reserve quota for file blocks

From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 06/11] xfs: flush eof/cowblocks if we can't reserve quota for file blocks
Date: Tue, 19 Jan 2021 11:43:37 -0800	[thread overview]
Message-ID: <20210119194337.GQ3134581@magnolia> (raw)
In-Reply-To: <YAaFfUxZjXAabnoV@infradead.org>

On Tue, Jan 19, 2021 at 08:08:45AM +0100, Christoph Hellwig wrote:
> > @@ -351,13 +351,14 @@ xfs_reflink_allocate_cow(
> >  	bool			convert_now)
> >  {
> >  	struct xfs_mount	*mp = ip->i_mount;
> > +	struct xfs_trans	*tp;
> >  	xfs_fileoff_t		offset_fsb = imap->br_startoff;
> >  	xfs_filblks_t		count_fsb = imap->br_blockcount;
> > -	struct xfs_trans	*tp;
> > -	int			nimaps, error = 0;
> > -	bool			found;
> >  	xfs_filblks_t		resaligned;
> >  	xfs_extlen_t		resblks = 0;
> > +	bool			found;
> > +	bool			quota_retry = false;
> > +	int			nimaps, error = 0;
> 
> Any good reason for reshuffling the declarations here?
> 
> > +	if (error) {
> > +		/* This function must return with the ILOCK held. */
> > +		xfs_ilock(ip, *lockmode);
> > +		return error;
> > +	}
> 
> Ugg.

Yeah.  I can't think of a good (as in non-brain-straining) way to fix
this unusual locking -- this function can be called with the ILOCK held,
and it's possible that we then find what we are looking for due to a
speculative preallocation and can exit without cycling the lock.

I think what we really want is for xfs_direct_write_iomap_begin to call
xfs_find_trim_cow_extent and xfs_reflink_convert_cow_locked directly,
and if the first call doesn't find a cow staging extent then drop the
ILOCK, call xfs_reflink_allocate_cow, and re-take the ILOCK after.

> > +	if (error) {
> > +		xfs_trans_cancel(*tpp);
> > +		*tpp = NULL;
> > +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +	}
> > +
> > +	/* We only allow one retry for EDQUOT/ENOSPC. */
> > +	if (*retry || (error != -EDQUOT && error != -ENOSPC)) {
> > +		*retry = false;
> > +		return error;
> > +	}
> 
> Id really don't like the semantics where this wrapper unlocks the
> ilock.  Keeping all the locking at one layer, which is the callers
> makes the code much easier to reason about
> 
> > +
> > +	/* Try to free some quota for this file's dquots. */
> > +	err2 = xfs_blockgc_free_quota(ip, 0, retry);
> > +	if (err2)
> > +		return err2;
> > +	return *retry ? 0 : error;
> >  }
> 
> Why not have a should_retry helper for the callers and let them call
> xfs_blockgc_free_quota?  That is a little more boilerplate code, but
> a lot less obsfucated.

The previous version of this patchset did that, but Dave complained that
spread the retry calls and error/retry branching all over the code base
and asked for the structure that's in this version:

https://lore.kernel.org/linux-xfs/161040735389.1582114.15084485390769234805.stgit@magnolia/T/#mfcd6786f99791adf771697416fc51d168d3050f8

--D