xfs: miscellaneous log recovery fixes
v2 cover xfs: miscellaneous log recovery fixes
Hi all,
Here's v2 of the log recovery fixups for filesystems with undersized logs and the v4 NULL buffer verifier problem. The most significant change is that patch 1 is rewritten to take a different approach to sanity check the log block addresses used during log recovery cycle and record verification. Specifically, it validates the log block number used for each buffer on read or write. Also, I've dropped patch 4 for now since I've not heard anything to suggest it's really necessary.
Brian
v2:
- Use xlog buffer validation rather than explicit checks.
- Don't push AIL on log recovery error.
- Drop patch 4 (rfc).
- Fix up commit logs.
v1: https://marc.info/?l=linux-xfs&m=150877001230751&w=2
Brian Foster (3):
xfs: more robust recovery xlog buffer validation
xfs: fix log block underflow during recovery cycle verification
xfs: drain the buffer LRU on mount
fs/xfs/xfs_log.c | 16 ++++++++++++++++ fs/xfs/xfs_log_recover.c | 40 +++++++++++++++++++++++++--------------- 2 files changed, 41 insertions(+), 15 deletions(-)
Signature · 1 line
2.9.5
PATCH v2 1/3 xfs: more robust recovery xlog buffer validation
mkfs has a historical problem where it can format very small filesystems with too small of a physical log. Under certain conditions, log recovery of an associated filesystem can end up passing garbage parameter values to some of the cycle and log record verification functions due to bugs in log recovery not dealing with such filesystems properly. This results in attempts to read from bogus/underflowed log block addresses.
Since the buffer read may ultimately succeed, log recovery can proceed with bogus data and otherwise go off the rails and crash. One example of this is a negative last_blk being passed to xlog_find_verify_log_record() causing us to skip the loop, pass a NULL head pointer to xlog_header_check_mount() and crash.
Improve the xlog buffer verification to address this problem. We already verify xlog buffer length, so update this mechanism to also sanity check for a valid log relative block address and otherwise return an error. Pass a fixed, valid log block address from xlog_get_bp() since the target address will be validated when the buffer is read. This ensures that any bogus log block address/length calculations lead to graceful mount failure rather than risking a crash or worse if recovery proceeds with bogus data.
--- fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)
fs/xfs/xfs_log_recover.c +24 -14
@@ -85,17 +85,21 @@ struct xfs_buf_cancel {*/ /* -* Verify the given count of basic blocks is valid number of blocks -* to specify for an operation involving the given XFS log buffer. -* Returns nonzero if the count is valid, 0 otherwise. +* Verify the log-relative block number and length in basic blocks are valid for +* an operation involving the given XFS log buffer. Returns true if the fields +* are valid, false otherwise. */ - -static inline int -xlog_buf_bbcount_valid ( +static inline bool +xlog_verify_bp (struct xlog *log, +xfs_daddr_t blk_no,int bbcount) { -return bbcount >0 && bbcount <= log->l_logBBsize; +if (blk_no <0 || blk_no >= log->l_logBBsize) +return false; +if (bbcount <=0 || bbcount > log->l_logBBsize) +return false; +return true; }/*
@@ -110,7 +114,11 @@ xlog_get_bp( {struct xfs_buf *bp; -if (!xlog_buf_bbcount_valid (log, nbblks)) { +/* +* Pass log block 0 since we don't have an addr yet, buffer will be +* verified on read. +*/ +if (!xlog_verify_bp (log,0 , nbblks)) {xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);
@@ -180,9 +188,10 @@ xlog_bread_noalign( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }
@@ -265,9 +274,10 @@ xlog_bwrite( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }
Signature · 1 line
2.9.5
Re: [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation
Quoted 48 lines · Brian Foster
mkfs has a historical problem where it can format very small filesystems with too small of a physical log. Under certain conditions, log recovery of an associated filesystem can end up passing garbage parameter values to some of the cycle and log record verification functions due to bugs in log recovery not dealing with such filesystems properly. This results in attempts to read from bogus/underflowed log block addresses.
Since the buffer read may ultimately succeed, log recovery can proceed with bogus data and otherwise go off the rails and crash. One example of this is a negative
last_blkbeing passed toxlog_find_verify_log_record()causing us to skip the loop, pass a NULL head pointer toxlog_header_check_mount()and crash.Improve the xlog buffer verification to address this problem. We already verify xlog buffer length, so update this mechanism to also sanity check for a valid log relative block address and otherwise return an error. Pass a fixed, valid log block address from
xlog_get_bp()since the target address will be validated when the buffer is read. This ensures that any bogus log block address/length calculations lead to graceful mount failure rather than risking a crash or worse if recovery proceeds with bogus data.Reported-by Zorro Lang <[email protected]>Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)fs/xfs/xfs_log_recover.c +4
-6@@ -85,17 +85,21 @@ struct xfs_buf_cancel {*/ /* -* Verify the given count of basic blocks is valid number of blocks -* to specify for an operation involving the given XFS log buffer. -* Returns nonzero if the count is valid, 0 otherwise. +* Verify the log-relative block number and length in basic blocks are valid for +* an operation involving the given XFS log buffer. Returns true if the fields +* are valid, false otherwise. */ - -static inline int -xlog_buf_bbcount_valid( +static inline bool
fs/xfs/xfs_log_recover.c struct xfs_buf_cancel { · line 101
Quoted 10 lines
fs/xfs/xfs_log_recover.c +6
-1+xlog_verify_bp ( struct xlog *log, + xfs_daddr_t blk_no, int bbcount) { -return bbcount >0 && bbcount <= log->l_logBBsize; + if (blk_no < 0 || blk_no >= log->l_logBBsize) + return false; +if (bbcount <=0 || bbcount > log->l_logBBsize) +return false;
Shouldn't this be (blk_no + bbcount) > log->l_logBBsize, since the blk_no/bbcount parameters identify an extent within the log?
--D
Quoted 52 lines
fs/xfs/xfs_log_recover.c +14
-7+return true; }/* @@ -110,7 +114,11 @@ xlog_get_bp( {struct xfs_buf *bp; -if (!xlog_buf_bbcount_valid (log, nbblks)) { +/* +* Pass log block 0 since we don't have an addr yet, buffer will be +* verified on read. +*/ +if (!xlog_verify_bp (log,0 , nbblks)) {xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);@@ -180,9 +188,10 @@ xlog_bread_noalign( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }@@ -265,9 +274,10 @@ xlog_bwrite( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation
Quoted 54 lines · Darrick J. Wong
Quoted 53 lines · Brian Foster
mkfs has a historical problem where it can format very small filesystems with too small of a physical log. Under certain conditions, log recovery of an associated filesystem can end up passing garbage parameter values to some of the cycle and log record verification functions due to bugs in log recovery not dealing with such filesystems properly. This results in attempts to read from bogus/underflowed log block addresses.
Since the buffer read may ultimately succeed, log recovery can proceed with bogus data and otherwise go off the rails and crash. One example of this is a negative
last_blkbeing passed toxlog_find_verify_log_record()causing us to skip the loop, pass a NULL head pointer toxlog_header_check_mount()and crash.Improve the xlog buffer verification to address this problem. We already verify xlog buffer length, so update this mechanism to also sanity check for a valid log relative block address and otherwise return an error. Pass a fixed, valid log block address from
xlog_get_bp()since the target address will be validated when the buffer is read. This ensures that any bogus log block address/length calculations lead to graceful mount failure rather than risking a crash or worse if recovery proceeds with bogus data.Reported-by Zorro Lang <[email protected]>Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)fs/xfs/xfs_log_recover.c +6
-6@@ -85,17 +85,21 @@ struct xfs_buf_cancel {*/ /* -* Verify the given count of basic blocks is valid number of blocks -* to specify for an operation involving the given XFS log buffer. -* Returns nonzero if the count is valid, 0 otherwise. +* Verify the log-relative block number and length in basic blocks are valid for +* an operation involving the given XFS log buffer. Returns true if the fields +* are valid, false otherwise. */ - -static inline int -xlog_buf_bbcount_valid ( +static inline bool +xlog_verify_bp (struct xlog *log, +xfs_daddr_t blk_no,int bbcount) {
Quoted 9 lines
fs/xfs/xfs_log_recover.c struct xfs_buf_cancel { · line 101
Quoted 5 lines
fs/xfs/xfs_log_recover.c +4
-1-return bbcount >0 && bbcount <= log->l_logBBsize; +if (blk_no <0 || blk_no >= log->l_logBBsize) +return false; +if (bbcount <=0 || bbcount > log->l_logBBsize) +return false;Shouldn't this be (
blk_no+ bbcount) >log->l_logBBsize, since the blk_no/bbcount parameters identify an extent within the log?
Yes, I suppose we can do that since we pass blk_no = 0 from the get_bp() case. The new invocations pass the blockcount of the requested I/O operation (as opposed to the buffer size, which is probably what I was thinking), so that seems reasonable to me.
Brian
Quoted 54 lines
--D
Quoted 52 lines
fs/xfs/xfs_log_recover.c +14
-7+return true; }/* @@ -110,7 +114,11 @@ xlog_get_bp( {struct xfs_buf *bp; -if (!xlog_buf_bbcount_valid (log, nbblks)) { +/* +* Pass log block 0 since we don't have an addr yet, buffer will be +* verified on read. +*/ +if (!xlog_verify_bp (log,0 , nbblks)) {xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);@@ -180,9 +188,10 @@ xlog_bread_noalign( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }@@ -265,9 +274,10 @@ xlog_bwrite( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 1/1 xfs: more robust recovery xlog buffer validation
In reply to Brian Fostermkfs has a historical problem where it can format very small filesystems with too small of a physical log. Under certain conditions, log recovery of an associated filesystem can end up passing garbage parameter values to some of the cycle and log record verification functions due to bugs in log recovery not dealing with such filesystems properly. This results in attempts to read from bogus/underflowed log block addresses.
Since the buffer read may ultimately succeed, log recovery can proceed with bogus data and otherwise go off the rails and crash. One example of this is a negative last_blk being passed to xlog_find_verify_log_record() causing us to skip the loop, pass a NULL head pointer to xlog_header_check_mount() and crash.
Improve the xlog buffer verification to address this problem. We already verify xlog buffer length, so update this mechanism to also sanity check for a valid log relative block address and otherwise return an error. Pass a fixed, valid log block address from xlog_get_bp() since the target address will be validated when the buffer is read. This ensures that any bogus log block address/length calculations lead to graceful mount failure rather than risking a crash or worse if recovery proceeds with bogus data.
---
v3:
- Verify log buf I/O range is within log boundary.
fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)
fs/xfs/xfs_log_recover.c +24 -14
@@ -85,17 +85,21 @@ struct xfs_buf_cancel {*/ /* -* Verify the given count of basic blocks is valid number of blocks -* to specify for an operation involving the given XFS log buffer. -* Returns nonzero if the count is valid, 0 otherwise. +* Verify the log-relative block number and length in basic blocks are valid for +* an operation involving the given XFS log buffer. Returns true if the fields +* are valid, false otherwise. */ - -static inline int -xlog_buf_bbcount_valid ( +static inline bool +xlog_verify_bp (struct xlog *log, +xfs_daddr_t blk_no,int bbcount) { -return bbcount >0 && bbcount <= log->l_logBBsize; +if (blk_no <0 || blk_no >= log->l_logBBsize) +return false; +if (bbcount <=0 || (blk_no + bbcount) > log->l_logBBsize) +return false; +return true; }/*
@@ -110,7 +114,11 @@ xlog_get_bp( {struct xfs_buf *bp; -if (!xlog_buf_bbcount_valid (log, nbblks)) { +/* +* Pass log block 0 since we don't have an addr yet, buffer will be +* verified on read. +*/ +if (!xlog_verify_bp (log,0 , nbblks)) {xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);
@@ -180,9 +188,10 @@ xlog_bread_noalign( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }
@@ -265,9 +274,10 @@ xlog_bwrite( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }
Signature · 1 line
2.9.5
Re: [PATCH v3] xfs: more robust recovery xlog buffer validation
Quoted 23 lines · Brian Foster
mkfs has a historical problem where it can format very small filesystems with too small of a physical log. Under certain conditions, log recovery of an associated filesystem can end up passing garbage parameter values to some of the cycle and log record verification functions due to bugs in log recovery not dealing with such filesystems properly. This results in attempts to read from bogus/underflowed log block addresses.
Since the buffer read may ultimately succeed, log recovery can proceed with bogus data and otherwise go off the rails and crash. One example of this is a negative
last_blkbeing passed toxlog_find_verify_log_record()causing us to skip the loop, pass a NULL head pointer toxlog_header_check_mount()and crash.Improve the xlog buffer verification to address this problem. We already verify xlog buffer length, so update this mechanism to also sanity check for a valid log relative block address and otherwise return an error. Pass a fixed, valid log block address from
xlog_get_bp()since the target address will be validated when the buffer is read. This ensures that any bogus log block address/length calculations lead to graceful mount failure rather than risking a crash or worse if recovery proceeds with bogus data.
Quoted 2 lines
Reported-by Zorro Lang <[email protected]>Signed-off-by Brian Foster <[email protected]>
Looks ok,
Quoted 89 lines
---
v3:
- Verify log buf I/O range is within log boundary.fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)fs/xfs/xfs_log_recover.c +24
-14@@ -85,17 +85,21 @@ struct xfs_buf_cancel {*/ /* -* Verify the given count of basic blocks is valid number of blocks -* to specify for an operation involving the given XFS log buffer. -* Returns nonzero if the count is valid, 0 otherwise. +* Verify the log-relative block number and length in basic blocks are valid for +* an operation involving the given XFS log buffer. Returns true if the fields +* are valid, false otherwise. */ - -static inline int -xlog_buf_bbcount_valid ( +static inline bool +xlog_verify_bp (struct xlog *log, +xfs_daddr_t blk_no,int bbcount) { -return bbcount >0 && bbcount <= log->l_logBBsize; +if (blk_no <0 || blk_no >= log->l_logBBsize) +return false; +if (bbcount <=0 || (blk_no + bbcount) > log->l_logBBsize) +return false; +return true; }/* @@ -110,7 +114,11 @@ xlog_get_bp( {struct xfs_buf *bp; -if (!xlog_buf_bbcount_valid (log, nbblks)) { +/* +* Pass log block 0 since we don't have an addr yet, buffer will be +* verified on read. +*/ +if (!xlog_verify_bp (log,0 , nbblks)) {xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);@@ -180,9 +188,10 @@ xlog_bread_noalign( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }@@ -265,9 +274,10 @@ xlog_bwrite( {int error; -if (!xlog_buf_bbcount_valid (log, nbblks)) { -xfs_warn (log->l_mp,"Invalid block length (0x%x) for buffer" , - nbblks); +if (!xlog_verify_bp (log, blk_no, nbblks)) { +xfs_warn (log->l_mp, +"Invalid log block/length (0x%llx, 0x%x) for buffer" , + blk_no, nbblks);XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_HIGH , log->l_mp);return -EFSCORRUPTED ; }Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 2/3 xfs: fix log block underflow during recovery cycle verification
In reply to Brian FosterIt is possible for mkfs to format very small filesystems with too small of an internal log with respect to the various minimum size and block count requirements. If this occurs when the log happens to be smaller than the scan window used for cycle verification and the scan wraps the end of the log, the start_blk calculation in xlog_find_head() underflows and leads to an attempt to scan an invalid range of log blocks. This results in log recovery failure and a failed mount.
Since there may be filesystems out in the wild with this kind of geometry, we cannot simply refuse to mount. Instead, cap the scan window for cycle verification to the size of the physical log. This ensures that the cycle verification proceeds as expected when the scan wraps the end of the log.
--- fs/xfs/xfs_log_recover.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_log_recover.c +1 -1
@@ -763,7 +763,7 @@ xlog_find_head(* in the in-core log. The following number can be made tighter if * we actually look at the block size of the filesystem. */ - num_scan_bblks =XLOG_TOTAL_REC_SHIFT (log); + num_scan_bblks =min_t (int, log_bbnum,XLOG_TOTAL_REC_SHIFT (log));if (head_blk >= num_scan_bblks) {/* * We are guaranteed that the entire check can be performed
Signature · 1 line
2.9.5
PATCH v2 3/3 xfs: drain the buffer LRU on mount
In reply to Brian FosterLog recovery of v4 filesystems does not use buffer verifiers because log recovery historically can result in transient buffer corruption when target buffers might be ahead of the log after a crash. v5 filesystems work around this problem with metadata LSN ordering.
While this log recovery verifier behavior is necessary on v4 supers, it can result in leaving buffers around in the LRU without verifiers attached for a significant amount of time. This leads to use of unverified buffers while the filesystem is in active use, long after recovery has completed.
To address this problem, drain all buffers from the LRU as a final step of the log mount sequence. Note that this is done unconditionally to provide a consistently clean cache footprint, regardless of superblock version or log state. As a side effect, this ensures that all cache resident, unverified buffers are reclaimed after log recovery and therefore must be recreated with verifiers on subsequent use.
--- fs/xfs/xfs_log.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
fs/xfs/xfs_log.c +16 -0
@@ -744,6 +744,7 @@ xfs_log_mount_finish( {int error =0 ;bool readonly = (mp->m_flags &XFS_MOUNT_RDONLY ); +bool recovered = mp->m_log->l_flags &XLOG_RECOVERY_NEEDED ;if (mp->m_flags &XFS_MOUNT_NORECOVERY ) {ASSERT (mp->m_flags &XFS_MOUNT_RDONLY );
@@ -780,6 +781,21 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;evict_inodes (mp->m_super); +/* +* Drain the buffer LRU after log recovery. This is required for v4 +* filesystems to avoid leaving around buffers with NULL verifier ops, +* but we do it unconditionally to make sure we're always in a clean +* cache state after mount. +* +* Don't push in the error case because the AIL may have pending intents +* that aren't removed until recovery is cancelled. +*/ +if (!error && recovered) { +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail); + } +xfs_wait_buftarg (mp->m_ddev_targp); +if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ;
Signature · 1 line
2.9.5
Re: [PATCH v2 3/3] xfs: drain the buffer LRU on mount
Quoted 19 lines · Brian Foster
Log recovery of v4 filesystems does not use buffer verifiers because log recovery historically can result in transient buffer corruption when target buffers might be ahead of the log after a crash. v5 filesystems work around this problem with metadata LSN ordering.
While this log recovery verifier behavior is necessary on v4 supers, it can result in leaving buffers around in the LRU without verifiers attached for a significant amount of time. This leads to use of unverified buffers while the filesystem is in active use, long after recovery has completed.
To address this problem, drain all buffers from the LRU as a final step of the log mount sequence. Note that this is done unconditionally to provide a consistently clean cache footprint, regardless of superblock version or log state. As a side effect, this ensures that all cache resident, unverified buffers are reclaimed after log recovery and therefore must be recreated with verifiers on subsequent use.
Quoted 2 lines
Reported-by Darrick Wong <[email protected]>Signed-off-by Brian Foster <[email protected]>
Looks ok,
Quoted 45 lines
--- fs/xfs/xfs_log.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)fs/xfs/xfs_log.c +16
-0@@ -744,6 +744,7 @@ xfs_log_mount_finish( {int error =0 ;bool readonly = (mp->m_flags &XFS_MOUNT_RDONLY ); +bool recovered = mp->m_log->l_flags &XLOG_RECOVERY_NEEDED ;if (mp->m_flags &XFS_MOUNT_NORECOVERY ) {ASSERT (mp->m_flags &XFS_MOUNT_RDONLY );@@ -780,6 +781,21 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;evict_inodes (mp->m_super); +/* +* Drain the buffer LRU after log recovery. This is required for v4 +* filesystems to avoid leaving around buffers with NULL verifier ops, +* but we do it unconditionally to make sure we're always in a clean +* cache state after mount. +* +* Don't push in the error case because the AIL may have pending intents +* that aren't removed until recovery is cancelled. +*/ +if (!error && recovered) { +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail); + } +xfs_wait_buftarg (mp->m_ddev_targp); +if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ;Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html