On Thu, Feb 09, 2017 at 02:34:53PM -0500, Brian Foster wrote:
> Certain workoads that punch holes into speculative preallocation can
> cause delalloc indirect reservation splits when the delalloc extent is
> split in two. If further splits occur, an already short-handed extent
> can be split into two in a manner that leaves zero indirect blocks for
> one of the two new extents. This occurs because the shortage is large
> enough that the xfs_bmap_split_indlen() algorithm completely drains the
> requested indlen of one of the extents before it honors the existing
> reservation.
>
> This ultimately results in a warning from xfs_bmap_del_extent(). This
> has been observed during file copies of large, sparse files using 'cp
> --sparse=always.'
>
> To avoid this problem, update xfs_bmap_split_indlen() to explicitly
> apply the reservation shortage fairly between both extents. This smooths
> out the overall indlen shortage and defers the situation where we end up
> with a delalloc extent with zero indlen reservation to extreme
> circumstances.
>
> Reported-by: Patrick Dung <mpatdung@xxxxxxxxx>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 61 ++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 49edea4..e27b9ee 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4790,6 +4790,7 @@ xfs_bmap_split_indlen(
> xfs_filblks_t len2 = *indlen2;
> xfs_filblks_t nres = len1 + len2; /* new total res. */
> xfs_filblks_t stolen = 0;
> + xfs_filblks_t resfactor;
>
> trace_printk("%d: ores %llu len1 %llu len2 %llu\n", __LINE__, ores, len1, len2);
>
> @@ -4797,29 +4798,53 @@ xfs_bmap_split_indlen(
> * Steal as many blocks as we can to try and satisfy the worst case
> * indlen for both new extents.
> */
> - while (nres > ores && avail) {
> - nres--;
> - avail--;
> - stolen++;
> - }
> + if (ores < nres && avail)
> + stolen = XFS_FILBLKS_MIN(nres - ores, avail);
> + ores += stolen;
> +
> + /* nothing else to do if we've satisfied the new reservation */
> + if (ores >= nres)
> + return stolen;
> +
> + /*
> + * We can't meet the total required reservation for the two extents.
> + * Calculate the percent of the overall shortage between both extents
> + * and apply this percentage to each of the requested indlen values.
> + * This distributes the shortage fairly and reduces the chances that one
> + * of the two extents is left with nothing when extents are repeatedly
> + * split.
> + */
> + resfactor = (ores * 100);
> + do_div(resfactor, nres);
> + len1 *= resfactor;
> + do_div(len1, 100);
> + len2 *= resfactor;
> + do_div(len2, 100);
/me wonders if it's worth saving ourselves two 64bit divisions by changing
the 100 to 128 and shifting? Probably not.
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> + ASSERT(len1 + len2 <= ores);
> + ASSERT(len1 < *indlen1 && len2 < *indlen2);
>
> /*
> - * The only blocks available are those reserved for the original
> - * extent and what we can steal from the extent being removed.
> - * If this still isn't enough to satisfy the combined
> - * requirements for the two new extents, skim blocks off of each
> - * of the new reservations until they match what is available.
> + * Hand out the remainder to each extent. If one of the two reservations
> + * is zero, we want to make sure that one gets a block first. The loop
> + * below starts with len1, so hand len2 a block right off the bat if it
> + * is zero.
> */
> - while (nres > ores) {
> - if (len1) {
> - len1--;
> - nres--;
> + ores -= (len1 + len2);
> + ASSERT((*indlen1 - len1) + (*indlen2 - len2) >= ores);
> + if (ores && !len2 && *indlen2) {
> + len2++;
> + ores--;
> + }
> + while (ores) {
> + if (len1 < *indlen1) {
> + len1++;
> + ores--;
> }
> - if (nres == ores)
> + if (!ores)
> break;
> - if (len2) {
> - len2--;
> - nres--;
> + if (len2 < *indlen2) {
> + len2++;
> + ores--;
> }
> }
>
> --
> 2.7.4
>
> --
> 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
