fix up indlen reservations on extent split
cover fix up indlen reservations on extent split
Hi all,
Here's a couple patches to fix up the indirect block reservation problem when splitting delalloc extents, described in more detail in the patch 2 commit log.
This runs through xfstests without any explosions and quiets down generic/033 when used in combination with the zero range rework (current for-next includes the writeback on zero range workaround that also prevents generic/033 asserts).
Brian
v1:
- xfs_bunmapi() code into independent patch.
- Refactor fix into separate helper function.
rfc: http://oss.sgi.com/archives/xfs/2014-09/msg00337.html
Brian Foster (2):
xfs: update icsb freeblocks counter after extent deletion
xfs: borrow indirect blocks from freed extent when available
fs/xfs/libxfs/xfs_bmap.c | 159 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 116 insertions(+), 43 deletions(-)
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/2 xfs: update icsb freeblocks counter after extent deletion
xfs_bunmapi() currently updates the icsb fdblocks counter, unreserves quota, etc. before the extent is deleted by xfs_bmap_del_extent(). The function has problems dividing up the indirect reserved blocks for scenarios where a single delalloc extent is split in two. Particularly, there aren't always enough blocks reserved for multiple extents in a single extent reservation.
The solution to this problem is to allow the extent removal code to steal from the deleted extent to meet indirect reservation requirements. Move the block of code in xfs_bmapi() that updates the icsb fdblocks counter to after the call to xfs_bmap_del_extent() to allow the codepath to update the extent record before the free blocks are accounted. Also, reshuffle the code slightly so the delalloc accounting occurs near the xfs_bmap_del_extent() call to provide context for the comments.
--- fs/xfs/libxfs/xfs_bmap.c | 59 ++++++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 24 deletions(-)
fs/xfs/libxfs/xfs_bmap.c +35 -24
@@ -5277,9 +5277,37 @@ xfs_bunmapi( goto nodelete; } } + +/* +* If it's the case where the directory code is running +* with no block reservation, and the deleted block is in +* the middle of its extent, and the resulting insert +* of an extent would cause transformation to btree format, +* then reject it. The calling code will then swap +* blocks around instead. +* We have to do this now, rather than waiting for the +* conversion to btree format, since the transaction +* will be dirty. +*/ +if (!wasdel &&xfs_trans_get_block_res (tp) ==0 && +XFS_IFORK_FORMAT (ip, whichfork) ==XFS_DINODE_FMT_EXTENTS && +XFS_IFORK_NEXTENTS (ip, whichfork) >=/* Note the >= */ +XFS_IFORK_MAXEXT (ip, whichfork) && + del.br_startoff > got.br_startoff && + del.br_startoff + del.br_blockcount < + got.br_startoff + got.br_blockcount) { + error = -ENOSPC ; + goto error0; + } + +/* +* Unreserve quota and update realtime free space, if +* appropriate. If delayed allocation, update the inode delalloc +* counter now and wait to update the sb counters as +* xfs_bmap_del_extent() might need to borrow some blocks. +*/ if (wasdel) {ASSERT (startblockval (del.br_startblock) >0 ); -/* Update realtime/data freespace, unreserve quota */ if (isrt) {xfs_filblks_t rtexts;
@@ -5291,8 +5319,6 @@ xfs_bunmapi( ip, -((long )del.br_blockcount),0 , XFS_QMOPT_RES_RTBLKS); }else { -xfs_icsb_modify_counters (mp,XFS_SBS_FDBLOCKS , - (int64_t )del.br_blockcount,0 ); (void )xfs_trans_reserve_quota_nblks (NULL , ip, -((long )del.br_blockcount),0 ,XFS_QMOPT_RES_REGBLKS );
@@ -5303,32 +5329,17 @@ xfs_bunmapi(XFS_BTCUR_BPRV_WASDEL ; }else if (cur) cur->bc_private.b.flags &= ~XFS_BTCUR_BPRV_WASDEL ; -/* -* If it's the case where the directory code is running -* with no block reservation, and the deleted block is in -* the middle of its extent, and the resulting insert -* of an extent would cause transformation to btree format, -* then reject it. The calling code will then swap -* blocks around instead. -* We have to do this now, rather than waiting for the -* conversion to btree format, since the transaction -* will be dirty. -*/ -if (!wasdel &&xfs_trans_get_block_res (tp) ==0 && -XFS_IFORK_FORMAT (ip, whichfork) ==XFS_DINODE_FMT_EXTENTS && -XFS_IFORK_NEXTENTS (ip, whichfork) >=/* Note the >= */ -XFS_IFORK_MAXEXT (ip, whichfork) && - del.br_startoff > got.br_startoff && - del.br_startoff + del.br_blockcount < - got.br_startoff + got.br_blockcount) { - error = -ENOSPC ; - goto error0; - } + error =xfs_bmap_del_extent (ip, tp, &lastx, flist, cur, &del, &tmp_logflags, whichfork); logflags |= tmp_logflags;if (error) goto error0; + +if (!isrt && wasdel) +xfs_icsb_modify_counters (mp,XFS_SBS_FDBLOCKS , + (int64_t ) del.br_blockcount,0 ); + bno = del.br_startoff -1 ; nodelete:/*
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/2 xfs: borrow indirect blocks from freed extent when available
In reply to Brian Fosterxfs_bmap_del_extent() handles extent removal from the in-core and on-disk extent lists. When removing a delalloc range, it updates the indirect block reservation appropriately based on the removal. It currently enforces that the new indirect block reservation is less than or equal to the original. This is normally the case in all situations except for in certain cases when the removed range creates a hole in a single delalloc extent, thus splitting a single delalloc extent in two.
It is possible with small enough extents to split an indlen==1 extent into two such slightly smaller extents. This leaves one extent with 0 indirect blocks and leads to assert failures in other areas (e.g., xfs_bunmapi() if the extent happens to be removed).
Refactor the indirect reservation code into a new helper function invoked by xfs_bunmapi(). Allow the helper to steal blocks from the deleted extent if necessary to satisfy the worst case indirect reservation of the new extents. This is safe now that the caller does not update icsb counters until the extent is removed (such stolen blocks simply remain accounted as allocated). Fall back to the original reservation using the existing algorithm and warn if we end up with extents without any reservation at all to detect this more easily in the future.
This allows generic/033 to run on XFS without assert failures.
--- fs/xfs/libxfs/xfs_bmap.c | 100 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 19 deletions(-)
fs/xfs/libxfs/xfs_bmap.c +81 -19
@@ -4701,6 +4701,68 @@ error0: }/* +* When a delalloc extent is split (e.g., due to a hole punch), the original +* indlen reservation must be shared across the two new extents that are left +* behind. +* +* Provided the worst case indlen for two extents, limit the total reservation +* against that of the original extent. We may want to steal blocks from any +* extra that might be available according to the caller to make up a deficiency +* (e.g., if the original reservation consists of one block). The number of such +* stolen blocks is returned. The availability and accounting of stealable +* blocks is the responsibility of the caller. +*/ +static xfs_filblks_t +xfs_bmap_limit_indlen ( +xfs_filblks_t ores,/* original res. */ +xfs_filblks_t *indlen1,/* ext1 worst indlen */ +xfs_filblks_t *indlen2,/* ext2 worst indlen */ +xfs_filblks_t avail)/* stealable blocks */ +{ +xfs_filblks_t nres;/* new total res. */ +xfs_filblks_t temp; +xfs_filblks_t temp2; +xfs_filblks_t stolen =0 ; + + temp = *indlen1; + temp2 = *indlen2; + nres = temp + temp2; + +/* +* Steal as many blocks as we can to try and satisfy the worst case +* indlen of both new extents. +*/ +while (nres > ores && avail) { + nres--; + avail--; + stolen++; + } + +/* +* If we've exhausted stealable blocks and still haven't satisfied the +* worst case reservation, we have no choice but to pare back until +* covered by the original reservation. +*/ +while (nres > ores) { +if (temp) { + temp--; + nres--; + } +if (nres == ores) +break ; +if (temp2) { + temp2--; + nres--; + } + } + + *indlen1 = temp; + *indlen2 = temp2; + +return stolen; +} + +/* * Called by xfs_bmapi to update file extent records and the btree * after removing space (or undoing a delayed allocation). */
@@ -4963,28 +5025,28 @@ xfs_bmap_del_extent(XFS_IFORK_NEXT_SET (ip, whichfork,XFS_IFORK_NEXTENTS (ip, whichfork) +1 ); }else { +xfs_filblks_t stolen;ASSERT (whichfork ==XFS_DATA_FORK ); - temp =xfs_bmap_worst_indlen (ip, temp); + +/* +* Fix up the indlen reservations. We can safely steal +* blocks from the deleted extent as this simply fudges +* the icsb fdblocks accounting in xfs_bunmapi(). +*/ + temp =xfs_bmap_worst_indlen (ip, got.br_blockcount); + temp2 =xfs_bmap_worst_indlen (ip, new.br_blockcount); + stolen =xfs_bmap_limit_indlen (da_old, &temp, &temp2, + del->br_blockcount); + da_new = temp + temp2 - stolen; + del->br_blockcount -= stolen; + +/* +* Set the reservation for each extent. Warn if either +* is zero as this can lead to delalloc problems. +*/ +WARN_ON_ONCE (!temp || !temp2);xfs_bmbt_set_startblock (ep,nullstartblock ((int )temp)); - temp2 =xfs_bmap_worst_indlen (ip, temp2); new.br_startblock =nullstartblock ((int )temp2); - da_new = temp + temp2; -while (da_new > da_old) { -if (temp) { - temp--; - da_new--; -xfs_bmbt_set_startblock (ep, -nullstartblock ((int )temp)); - } -if (da_new == da_old) -break ; -if (temp2) { - temp2--; - da_new--; - new.br_startblock = -nullstartblock ((int )temp2); - } - } }trace_xfs_bmap_post_update (ip, *idx, state, _THIS_IP_);xfs_iext_insert (ip, *idx +1 ,1 , &new, state);
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Brian Foster
Hi all,
Here's a couple patches to fix up the indirect block reservation problem when splitting delalloc extents, described in more detail in the patch 2 commit log.
This runs through xfstests without any explosions and quiets down generic/033 when used in combination with the zero range rework (current for-next includes the writeback on zero range workaround that also prevents generic/033 asserts).
Quoted 7 lines
Brian
v1:
-xfs_bunmapi()code into independent patch.
- Refactor fix into separate helper function.
rfc: http://oss.sgi.com/archives/xfs/2014-09/msg00337.html
ping?
Quoted 4 lines
Brian Foster (2):
xfs: update icsb freeblocks counter after extent deletion
xfs: borrow indirect blocks from freed extent when available
Quoted 10 lines
fs/xfs/libxfs/xfs_bmap.c | 159 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 116 insertions(+), 43 deletions(-)Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 14 lines · Brian Foster
Quoted 13 lines · Brian Foster
Hi all,
Here's a couple patches to fix up the indirect block reservation problem when splitting delalloc extents, described in more detail in the patch 2 commit log.
This runs through xfstests without any explosions and quiets down generic/033 when used in combination with the zero range rework (current for-next includes the writeback on zero range workaround that also prevents generic/033 asserts).
Brian
Quoted 7 lines
Quoted 5 lines
v1:
-xfs_bunmapi()code into independent patch.
- Refactor fix into separate helper function.
rfc: http://oss.sgi.com/archives/xfs/2014-09/msg00337.htmlping?
In my list of things to look at once I've got the bulkstat problems sorted out.
-Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs