> @@ -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.
> + 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.
