On Fri, Feb 03, 2017 at 01:34:25PM -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 looks ok from a "try to avoid the superbadness of ending up with
zero indlen blocks" point of view, but I've been wondering -- how does
punching holes in da reservations result in the reservations being
shorthanded?
Is the situation here something like: we create a reservation with
indlen=14 blocks, punch a one-block hole, and now we have two
reservations each wanting indlen=8 so now we're under-reserved?
If so, is there a way to fix that?
> 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.'
Is there a test case to reproduce this? I ran this patch through the
auto group with no ill effects but I've never seen that assert trigger
while running xfstests so it's harder to tell that the fix is actually
working.
> 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.
Could you elaborate on what are 'extreme circumstances' here?
(FWIW I'm not opposed to the two patches, but my curiosity isn't yet
satisfied.)
--D
> Reported-by: Patrick Dung <mpatdung@xxxxxxxxx>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 58 +++++++++++++++++++++++++++++++++---------------
> 1 file changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index d2e48ed..4ca4606 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4790,34 +4790,56 @@ 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;
>
> /*
> * 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) / nres;
> + len1 = resfactor * len1 / 100;
> + len2 = resfactor * len2 / 100;
> + 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
