| Message ID | 20170918152422.24345-13-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Sep 18, 2017 at 08:24:15AM -0700, Christoph Hellwig wrote: > Use xfs_iext_get_extent to find, and xfs_iext_update_extent to update > entries in the in-core extent list. This isolates the function from > the detailed layout of the extent list, and generally makes the code > a lot more readable. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_bmap.c | 181 +++++++++++++++++++++++++---------------------- > 1 file changed, 95 insertions(+), 86 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 4fb73c0aca05..731854cc8b58 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c ... > @@ -2064,12 +2082,12 @@ xfs_bmap_add_extent_delay_real( > if (error) > goto done; > } > - temp = xfs_bmap_worst_indlen(bma->ip, temp); > - temp2 = xfs_bmap_worst_indlen(bma->ip, temp2); > - diff = (int)(temp + temp2 - > - (startblockval(PREV.br_startblock) - > - (bma->cur ? > - bma->cur->bc_private.b.allocated : 0))); > + > + da_new = startblockval(PREV.br_blockcount) + > + startblockval(RIGHT.br_blockcount); s/br_blockcount/br_startblock/ :) Brian > + diff = da_new - startblockval(old.br_startblock); > + if (bma->cur) > + diff += bma->cur->bc_private.b.allocated; > if (diff > 0) { > error = xfs_mod_fdblocks(bma->ip->i_mount, > -((int64_t)diff), false); > @@ -2078,16 +2096,7 @@ xfs_bmap_add_extent_delay_real( > goto done; > } > > - ep = xfs_iext_get_ext(ifp, bma->idx); > - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp)); > - trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > - trace_xfs_bmap_pre_update(bma->ip, bma->idx + 2, state, _THIS_IP_); > - xfs_bmbt_set_startblock(xfs_iext_get_ext(ifp, bma->idx + 2), > - nullstartblock((int)temp2)); > - trace_xfs_bmap_post_update(bma->ip, bma->idx + 2, state, _THIS_IP_); > - > bma->idx++; > - da_new = temp + temp2; > break; > > case BMAP_LEFT_FILLING | BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG: > -- > 2.14.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 18, 2017 at 08:24:15AM -0700, Christoph Hellwig wrote: > Use xfs_iext_get_extent to find, and xfs_iext_update_extent to update > entries in the in-core extent list. This isolates the function from > the detailed layout of the extent list, and generally makes the code > a lot more readable. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_bmap.c | 181 +++++++++++++++++++++++++---------------------- > 1 file changed, 95 insertions(+), 86 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 4fb73c0aca05..731854cc8b58 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -1588,7 +1588,6 @@ xfs_bmap_add_extent_delay_real( > { > struct xfs_bmbt_irec *new = &bma->got; > int diff; /* temp value */ > - xfs_bmbt_rec_host_t *ep; /* extent entry for idx */ > int error; /* error return value */ > int i; /* temp state */ > xfs_ifork_t *ifp; /* inode fork pointer */ > @@ -1600,10 +1599,10 @@ xfs_bmap_add_extent_delay_real( > xfs_filblks_t da_new; /* new count del alloc blocks used */ > xfs_filblks_t da_old; /* old count del alloc blocks used */ > xfs_filblks_t temp=0; /* value for da_new calculations */ > - xfs_filblks_t temp2=0;/* value for da_new calculations */ > int tmp_rval; /* partial logging flags */ > struct xfs_mount *mp; > xfs_extnum_t *nextents; > + struct xfs_bmbt_irec old; > > mp = bma->ip->i_mount; > ifp = XFS_IFORK_PTR(bma->ip, whichfork); > @@ -1629,9 +1628,9 @@ xfs_bmap_add_extent_delay_real( > /* > * Set up a bunch of variables to make the tests simpler. > */ > - ep = xfs_iext_get_ext(ifp, bma->idx); > - xfs_bmbt_get_all(ep, &PREV); > + xfs_iext_get_extent(ifp, bma->idx, &PREV); > new_endoff = new->br_startoff + new->br_blockcount; > + ASSERT(isnullstartblock(PREV.br_startblock)); > ASSERT(PREV.br_startoff <= new->br_startoff); > ASSERT(PREV.br_startoff + PREV.br_blockcount >= new_endoff); > > @@ -1706,9 +1705,8 @@ xfs_bmap_add_extent_delay_real( > */ > bma->idx--; > trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx), > - LEFT.br_blockcount + PREV.br_blockcount + > - RIGHT.br_blockcount); > + LEFT.br_blockcount += PREV.br_blockcount + RIGHT.br_blockcount; > + xfs_iext_update_extent(ifp, bma->idx, &LEFT); > trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > xfs_iext_remove(bma->ip, bma->idx + 1, 2, state); > @@ -1733,9 +1731,7 @@ xfs_bmap_add_extent_delay_real( > XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); > error = xfs_bmbt_update(bma->cur, LEFT.br_startoff, > LEFT.br_startblock, > - LEFT.br_blockcount + > - PREV.br_blockcount + > - RIGHT.br_blockcount, LEFT.br_state); > + LEFT.br_blockcount, LEFT.br_state); > if (error) > goto done; > } > @@ -1748,9 +1744,10 @@ xfs_bmap_add_extent_delay_real( > */ > bma->idx--; > > + old = LEFT; > trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx), > - LEFT.br_blockcount + PREV.br_blockcount); > + LEFT.br_blockcount += PREV.br_blockcount; > + xfs_iext_update_extent(ifp, bma->idx, &LEFT); > trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > xfs_iext_remove(bma->ip, bma->idx + 1, 1, state); > @@ -1758,16 +1755,15 @@ xfs_bmap_add_extent_delay_real( > rval = XFS_ILOG_DEXT; > else { > rval = 0; > - error = xfs_bmbt_lookup_eq(bma->cur, LEFT.br_startoff, > - LEFT.br_startblock, LEFT.br_blockcount, > + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff, > + old.br_startblock, old.br_blockcount, > &i); > if (error) > goto done; > XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); > error = xfs_bmbt_update(bma->cur, LEFT.br_startoff, > LEFT.br_startblock, > - LEFT.br_blockcount + > - PREV.br_blockcount, LEFT.br_state); > + LEFT.br_blockcount, LEFT.br_state); > if (error) > goto done; > } > @@ -1779,9 +1775,9 @@ xfs_bmap_add_extent_delay_real( > * The right neighbor is contiguous, the left is not. > */ > trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_startblock(ep, new->br_startblock); > - xfs_bmbt_set_blockcount(ep, > - PREV.br_blockcount + RIGHT.br_blockcount); > + PREV.br_startblock = new->br_startblock; > + PREV.br_blockcount += RIGHT.br_blockcount; > + xfs_iext_update_extent(ifp, bma->idx, &PREV); > trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > xfs_iext_remove(bma->ip, bma->idx + 1, 1, state); > @@ -1796,9 +1792,8 @@ xfs_bmap_add_extent_delay_real( > goto done; > XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); > error = xfs_bmbt_update(bma->cur, PREV.br_startoff, > - new->br_startblock, > - PREV.br_blockcount + > - RIGHT.br_blockcount, PREV.br_state); > + PREV.br_startblock, > + PREV.br_blockcount, PREV.br_state); > if (error) > goto done; > } > @@ -1811,8 +1806,9 @@ xfs_bmap_add_extent_delay_real( > * the new one. > */ > trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_startblock(ep, new->br_startblock); > - xfs_bmbt_set_state(ep, new->br_state); > + PREV.br_startblock = new->br_startblock; > + PREV.br_state = new->br_state; > + xfs_iext_update_extent(ifp, bma->idx, &PREV); > trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > (*nextents)++; > @@ -1839,38 +1835,39 @@ xfs_bmap_add_extent_delay_real( > * Filling in the first part of a previous delayed allocation. > * The left neighbor is contiguous. > */ > + old = LEFT; > + temp = PREV.br_blockcount - new->br_blockcount; > + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > + startblockval(PREV.br_startblock)); Needs another indent? (Also the blockcount/startblock thing that Brian already pointed out.) --D > + > trace_xfs_bmap_pre_update(bma->ip, bma->idx - 1, state, _THIS_IP_); > - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx - 1), > - LEFT.br_blockcount + new->br_blockcount); > - xfs_bmbt_set_startoff(ep, > - PREV.br_startoff + new->br_blockcount); > + LEFT.br_blockcount += new->br_blockcount; > + xfs_iext_update_extent(ifp, bma->idx - 1, &LEFT); > trace_xfs_bmap_post_update(bma->ip, bma->idx - 1, state, _THIS_IP_); > > - temp = PREV.br_blockcount - new->br_blockcount; > trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_blockcount(ep, temp); > + PREV.br_blockcount = temp = PREV.br_blockcount - new->br_blockcount; > + PREV.br_startoff += new->br_blockcount; > + PREV.br_startblock = nullstartblock(da_new); > + xfs_iext_update_extent(ifp, bma->idx, &PREV); > + trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > + > if (bma->cur == NULL) > rval = XFS_ILOG_DEXT; > else { > rval = 0; > - error = xfs_bmbt_lookup_eq(bma->cur, LEFT.br_startoff, > - LEFT.br_startblock, LEFT.br_blockcount, > + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff, > + old.br_startblock, old.br_blockcount, > &i); > if (error) > goto done; > XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); > error = xfs_bmbt_update(bma->cur, LEFT.br_startoff, > - LEFT.br_startblock, > - LEFT.br_blockcount + > - new->br_blockcount, > + LEFT.br_startblock, LEFT.br_blockcount, > LEFT.br_state); > if (error) > goto done; > } > - da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > - startblockval(PREV.br_startblock)); > - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); > - trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > bma->idx--; > break; > @@ -1880,10 +1877,6 @@ xfs_bmap_add_extent_delay_real( > * Filling in the first part of a previous delayed allocation. > * The left neighbor is not contiguous. > */ > - trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_startoff(ep, new_endoff); > - temp = PREV.br_blockcount - new->br_blockcount; > - xfs_bmbt_set_blockcount(ep, temp); > xfs_iext_insert(bma->ip, bma->idx, 1, new, state); > (*nextents)++; > if (bma->cur == NULL) > @@ -1911,12 +1904,19 @@ xfs_bmap_add_extent_delay_real( > if (error) > goto done; > } > + > + temp = PREV.br_blockcount - new->br_blockcount; > da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > startblockval(PREV.br_startblock) - > (bma->cur ? bma->cur->bc_private.b.allocated : 0)); > - ep = xfs_iext_get_ext(ifp, bma->idx + 1); > - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); > + > + trace_xfs_bmap_pre_update(bma->ip, bma->idx + 1, state, _THIS_IP_); > + PREV.br_startoff = new_endoff; > + PREV.br_blockcount = temp; > + PREV.br_startblock = nullstartblock(da_new); > + xfs_iext_update_extent(ifp, bma->idx + 1, &PREV); > trace_xfs_bmap_post_update(bma->ip, bma->idx + 1, state, _THIS_IP_); > + > break; > > case BMAP_RIGHT_FILLING | BMAP_RIGHT_CONTIG: > @@ -1924,37 +1924,39 @@ xfs_bmap_add_extent_delay_real( > * Filling in the last part of a previous delayed allocation. > * The right neighbor is contiguous with the new allocation. > */ > - temp = PREV.br_blockcount - new->br_blockcount; > + old = RIGHT; > trace_xfs_bmap_pre_update(bma->ip, bma->idx + 1, state, _THIS_IP_); > - xfs_bmbt_set_blockcount(ep, temp); > - xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, bma->idx + 1), > - new->br_startoff, new->br_startblock, > - new->br_blockcount + RIGHT.br_blockcount, > - RIGHT.br_state); > + RIGHT.br_startoff = new->br_startoff; > + RIGHT.br_startblock = new->br_startblock; > + RIGHT.br_blockcount += new->br_blockcount; > + xfs_iext_update_extent(ifp, bma->idx + 1, &RIGHT); > trace_xfs_bmap_post_update(bma->ip, bma->idx + 1, state, _THIS_IP_); > + > if (bma->cur == NULL) > rval = XFS_ILOG_DEXT; > else { > rval = 0; > - error = xfs_bmbt_lookup_eq(bma->cur, RIGHT.br_startoff, > - RIGHT.br_startblock, > - RIGHT.br_blockcount, &i); > + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff, > + old.br_startblock, > + old.br_blockcount, &i); > if (error) > goto done; > XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); > - error = xfs_bmbt_update(bma->cur, new->br_startoff, > - new->br_startblock, > - new->br_blockcount + > - RIGHT.br_blockcount, > + error = xfs_bmbt_update(bma->cur, RIGHT.br_startoff, > + RIGHT.br_startblock, RIGHT.br_blockcount, > RIGHT.br_state); > if (error) > goto done; > } > > + temp = PREV.br_blockcount - new->br_blockcount; > da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > startblockval(PREV.br_startblock)); > + > trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); > + PREV.br_blockcount = temp; > + PREV.br_startblock = nullstartblock(da_new); > + xfs_iext_update_extent(ifp, bma->idx, &PREV); > trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > bma->idx++; > @@ -1965,9 +1967,6 @@ xfs_bmap_add_extent_delay_real( > * Filling in the last part of a previous delayed allocation. > * The right neighbor is not contiguous. > */ > - temp = PREV.br_blockcount - new->br_blockcount; > - trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > - xfs_bmbt_set_blockcount(ep, temp); > xfs_iext_insert(bma->ip, bma->idx + 1, 1, new, state); > (*nextents)++; > if (bma->cur == NULL) > @@ -1995,11 +1994,16 @@ xfs_bmap_add_extent_delay_real( > if (error) > goto done; > } > + > + temp = PREV.br_blockcount - new->br_blockcount; > da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > startblockval(PREV.br_startblock) - > (bma->cur ? bma->cur->bc_private.b.allocated : 0)); > - ep = xfs_iext_get_ext(ifp, bma->idx); > - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); > + > + trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); > + PREV.br_startblock = nullstartblock(da_new); > + PREV.br_blockcount = temp; > + xfs_iext_update_extent(ifp, bma->idx, &PREV); > trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > > bma->idx++; > @@ -2026,19 +2030,33 @@ xfs_bmap_add_extent_delay_real( > * PREV @ idx LEFT RIGHT > * inserted at idx + 1 > */ > - temp = new->br_startoff - PREV.br_startoff; > - temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff; > - trace_xfs_bmap_pre_update(bma->ip, bma->idx, 0, _THIS_IP_); > - xfs_bmbt_set_blockcount(ep, temp); /* truncate PREV */ > + old = PREV; > + > + /* LEFT is the new middle */ > LEFT = *new; > + > + /* RIGHT is the new right */ > RIGHT.br_state = PREV.br_state; > - RIGHT.br_startblock = nullstartblock( > - (int)xfs_bmap_worst_indlen(bma->ip, temp2)); > RIGHT.br_startoff = new_endoff; > - RIGHT.br_blockcount = temp2; > + RIGHT.br_blockcount = > + PREV.br_startoff + PREV.br_blockcount - new_endoff; > + RIGHT.br_startblock = > + nullstartblock(xfs_bmap_worst_indlen(bma->ip, > + RIGHT.br_blockcount)); > + > + /* truncate PREV */ > + trace_xfs_bmap_pre_update(bma->ip, bma->idx, 0, _THIS_IP_); > + PREV.br_blockcount = new->br_startoff - PREV.br_startoff; > + PREV.br_startblock = > + nullstartblock(xfs_bmap_worst_indlen(bma->ip, > + PREV.br_blockcount)); > + xfs_iext_update_extent(ifp, bma->idx, &PREV); > + trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > + > /* insert LEFT (r[0]) and RIGHT (r[1]) at the same time */ > xfs_iext_insert(bma->ip, bma->idx + 1, 2, &LEFT, state); > (*nextents)++; > + > if (bma->cur == NULL) > rval = XFS_ILOG_CORE | XFS_ILOG_DEXT; > else { > @@ -2064,12 +2082,12 @@ xfs_bmap_add_extent_delay_real( > if (error) > goto done; > } > - temp = xfs_bmap_worst_indlen(bma->ip, temp); > - temp2 = xfs_bmap_worst_indlen(bma->ip, temp2); > - diff = (int)(temp + temp2 - > - (startblockval(PREV.br_startblock) - > - (bma->cur ? > - bma->cur->bc_private.b.allocated : 0))); > + > + da_new = startblockval(PREV.br_blockcount) + > + startblockval(RIGHT.br_blockcount); > + diff = da_new - startblockval(old.br_startblock); > + if (bma->cur) > + diff += bma->cur->bc_private.b.allocated; > if (diff > 0) { > error = xfs_mod_fdblocks(bma->ip->i_mount, > -((int64_t)diff), false); > @@ -2078,16 +2096,7 @@ xfs_bmap_add_extent_delay_real( > goto done; > } > > - ep = xfs_iext_get_ext(ifp, bma->idx); > - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp)); > - trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); > - trace_xfs_bmap_pre_update(bma->ip, bma->idx + 2, state, _THIS_IP_); > - xfs_bmbt_set_startblock(xfs_iext_get_ext(ifp, bma->idx + 2), > - nullstartblock((int)temp2)); > - trace_xfs_bmap_post_update(bma->ip, bma->idx + 2, state, _THIS_IP_); > - > bma->idx++; > - da_new = temp + temp2; > break; > > case BMAP_LEFT_FILLING | BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG: > -- > 2.14.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 19, 2017 at 12:35:25PM -0400, Brian Foster wrote: > > @@ -2064,12 +2082,12 @@ xfs_bmap_add_extent_delay_real( > > if (error) > > goto done; > > } > > - temp = xfs_bmap_worst_indlen(bma->ip, temp); > > - temp2 = xfs_bmap_worst_indlen(bma->ip, temp2); > > - diff = (int)(temp + temp2 - > > - (startblockval(PREV.br_startblock) - > > - (bma->cur ? > > - bma->cur->bc_private.b.allocated : 0))); > > + > > + da_new = startblockval(PREV.br_blockcount) + > > + startblockval(RIGHT.br_blockcount); > > s/br_blockcount/br_startblock/ :) Yes. And I've officially lost all faith in xfstests ever even testing this case in xfs_bmap_add_extent_delay_real at all. I think it should be really easily testable by creating a large delalloc reservation and then fsyncing out the middle of it. Except of course we don't have a range fsync, and even then the writeback code might cluster it. I might have to come up with a special kernel module to even reproduce this reliably.. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[fullquote of a fullquote removed, sigh..] > > + temp = PREV.br_blockcount - new->br_blockcount; > > + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > > + startblockval(PREV.br_startblock)); > > Needs another indent? What for? -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 21, 2017 at 03:27:35PM +0200, Christoph Hellwig wrote: > On Tue, Sep 19, 2017 at 12:35:25PM -0400, Brian Foster wrote: > > > @@ -2064,12 +2082,12 @@ xfs_bmap_add_extent_delay_real( > > > if (error) > > > goto done; > > > } > > > - temp = xfs_bmap_worst_indlen(bma->ip, temp); > > > - temp2 = xfs_bmap_worst_indlen(bma->ip, temp2); > > > - diff = (int)(temp + temp2 - > > > - (startblockval(PREV.br_startblock) - > > > - (bma->cur ? > > > - bma->cur->bc_private.b.allocated : 0))); > > > + > > > + da_new = startblockval(PREV.br_blockcount) + > > > + startblockval(RIGHT.br_blockcount); > > > > s/br_blockcount/br_startblock/ :) > > Yes. And I've officially lost all faith in xfstests ever even testing > this case in xfs_bmap_add_extent_delay_real at all. > > I think it should be really easily testable by creating a large > delalloc reservation and then fsyncing out the middle of it. > Except of course we don't have a range fsync, and even then the > writeback code might cluster it. I might have to come up with > a special kernel module to even reproduce this reliably.. What about creating a DEBUG mode option and/or an error injection tag that allows for random partial extent allocation rather than the current behavior of attempting to allocate the whole delalloc extent? E.g., could we "randomly" trim the ends of the delalloc extent looked up at writeback time such that what we alloc it is smaller than the full delalloc extent, but still large enough to satisfy the writeback? The ideal result would be some combination of full allocs, some that trigger either the left or right filling cases and some that trigger the !LEFT_FILLING && !RIGHT_FILLING case. A bit hacky, but perhaps a test could then use dio reads or something to induce ranged writebacks. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 21, 2017 at 03:28:26PM +0200, Christoph Hellwig wrote: > [fullquote of a fullquote removed, sigh..] > > > > + temp = PREV.br_blockcount - new->br_blockcount; > > > + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), > > > + startblockval(PREV.br_startblock)); > > > > Needs another indent? > > > What for? I thought the second line of a continued statement was indented ether twice more or up to the paren, e.g.: da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), startblockval(PREV.br_startblock)); or: da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), startblockval(PREV.br_startblock)); but never a single tab: da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), startblockval(PREV.br_startblock)); The 'startblockval' line only has one more tab indent than the 'da_new =' line. --D > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 4fb73c0aca05..731854cc8b58 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -1588,7 +1588,6 @@ xfs_bmap_add_extent_delay_real( { struct xfs_bmbt_irec *new = &bma->got; int diff; /* temp value */ - xfs_bmbt_rec_host_t *ep; /* extent entry for idx */ int error; /* error return value */ int i; /* temp state */ xfs_ifork_t *ifp; /* inode fork pointer */ @@ -1600,10 +1599,10 @@ xfs_bmap_add_extent_delay_real( xfs_filblks_t da_new; /* new count del alloc blocks used */ xfs_filblks_t da_old; /* old count del alloc blocks used */ xfs_filblks_t temp=0; /* value for da_new calculations */ - xfs_filblks_t temp2=0;/* value for da_new calculations */ int tmp_rval; /* partial logging flags */ struct xfs_mount *mp; xfs_extnum_t *nextents; + struct xfs_bmbt_irec old; mp = bma->ip->i_mount; ifp = XFS_IFORK_PTR(bma->ip, whichfork); @@ -1629,9 +1628,9 @@ xfs_bmap_add_extent_delay_real( /* * Set up a bunch of variables to make the tests simpler. */ - ep = xfs_iext_get_ext(ifp, bma->idx); - xfs_bmbt_get_all(ep, &PREV); + xfs_iext_get_extent(ifp, bma->idx, &PREV); new_endoff = new->br_startoff + new->br_blockcount; + ASSERT(isnullstartblock(PREV.br_startblock)); ASSERT(PREV.br_startoff <= new->br_startoff); ASSERT(PREV.br_startoff + PREV.br_blockcount >= new_endoff); @@ -1706,9 +1705,8 @@ xfs_bmap_add_extent_delay_real( */ bma->idx--; trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx), - LEFT.br_blockcount + PREV.br_blockcount + - RIGHT.br_blockcount); + LEFT.br_blockcount += PREV.br_blockcount + RIGHT.br_blockcount; + xfs_iext_update_extent(ifp, bma->idx, &LEFT); trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); xfs_iext_remove(bma->ip, bma->idx + 1, 2, state); @@ -1733,9 +1731,7 @@ xfs_bmap_add_extent_delay_real( XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); error = xfs_bmbt_update(bma->cur, LEFT.br_startoff, LEFT.br_startblock, - LEFT.br_blockcount + - PREV.br_blockcount + - RIGHT.br_blockcount, LEFT.br_state); + LEFT.br_blockcount, LEFT.br_state); if (error) goto done; } @@ -1748,9 +1744,10 @@ xfs_bmap_add_extent_delay_real( */ bma->idx--; + old = LEFT; trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx), - LEFT.br_blockcount + PREV.br_blockcount); + LEFT.br_blockcount += PREV.br_blockcount; + xfs_iext_update_extent(ifp, bma->idx, &LEFT); trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); xfs_iext_remove(bma->ip, bma->idx + 1, 1, state); @@ -1758,16 +1755,15 @@ xfs_bmap_add_extent_delay_real( rval = XFS_ILOG_DEXT; else { rval = 0; - error = xfs_bmbt_lookup_eq(bma->cur, LEFT.br_startoff, - LEFT.br_startblock, LEFT.br_blockcount, + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff, + old.br_startblock, old.br_blockcount, &i); if (error) goto done; XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); error = xfs_bmbt_update(bma->cur, LEFT.br_startoff, LEFT.br_startblock, - LEFT.br_blockcount + - PREV.br_blockcount, LEFT.br_state); + LEFT.br_blockcount, LEFT.br_state); if (error) goto done; } @@ -1779,9 +1775,9 @@ xfs_bmap_add_extent_delay_real( * The right neighbor is contiguous, the left is not. */ trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_startblock(ep, new->br_startblock); - xfs_bmbt_set_blockcount(ep, - PREV.br_blockcount + RIGHT.br_blockcount); + PREV.br_startblock = new->br_startblock; + PREV.br_blockcount += RIGHT.br_blockcount; + xfs_iext_update_extent(ifp, bma->idx, &PREV); trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); xfs_iext_remove(bma->ip, bma->idx + 1, 1, state); @@ -1796,9 +1792,8 @@ xfs_bmap_add_extent_delay_real( goto done; XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); error = xfs_bmbt_update(bma->cur, PREV.br_startoff, - new->br_startblock, - PREV.br_blockcount + - RIGHT.br_blockcount, PREV.br_state); + PREV.br_startblock, + PREV.br_blockcount, PREV.br_state); if (error) goto done; } @@ -1811,8 +1806,9 @@ xfs_bmap_add_extent_delay_real( * the new one. */ trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_startblock(ep, new->br_startblock); - xfs_bmbt_set_state(ep, new->br_state); + PREV.br_startblock = new->br_startblock; + PREV.br_state = new->br_state; + xfs_iext_update_extent(ifp, bma->idx, &PREV); trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); (*nextents)++; @@ -1839,38 +1835,39 @@ xfs_bmap_add_extent_delay_real( * Filling in the first part of a previous delayed allocation. * The left neighbor is contiguous. */ + old = LEFT; + temp = PREV.br_blockcount - new->br_blockcount; + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), + startblockval(PREV.br_startblock)); + trace_xfs_bmap_pre_update(bma->ip, bma->idx - 1, state, _THIS_IP_); - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx - 1), - LEFT.br_blockcount + new->br_blockcount); - xfs_bmbt_set_startoff(ep, - PREV.br_startoff + new->br_blockcount); + LEFT.br_blockcount += new->br_blockcount; + xfs_iext_update_extent(ifp, bma->idx - 1, &LEFT); trace_xfs_bmap_post_update(bma->ip, bma->idx - 1, state, _THIS_IP_); - temp = PREV.br_blockcount - new->br_blockcount; trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_blockcount(ep, temp); + PREV.br_blockcount = temp = PREV.br_blockcount - new->br_blockcount; + PREV.br_startoff += new->br_blockcount; + PREV.br_startblock = nullstartblock(da_new); + xfs_iext_update_extent(ifp, bma->idx, &PREV); + trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); + if (bma->cur == NULL) rval = XFS_ILOG_DEXT; else { rval = 0; - error = xfs_bmbt_lookup_eq(bma->cur, LEFT.br_startoff, - LEFT.br_startblock, LEFT.br_blockcount, + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff, + old.br_startblock, old.br_blockcount, &i); if (error) goto done; XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); error = xfs_bmbt_update(bma->cur, LEFT.br_startoff, - LEFT.br_startblock, - LEFT.br_blockcount + - new->br_blockcount, + LEFT.br_startblock, LEFT.br_blockcount, LEFT.br_state); if (error) goto done; } - da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), - startblockval(PREV.br_startblock)); - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); - trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); bma->idx--; break; @@ -1880,10 +1877,6 @@ xfs_bmap_add_extent_delay_real( * Filling in the first part of a previous delayed allocation. * The left neighbor is not contiguous. */ - trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_startoff(ep, new_endoff); - temp = PREV.br_blockcount - new->br_blockcount; - xfs_bmbt_set_blockcount(ep, temp); xfs_iext_insert(bma->ip, bma->idx, 1, new, state); (*nextents)++; if (bma->cur == NULL) @@ -1911,12 +1904,19 @@ xfs_bmap_add_extent_delay_real( if (error) goto done; } + + temp = PREV.br_blockcount - new->br_blockcount; da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), startblockval(PREV.br_startblock) - (bma->cur ? bma->cur->bc_private.b.allocated : 0)); - ep = xfs_iext_get_ext(ifp, bma->idx + 1); - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); + + trace_xfs_bmap_pre_update(bma->ip, bma->idx + 1, state, _THIS_IP_); + PREV.br_startoff = new_endoff; + PREV.br_blockcount = temp; + PREV.br_startblock = nullstartblock(da_new); + xfs_iext_update_extent(ifp, bma->idx + 1, &PREV); trace_xfs_bmap_post_update(bma->ip, bma->idx + 1, state, _THIS_IP_); + break; case BMAP_RIGHT_FILLING | BMAP_RIGHT_CONTIG: @@ -1924,37 +1924,39 @@ xfs_bmap_add_extent_delay_real( * Filling in the last part of a previous delayed allocation. * The right neighbor is contiguous with the new allocation. */ - temp = PREV.br_blockcount - new->br_blockcount; + old = RIGHT; trace_xfs_bmap_pre_update(bma->ip, bma->idx + 1, state, _THIS_IP_); - xfs_bmbt_set_blockcount(ep, temp); - xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, bma->idx + 1), - new->br_startoff, new->br_startblock, - new->br_blockcount + RIGHT.br_blockcount, - RIGHT.br_state); + RIGHT.br_startoff = new->br_startoff; + RIGHT.br_startblock = new->br_startblock; + RIGHT.br_blockcount += new->br_blockcount; + xfs_iext_update_extent(ifp, bma->idx + 1, &RIGHT); trace_xfs_bmap_post_update(bma->ip, bma->idx + 1, state, _THIS_IP_); + if (bma->cur == NULL) rval = XFS_ILOG_DEXT; else { rval = 0; - error = xfs_bmbt_lookup_eq(bma->cur, RIGHT.br_startoff, - RIGHT.br_startblock, - RIGHT.br_blockcount, &i); + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff, + old.br_startblock, + old.br_blockcount, &i); if (error) goto done; XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done); - error = xfs_bmbt_update(bma->cur, new->br_startoff, - new->br_startblock, - new->br_blockcount + - RIGHT.br_blockcount, + error = xfs_bmbt_update(bma->cur, RIGHT.br_startoff, + RIGHT.br_startblock, RIGHT.br_blockcount, RIGHT.br_state); if (error) goto done; } + temp = PREV.br_blockcount - new->br_blockcount; da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), startblockval(PREV.br_startblock)); + trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); + PREV.br_blockcount = temp; + PREV.br_startblock = nullstartblock(da_new); + xfs_iext_update_extent(ifp, bma->idx, &PREV); trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); bma->idx++; @@ -1965,9 +1967,6 @@ xfs_bmap_add_extent_delay_real( * Filling in the last part of a previous delayed allocation. * The right neighbor is not contiguous. */ - temp = PREV.br_blockcount - new->br_blockcount; - trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); - xfs_bmbt_set_blockcount(ep, temp); xfs_iext_insert(bma->ip, bma->idx + 1, 1, new, state); (*nextents)++; if (bma->cur == NULL) @@ -1995,11 +1994,16 @@ xfs_bmap_add_extent_delay_real( if (error) goto done; } + + temp = PREV.br_blockcount - new->br_blockcount; da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp), startblockval(PREV.br_startblock) - (bma->cur ? bma->cur->bc_private.b.allocated : 0)); - ep = xfs_iext_get_ext(ifp, bma->idx); - xfs_bmbt_set_startblock(ep, nullstartblock(da_new)); + + trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_); + PREV.br_startblock = nullstartblock(da_new); + PREV.br_blockcount = temp; + xfs_iext_update_extent(ifp, bma->idx, &PREV); trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); bma->idx++; @@ -2026,19 +2030,33 @@ xfs_bmap_add_extent_delay_real( * PREV @ idx LEFT RIGHT * inserted at idx + 1 */ - temp = new->br_startoff - PREV.br_startoff; - temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff; - trace_xfs_bmap_pre_update(bma->ip, bma->idx, 0, _THIS_IP_); - xfs_bmbt_set_blockcount(ep, temp); /* truncate PREV */ + old = PREV; + + /* LEFT is the new middle */ LEFT = *new; + + /* RIGHT is the new right */ RIGHT.br_state = PREV.br_state; - RIGHT.br_startblock = nullstartblock( - (int)xfs_bmap_worst_indlen(bma->ip, temp2)); RIGHT.br_startoff = new_endoff; - RIGHT.br_blockcount = temp2; + RIGHT.br_blockcount = + PREV.br_startoff + PREV.br_blockcount - new_endoff; + RIGHT.br_startblock = + nullstartblock(xfs_bmap_worst_indlen(bma->ip, + RIGHT.br_blockcount)); + + /* truncate PREV */ + trace_xfs_bmap_pre_update(bma->ip, bma->idx, 0, _THIS_IP_); + PREV.br_blockcount = new->br_startoff - PREV.br_startoff; + PREV.br_startblock = + nullstartblock(xfs_bmap_worst_indlen(bma->ip, + PREV.br_blockcount)); + xfs_iext_update_extent(ifp, bma->idx, &PREV); + trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); + /* insert LEFT (r[0]) and RIGHT (r[1]) at the same time */ xfs_iext_insert(bma->ip, bma->idx + 1, 2, &LEFT, state); (*nextents)++; + if (bma->cur == NULL) rval = XFS_ILOG_CORE | XFS_ILOG_DEXT; else { @@ -2064,12 +2082,12 @@ xfs_bmap_add_extent_delay_real( if (error) goto done; } - temp = xfs_bmap_worst_indlen(bma->ip, temp); - temp2 = xfs_bmap_worst_indlen(bma->ip, temp2); - diff = (int)(temp + temp2 - - (startblockval(PREV.br_startblock) - - (bma->cur ? - bma->cur->bc_private.b.allocated : 0))); + + da_new = startblockval(PREV.br_blockcount) + + startblockval(RIGHT.br_blockcount); + diff = da_new - startblockval(old.br_startblock); + if (bma->cur) + diff += bma->cur->bc_private.b.allocated; if (diff > 0) { error = xfs_mod_fdblocks(bma->ip->i_mount, -((int64_t)diff), false); @@ -2078,16 +2096,7 @@ xfs_bmap_add_extent_delay_real( goto done; } - ep = xfs_iext_get_ext(ifp, bma->idx); - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp)); - trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_); - trace_xfs_bmap_pre_update(bma->ip, bma->idx + 2, state, _THIS_IP_); - xfs_bmbt_set_startblock(xfs_iext_get_ext(ifp, bma->idx + 2), - nullstartblock((int)temp2)); - trace_xfs_bmap_post_update(bma->ip, bma->idx + 2, state, _THIS_IP_); - bma->idx++; - da_new = temp + temp2; break; case BMAP_LEFT_FILLING | BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
Use xfs_iext_get_extent to find, and xfs_iext_update_extent to update entries in the in-core extent list. This isolates the function from the detailed layout of the extent list, and generally makes the code a lot more readable. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_bmap.c | 181 +++++++++++++++++++++++++---------------------- 1 file changed, 95 insertions(+), 86 deletions(-)