xfs: fixes for 3.7-rc3
cover xfs: fixes for 3.7-rc3
Hi Ben,
Here are my current outstanding fixes that are candidates for 3.7. Patches 2 and 3 are the initial fixes from the verifier series (real bugs) and the other are regressions that I've recently fixed. Some need to be sent back to stable kernels as well and are tagged as such.
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/6 xfs: silence uninitialised f.file warning.
From: Dave Chinner <[email protected]>
Uninitialised variable build warning introduced by 2903ff0 ("switch simple cases of fget_light to fdget"), gcc is not smart enough to work out that the variable is not used uninitialised, and the commit removed the initialisation at declaration that the old variable had.
--- fs/xfs/xfs_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_ioctl.c +1 -1
@@ -70,7 +70,7 @@ xfs_find_handle(int hsize;xfs_handle_t handle;struct inode *inode; -struct fd f; +struct fd f = {0 };struct path path;int error;struct xfs_inode *ip;
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/6] xfs: silence uninitialised f.file warning.
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Uninitialised variable build warning introduced by 2903ff0 ("switch simple cases of
fget_lightto fdget"), gcc is not smart enough to work out that the variable is not used uninitialised, and the commit removed the initialisation at declaration that the old variable had.Signed-off-by Dave Chinner <[email protected]>
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/6] xfs: silence uninitialised f.file warning.
In reply to Dave ChinnerQuoted 9 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Uninitialised variable build warning introduced by 2903ff0 ("switch simple cases of
fget_lightto fdget"), gcc is not smart enough to work out that the variable is not used uninitialised, and the commit removed the initialisation at declaration that the old variable had.Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/6 xfs: growfs: don't read garbage for new secondary superblocks
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When updating new secondary superblocks in a growfs operation, the sueprblock buffer is read from the newly grown region of the underlying device. This is not guaranteed to be zero, so violates the underlying assumption that the unused parts of superblocks are zero filled. Get a new buffer for these secondary superblocks to ensure that the unused regions are zero filled correctly.
--- fs/xfs/xfs_fsops.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
fs/xfs/xfs_fsops.c +19 -2
@@ -399,9 +399,26 @@ xfs_growfs_data_private(/* update secondary superblocks. */ for (agno =1 ; agno < nagcount; agno++) { - error =xfs_trans_read_buf (mp,NULL , mp->m_ddev_targp, + error =0 ; +/* +* new secondary superblocks need to be zeroed, not read from +* disk as the contents of the new area we are growing into is +* completely unknown. +*/ +if (agno < oagcount) { + error =xfs_trans_read_buf (mp,NULL , mp->m_ddev_targp,XFS_AGB_TO_DADDR (mp, agno,XFS_SB_BLOCK (mp)),XFS_FSS_TO_BB (mp,1 ),0 , &bp); + }else { + bp =xfs_trans_get_buf (NULL , mp->m_ddev_targp, +XFS_AGB_TO_DADDR (mp, agno,XFS_SB_BLOCK (mp)), +XFS_FSS_TO_BB (mp,1 ),0 ); +if (bp) +xfs_buf_zero (bp,0 ,BBTOB (bp->b_length)); +else + error =ENOMEM ; + } +if (error) { xfs_warn(mp, "error %d reading secondary superblock for ag %d",
@@ -423,7 +440,7 @@ xfs_growfs_data_private(break ;/* no point in continuing */ } } -return 0 ; +return error; error0:xfs_trans_cancel (tp,XFS_TRANS_ABORT );
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/6 xfs: invalidate allocbt blocks moved to the free list
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When we free a block from the alloc btree tree, we move it to the freelist held in the AGFL and mark it busy in the busy extent tree. This typically happens when we merge btree blocks.
Once the transaction is committed and checkpointed, the block can remain on the free list for an indefinite amount of time. Now, this isn't the end of the world at this point - if the free list is shortened, the buffer is invalidated in the transaction that moves it back to free space. If the buffer is allocated as metadata from the free list, then all the modifications getted logged, and we have no issues, either. And if it gets allocated as userdata direct from the freelist, it gets invalidated and so will never get written.
However, during the time it sits on the free list, pressure on the log can cause the AIL to be pushed and the buffer that covers the block gets pushed for write. IOWs, we end up writing a freed metadata block to disk. Again, this isn't the end of the world because we know from the above we are only writing to free space.
The problem, however, is for validation callbacks. If the block was on old btree root block, then the level of the block is going to be higher than the current tree root, and so will fail validation. There may be other inconsistencies in the block as well, and currently we don't care because the block is in free space. Shutting down the filesystem because a freed block doesn't pass write validation, OTOH, is rather unfriendly.
So, make sure we always invalidate buffers as they move from the free space trees to the free list so that we guarantee they never get written to disk while on the free list.
--- fs/xfs/xfs_alloc_btree.c | 2 ++ 1 file changed, 2 insertions(+)
fs/xfs/xfs_alloc_btree.c +2 -0
@@ -121,6 +121,8 @@ xfs_allocbt_free_block(xfs_extent_busy_insert (cur->bc_tp,be32_to_cpu (agf->agf_seqno), bno,1 ,XFS_EXTENT_BUSY_SKIP_DISCARD );xfs_trans_agbtree_delta (cur->bc_tp,-1 ); + +xfs_trans_binval (cur->bc_tp, bp);return 0 ; }
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list
Quoted 29 lines · Dave Chinner
From: Dave Chinner<[email protected]>
When we free a block from the alloc btree tree, we move it to the freelist held in the AGFL and mark it busy in the busy extent tree. This typically happens when we merge btree blocks.
Once the transaction is committed and checkpointed, the block can remain on the free list for an indefinite amount of time. Now, this isn't the end of the world at this point - if the free list is shortened, the buffer is invalidated in the transaction that moves it back to free space. If the buffer is allocated as metadata from the free list, then all the modifications getted logged, and we have no issues, either. And if it gets allocated as userdata direct from the freelist, it gets invalidated and so will never get written.
However, during the time it sits on the free list, pressure on the log can cause the AIL to be pushed and the buffer that covers the block gets pushed for write. IOWs, we end up writing a freed metadata block to disk. Again, this isn't the end of the world because we know from the above we are only writing to free space.
The problem, however, is for validation callbacks. If the block was on old btree root block, then the level of the block is going to be higher than the current tree root, and so will fail validation. There may be other inconsistencies in the block as well, and currently we don't care because the block is in free space. Shutting down the filesystem because a freed block doesn't pass write validation, OTOH, is rather unfriendly.
Quoted 6 lines
So, make sure we always invalidate buffers as they move from the free space trees to the free list so that we guarantee they never get written to disk while on the free list.
Signed-off-by Dave Chinner<[email protected]>Reviewed-by Christoph Hellwig<[email protected]>
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/6 xfs: don't vmap inode cluster buffers during free
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Inode buffers do not need to be mapped as inodes are read or written directly from/to the pages underlying the buffer. This fixes a regression introduced by commit 611c994 ("xfs: make XBF_MAPPED the default behaviour").
--- fs/xfs/xfs_inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/xfs/xfs_inode.c +2 -1
@@ -1760,7 +1760,8 @@ xfs_ifree_cluster(* to mark all the active inodes on the buffer stale. */ bp =xfs_trans_get_buf (tp, mp->m_ddev_targp, blkno, - mp->m_bsize * blks_per_cluster,0 ); + mp->m_bsize * blks_per_cluster, +XBF_UNMAPPED );if (!bp)return ENOMEM ;
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/6] xfs: don't vmap inode cluster buffers during free
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Inode buffers do not need to be mapped as inodes are read or written directly from/to the pages underlying the buffer. This fixes a regression introduced by commit 611c994 ("xfs: make
XBF_MAPPEDthe default behaviour").Signed-off-by Dave Chinner <[email protected]>
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/6] xfs: don't vmap inode cluster buffers during free
In reply to Dave ChinnerQuoted 9 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Inode buffers do not need to be mapped as inodes are read or written directly from/to the pages underlying the buffer. This fixes a regression introduced by commit 611c994 ("xfs: make
XBF_MAPPEDthe default behaviour").Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/6 xfs: fix buffer shudown reference count mismatch
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When we shut down the filesystem, we have to unpin and free all the buffers currently active in the CIL. To do this we unpin and remove them in one operation as a result of a failed iclogbuf write. For buffers, we do this removal via a simultated IO completion of after marking the buffer stale.
At the time we do this, we have two references to the buffer - the active LRU reference and the buf log item. The LRU reference is removed by marking the buffer stale, and the active CIL reference is by the xfs_buf_iodone() callback that is run by xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone callback).
However, ioend processing requires one more reference - that of the IO that it is completing. We don't have this reference, so we free the buffer prematurely and use it after it is freed. This leads to assert failures in xfs_buf_rele() on debug kernels because the b_hold count is zero.
Fix this by making sure we take the necessary IO reference before starting IO completion processing on the stale buffer.
--- fs/xfs/xfs_buf_item.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
fs/xfs/xfs_buf_item.c +16 -0
@@ -526,7 +526,23 @@ xfs_buf_item_unpin( }xfs_buf_relse (bp); }else if (freed && remove) { +/* +* There are currently two references to the buffer - the active +* LRU reference and the buf log item. What we are about to do +* here - simulate a failed IO completion - requires 3 +* references. +* +* The LRU reference is removed by the xfs_buf_stale() call. The +* buf item reference is removed by the xfs_buf_iodone() +* callback that is run by xfs_buf_do_callbacks() during ioend +* processing (via the bp->b_iodone callback), and then finally +* the ioend processing drops the IO reference. +* +* Hence we need to take an additional reference here so that IO +* completion processing doesn't free the buffer prematurely. +*/ xfs_buf_lock (bp); +xfs_buf_hold (bp);xfs_buf_ioerror (bp,EIO );XFS_BUF_UNDONE (bp);xfs_buf_stale (bp);
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
Quoted 21 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When we shut down the filesystem, we have to unpin and free all the buffers currently active in the CIL. To do this we unpin and remove them in one operation as a result of a failed iclogbuf write. For buffers, we do this removal via a simultated IO completion of after marking the buffer stale.
At the time we do this, we have two references to the buffer - the active LRU reference and the buf log item. The LRU reference is removed by marking the buffer stale, and the active CIL reference is by the
xfs_buf_iodone()callback that is run byxfs_buf_do_callbacks()during ioend processing (via thebp->b_iodonecallback).However, ioend processing requires one more reference - that of the IO that it is completing. We don't have this reference, so we free the buffer prematurely and use it after it is freed. This leads to assert failures in
xfs_buf_rele()on debug kernels because theb_holdcount is zero.
Quoted 5 lines
Fix this by making sure we take the necessary IO reference before starting IO completion processing on the stale buffer.
Cc <[email protected]>Signed-off-by Dave Chinner <[email protected]>
HOld off on this one for a little while - I think I've found the source of the subsequent failures that Mark reported.
The reason why this did't fail on every buffer is that not all buffers drop a reference in xfs_buf_iodone_work(). Only buffers that are marked with XBF_ASYNC will do this, and they seem to be rare. Hence the rare failure.
The fix that I've done here means all buffers going through this path will take an extra reference, but that reference is only dropped on async buffers. Because all the buffers are markd stale, they are removed from the LRU, and so xfs_buftarg_wait() during unmount does not find them and hence the remaining reference is never removed. Hence the perag reference still remains, and we assert fail there.
Solution seems simple - set the XBF_ASYNC flag on all buffers so that the last reference is taken away correctly. Testing that now.
(FWIW, I found this using perag get/put tracing :)
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 5/6 xfs: fix buffer shudown reference count mismatch
xfs: fix buffer shudown reference count mismatch
From: Dave Chinner <[email protected]>
When we shut down the filesystem, we have to unpin and free all the buffers currently active in the CIL. To do this we unpin and remove them in one operation as a result of a failed iclogbuf write. For buffers, we do this removal via a simultated IO completion of after marking the buffer stale.
At the time we do this, we have two references to the buffer - the active LRU reference and the buf log item. The LRU reference is removed by marking the buffer stale, and the active CIL reference is by the xfs_buf_iodone() callback that is run by xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone callback).
However, ioend processing requires one more reference - that of the IO that it is completing. We don't have this reference, so we free the buffer prematurely and use it after it is freed. For buffers marked with XBF_ASYNC, this leads to assert failures in xfs_buf_rele() on debug kernels because the b_hold count is zero.
Fix this by making sure we take the necessary IO reference before starting IO completion processing on the stale buffer, and set the XBF_ASYNC flag to ensure that IO completion processing removes all the active references from the buffer to ensure it is fully torn down.
---
V2 - add XBF_ASYNC to buffers so last reference is always removed.
Fixes shutdown assert failures due to references from buffers remaining the perag structures at unmount.
fs/xfs/xfs_buf_item.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
fs/xfs/xfs_buf_item.c +18 -0
@@ -526,7 +526,25 @@ xfs_buf_item_unpin( }xfs_buf_relse (bp); }else if (freed && remove) { +/* +* There are currently two references to the buffer - the active +* LRU reference and the buf log item. What we are about to do +* here - simulate a failed IO completion - requires 3 +* references. +* +* The LRU reference is removed by the xfs_buf_stale() call. The +* buf item reference is removed by the xfs_buf_iodone() +* callback that is run by xfs_buf_do_callbacks() during ioend +* processing (via the bp->b_iodone callback), and then finally +* the ioend processing will drop the IO reference if the buffer +* is marked XBF_ASYNC. +* +* Hence we need to take an additional reference here so that IO +* completion processing doesn't free the buffer prematurely. +*/ xfs_buf_lock (bp); +xfs_buf_hold (bp); + bp->b_flags |=XBF_ASYNC ;xfs_buf_ioerror (bp,EIO );XFS_BUF_UNDONE (bp);xfs_buf_stale (bp);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6 V2] xfs: fix buffer shudown reference count mismatch
Quoted 29 lines · Dave Chinner
xfs: fix buffer shudown reference count mismatch
From: Dave Chinner<[email protected]>
When we shut down the filesystem, we have to unpin and free all the buffers currently active in the CIL. To do this we unpin and remove them in one operation as a result of a failed iclogbuf write. For buffers, we do this removal via a simultated IO completion of after marking the buffer stale.
At the time we do this, we have two references to the buffer - the active LRU reference and the buf log item. The LRU reference is removed by marking the buffer stale, and the active CIL reference is by the
xfs_buf_iodone()callback that is run byxfs_buf_do_callbacks()during ioend processing (via thebp->b_iodonecallback).However, ioend processing requires one more reference - that of the IO that it is completing. We don't have this reference, so we free the buffer prematurely and use it after it is freed. For buffers marked with
XBF_ASYNC, this leads to assert failures inxfs_buf_rele()on debug kernels because theb_holdcount is zero.Fix this by making sure we take the necessary IO reference before starting IO completion processing on the stale buffer, and set the
XBF_ASYNCflag to ensure that IO completion processing removes all the active references from the buffer to ensure it is fully torn down.
Quoted 6 lines
Cc <[email protected]>Signed-off-by Dave Chinner<[email protected]>---
V2 - addXBF_ASYNCto buffers so last reference is always removed.
Fixes shutdown assert failures due to references from buffers remaining the perag structures at unmount.
Yes and thank-you. This eliminates both the b_hold and pag_ref asserts in xfstest 179.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
In reply to Dave ChinnerQuoted 10 lines
The fix that I've done here means all buffers going through this path will take an extra reference, but that reference is only dropped on async buffers. Because all the buffers are markd stale, they are removed from the LRU, and so
xfs_buftarg_wait()during unmount does not find them and hence the remaining reference is never removed. Hence the perag reference still remains, and we assert fail there.Solution seems simple - set the
XBF_ASYNCflag on all buffers so that the last reference is taken away correctly. Testing that now.
I don't like this. ioend processing is very different for synchrous writes, with the most important difference being that synchronous writes need to wake the submitter at I/O completion. From all I can see your v2 patch breaks that beahviour. For 3.7-rc I'd suggest taking the additional reference conditionally.
For 3.8 I'm going to look into simply acquiring an additional reference for synchronous writes during I/O submission to kill these special cases all over the buffer code.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
Quoted 9 lines · Christoph Hellwig
I don't like this. ioend processing is very different for synchrous writes, with the most important difference being that synchronous writes need to wake the submitter at I/O completion. From all I can see your v2 patch breaks that beahviour. For 3.7-rc I'd suggest taking the additional reference conditionally.
For 3.8 I'm going to look into simply acquiring an additional reference for synchronous writes during I/O submission to kill these special cases all over the buffer code.
Testing shows that conditionally taking the additional reference does work at avoiding the b_bhold is zero assert without causing pag is not empty assert.
Thanks,
--Mark Tinguely.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
In reply to Christoph HellwigQuoted 8 lines · Christoph Hellwig
Quoted 8 lines
The fix that I've done here means all buffers going through this path will take an extra reference, but that reference is only dropped on async buffers. Because all the buffers are markd stale, they are removed from the LRU, and so
xfs_buftarg_wait()during unmount does not find them and hence the remaining reference is never removed. Hence the perag reference still remains, and we assert fail there.
Quoted 6 lines
Quoted 2 lines
Solution seems simple - set the
XBF_ASYNCflag on all buffers so that the last reference is taken away correctly. Testing that now.I don't like this. ioend processing is very different for synchrous writes, with the most important difference being that synchronous writes need to wake the submitter at I/O completion.
I think that's irrelevant here - there will *never* be an IO waiter at this point in time. This processing is in log buffer IO completion context, so the buffers are still pinned in memory. Hence anyone trying to do IO on it will be waiting in xfs_buf_wait_unpin() and never get to xfs_buf_iowait(). And because xfs_buf_wait_unpin() is called with the buffer lock held, we'll never do the failure handling in xfs_buf_item_unpin until the buffer IO is completed and it is unlocked.
FWIW, this also indicates problem in IO submission - xfs_buf_wait_unpin() occurs after the shutdown checks, so we could wake a pinned waiter that then issues IO after the shutdown has started because there are no shutdown checks after xfs_buf_wait_unpin() is called....
Quoted 3 lines
From all I can
see your v2 patch breaks that beahviour. For 3.7-rc I'd suggest taking the additional reference conditionally.
It's not a conditional reference - every buffer needs it. Right now we have an unconditional use after free because bp->b_iodone() drops the last reference on the buffer, then calls xfs_buf_iodone() again. The only thing the XBF_ASYNC flag cwcurrently determines is is whether we get an assert failure by calling xfs_buf_rele() on a freed buffer or not.
What we are doing here is ioend processing without actually having issued an IO. i.e. we are using the ioend processing for error handling, not for IO completion. Hence we have to set the buffer up as though it has been under IO so that ioend processing does exactly what we want. And what we want is the buffer to be unconditionally freed at the end of ioend processing.
What we end up with when we hit the "remove && freed" case is a buffer that is not pinned, is not under IO and will have no new references because we are in a shutdown situation. We need to mark the buffer stale to ensure it is not written, and free the buffer when the last reference goes away. That reference is the reference the transaction subsystem owns and it guarantees that there is no IO completion waiters on the buffer.
IOWs, we have two options after taking an extra reference:
1. clear the XBF_ASYNC flag unconditionally and call xfs_buf_relse() ourselves to unlock and free the buffer
2. set the XBF_ASYNC flag unconditionally and let xfs_buf_ioend() call xfs_buf_relse() itself.
I tossed a coin, and it came down on #2. Either way will work, but we need that extra reference on all buffers passing through this code...
Quoted 3 lines
For 3.8 I'm going to look into simply acquiring an additional reference for synchronous writes during I/O submission to kill these special cases all over the buffer code.
Sure, but that's a different issue, not directly realted to fixing the bug at hand. It would also mean that if we choose #1, we'll need to take 2 references for the IO for this error handling path. That seems like a much stranger special case than just using async buffer IO completion to do the work we need...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
Quoted 8 lines · Dave Chinner
I think that's irrelevant here - there will *never* be an IO waiter at this point in time. This processing is in log buffer IO completion context, so the buffers are still pinned in memory. Hence anyone trying to do IO on it will be waiting in
xfs_buf_wait_unpin()and never get toxfs_buf_iowait(). And becausexfs_buf_wait_unpin()is called with the buffer lock held, we'll never do the failure handling inxfs_buf_item_unpinuntil the buffer IO is completed and it is unlocked.
How do we manage to submit it synchronously then? The inode and dquot reclaim xfs_bwrite calls already wait for an unpin first, so I don't think these are the problem. The only other call on a live fs seems to xfs_qm_shake -> xfs_buf_delwri_submit, but that one does wait for the complete() call on b_iowait. I suspect we are hitting that and due to it skipping the wait if b_ioerror is set and waiting on multiple buffers that complete together might hide the issue.
__xfs_buf_delwri_submit for the wait == true case also seems to be the only place that actually skips the ispinned check.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
Quoted 11 lines · Christoph Hellwig
Quoted 8 lines · Dave Chinner
I think that's irrelevant here - there will *never* be an IO waiter at this point in time. This processing is in log buffer IO completion context, so the buffers are still pinned in memory. Hence anyone trying to do IO on it will be waiting in
xfs_buf_wait_unpin()and never get toxfs_buf_iowait(). And becausexfs_buf_wait_unpin()is called with the buffer lock held, we'll never do the failure handling inxfs_buf_item_unpinuntil the buffer IO is completed and it is unlocked.How do we manage to submit it synchronously then?
I don't follow what problem you are talking about here. Fundamentally, races with IO are resolved like this regardless of whether the racing Io is sync or async
xfs_buf_lock make modifications ..... xfs_buf_lock ..... xfs_trans_commit .... IOP_PIN() IOP_UNLOCK()
xfs_buf_iorequest
xfs_buf_wait_unpin()
.....
<shutdown, no new buffers can get to xfs_buf_iorequest>
IOP_UNPIN(remove) xfs_buf_item_unpin(remove) wake_up_all(pin waiters) xfs_buf_lock() ..... submit IO ...... xfs_buf_ioend() wakeup(b_iowait) ..... xfs_buf_relse() xfs_buf_hold xfs_buf_stale ASYNC xfs_buf_ioend() bp->b_iodone() xfs_buf_rele xfs_buf_ioend() xfs_buf_rele xfs_buf_free
What this also points out is that we shoul dbe checking for shutdown after xfs_buf_wait_unpin(), too, because otherwise we are submitting IO after the shutdown is initiated....
Quoted 7 lines
The inode and dquot
reclaimxfs_bwritecalls already wait for an unpin first, so I don't think these are the problem. The only other call on a live fs seems toxfs_qm_shake->xfs_buf_delwri_submit, but that one does wait for thecomplete()call onb_iowait. I suspect we are hitting that and due to it skipping the wait ifb_ioerroris set and waiting on multiple buffers that complete together might hide the issue.
We are in a shutdown situation. xfs_buf_delwri_submit() goes via xfs_bdstrat_cb() which will stop any new IOs from being submitted via this path. If it is blocked on the above case, then it is also resolved by the above case...
Quoted 2 lines
__xfs_buf_delwri_submitfor the wait == true case also seems to be the only place that actually skips the ispinned check.
Sure, but that we're in a shutdown situation, so it doesn't matter - the buffer will never get to xfs_buf_wait_unpin() because of the shutdown check in xfs_bdstrat_cb().
I still don't see the problem you are trying to explain to me. Maybe I'm just being dense....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/6 xfs: fix reading of wrapped log data
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Commit 4439647 ("xfs: reset buffer pointers before freeing them") in 3.0-rc1 introduced a regression when recovering log buffers that wrapped around the end of log. The second part of the log buffer at the start of the physical log was being read into the header buffer rather than the data buffer, and hence recovery was seeing garbage in the data buffer when it got to the region of the log buffer that was incorrectly read.
--- fs/xfs/xfs_log_recover.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_log_recover.c +1 -1
@@ -3542,7 +3542,7 @@ xlog_do_recovery_pass(* - order is important. */ error =xlog_bread_offset (log,0 , - bblks - split_bblks, hbp, + bblks - split_bblks, dbp, offset +BBTOB (split_bblks));if (error) goto bread_err2;
Signature · 6 lines
1.7.10 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/6] xfs: fix reading of wrapped log data
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Commit 4439647 ("xfs: reset buffer pointers before freeing them") in 3.0-rc1 introduced a regression when recovering log buffers that wrapped around the end of log. The second part of the log buffer at the start of the physical log was being read into the header buffer rather than the data buffer, and hence recovery was seeing garbage in the data buffer when it got to the region of the log buffer that was incorrectly read.
I think you mentioned earlier you found a reproducer for this, any chance it could be added to xfstests?
fs/xfs/xfs_log_recover.c xlog_do_recovery_pass( · line 3545
Quoted 3 lines
fs/xfs/xfs_log_recover.c +1
-1error = xlog_bread_offset(log, 0, - bblks - split_bblks, hbp, + bblks - split_bblks, dbp,
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/6] xfs: fix reading of wrapped log data
Quoted 11 lines · Christoph Hellwig
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Commit 4439647 ("xfs: reset buffer pointers before freeing them") in 3.0-rc1 introduced a regression when recovering log buffers that wrapped around the end of log. The second part of the log buffer at the start of the physical log was being read into the header buffer rather than the data buffer, and hence recovery was seeing garbage in the data buffer when it got to the region of the log buffer that was incorrectly read.
Quoted 2 lines
I think you mentioned earlier you found a reproducer for this, any chance it could be added to xfstests?
My reproducer is specific to the hardware I was testing on and various filesystem parameters (e.g. log size, RAM, writeback rates to storage, etc), and it was dependent on compilebench to generate the load. I've been trying to isolate it down to a reproducable case simliar to test 016, but I haven't quite got it there yet.
And, yes, I very much want a test that covers recovery of wrapped log buffers. I was very surprised when I realised that it wasnt' specifically tested by xfstests....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/6] xfs: fix reading of wrapped log data
In reply to Dave ChinnerQuoted 10 lines · Dave Chinner
From: Dave Chinner<[email protected]>
Commit 4439647 ("xfs: reset buffer pointers before freeing them") in 3.0-rc1 introduced a regression when recovering log buffers that wrapped around the end of log. The second part of the log buffer at the start of the physical log was being read into the header buffer rather than the data buffer, and hence recovery was seeing garbage in the data buffer when it got to the region of the log buffer that was incorrectly read.
Quoted 4 lines
Cc <[email protected]> # 3.0.x, 3.2.x, 3.4.x 3.6.xReported-by Torsten Kaiser<[email protected]>Signed-off-by Dave Chinner<[email protected]>---
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] xfs: report projid32bit feature in geometry call
In reply to Dave ChinnerQuoted 5 lines · Eric Sandeen
When xfs gained the projid32bit feature, it was never added to the FSGEOMETRY ioctl feature flags, so it's not queryable without this patch.
Signed-off-by Eric Sandeen <[email protected]>
committed to git://oss.sgi.com/xfs/xfs.git, master and for-next branches.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · Dave Chinner
Hi Ben,
Here are my current outstanding fixes that are candidates for 3.7. Patches 2 and 3 are the initial fixes from the verifier series (real bugs) and the other are regressions that I've recently fixed. Some need to be sent back to stable kernels as well and are tagged as such.
Hi Ben,
What's the status with these? It's been almost a week since I posted them, and apart from patch 5, they are all good to go to Linus. IMO, patch 5 is also good to go as well, but I don't mind if that is not sent immediately as that one is not as critical as the others.
I'm particularly concerned because of the bug that patch 6 fixes - it's a real corruption bug that users are hitting we need to get it upstream and back into the stable kernels ASAP. But this patch hasn't even been committed to the dev tree yet...
Can you please get these upstream ASAP? If you don't have the time to do it today, let me know so I can send the patches straight to Linus and get them to the upstream and stable trees that way....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hi Dave,
Quoted 14 lines · Dave Chinner
Quoted 5 lines · Dave Chinner
Here are my current outstanding fixes that are candidates for 3.7. Patches 2 and 3 are the initial fixes from the verifier series (real bugs) and the other are regressions that I've recently fixed. Some need to be sent back to stable kernels as well and are tagged as such.
Hi Ben,
What's the status with these? It's been almost a week since I posted them, and apart from patch 5, they are all good to go to Linus. IMO, patch 5 is also good to go as well, but I don't mind if that is not sent immediately as that one is not as critical as the others.
Quoted 8 lines
I'm particularly concerned because of the bug that patch 6 fixes - it's a real corruption bug that users are hitting we need to get it upstream and back into the stable kernels ASAP. But this patch hasn't even been committed to the dev tree yet...
Can you please get these upstream ASAP? If you don't have the time to do it today, let me know so I can send the patches straight to Linus and get them to the upstream and stable trees that way....
I pulled these in yesterday. I'll send the pull request this afternoon.
Regards,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 5 lines · Dave Chinner
Here are my current outstanding fixes that are candidates for 3.7. Patches 2 and 3 are the initial fixes from the verifier series (real bugs) and the other are regressions that I've recently fixed. Some need to be sent back to stable kernels as well and are tagged as such.
Thanks for reposting. Some of these I didn't have in my queue.
I've committed the patches in this seres to git://oss.sgi.com/xfs/xfs.git,
master and for-next branches, with v2 of patch 5. I'll do a pull request asap.
Regards,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs