Question about 67dc288c ("xfs: ensure verifiers are attached to recovered buffers")
Hi all,
I have a question about 67dc288c ("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems from xfs_scrub_buffer_recheck (it's in scrub part 4) being unable to find a b_ops attached to the AGF buffer and signalling error.
The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of xfs_buf_read with a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually, xlog_recover_validate_buf_type gets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so the xfs_buf just sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.
If the v4 fs didn't need log recovery, the buffers get created with b_ops as you'd expect.
My question is, shouldn't xlog_recover_validate_buf_type unconditionally set b_ops and save the "if (hascrc)" bits for the part that ensures the LSN is up to date?
It seems like a bad idea to let buffers sit around with no verifier. The original patch adding this function is d75afeb3 ("xfs: add buffer types to directory and attribute buffers") and looks like it was supposed to do this for any filesystem, but I wasn't around to know the evolution of that part of xlog.
--D
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.
Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?
Seems reasonable, but I notice that the has_crc() check around _validate_buf_type() comes in sometime after the the original commit referenced below (d75afeb3) and commit 67dc288c. It appears to be due to commit 9222a9cf86 ("xfs: don't shutdown log recovery on validation errors").
IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
Brian
Quoted 11 lines
It seems like a bad idea to let buffers sit around with no verifier. The original patch adding this function is
d75afeb3("xfs: add buffer types to directory and attribute buffers") and looks like it was supposed to do this for any filesystem, but I wasn't around to know the evolution of that part of xlog.--D
--
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
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add b_ops later if they're missing?
fs/xfs/xfs_buf.c +9 -0
@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops; +if (!(bp->b_flags &XBF_DONE )) {XFS_STATS_INC (target->bt_mount, xb_get_read); bp->b_ops = ops;
(Sort of a hack, I think I prefer something along the lines of #2 better.)
--D
Quoted 18 lines
Brian
Quoted 11 lines
It seems like a bad idea to let buffers sit around with no verifier. The original patch adding this function is
d75afeb3("xfs: add buffer types to directory and attribute buffers") and looks like it was supposed to do this for any filesystem, but I wasn't around to know the evolution of that part of xlog.--D
--
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--
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
Quoted 78 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +9
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops; +if (!(bp->b_flags &XBF_DONE )) {XFS_STATS_INC (target->bt_mount, xb_get_read); bp->b_ops = ops;
Quoted 2 lines
(Sort of a hack, I think I prefer something along the lines of #2 better.)
Yeah, seems like a plausible fix from a technical perspective. The first thing that comes to mind is the need to verify that nothing actually looks up an in-core buffer rather than attempt to read it and could somehow use it from there. That may not be the case or even likely going forward, but you never know how code is going to change in the future. The fact that it's not a relevant concern with the other approaches is what makes me give slight preference to those, fwiw.
OTOH, a variant of this that say iterated the cached buffers and did a post-recovery "validation" to attach any missing verifiers might be a way to avoid that flaw. As Dave kind of alludes to in his reply, that just might be more work/code than it's really worth.
Brian
Quoted 24 lines
--D
Quoted 14 lines
Brian
Quoted 11 lines
It seems like a bad idea to let buffers sit around with no verifier. The original patch adding this function is
d75afeb3("xfs: add buffer types to directory and attribute buffers") and looks like it was supposed to do this for any filesystem, but I wasn't around to know the evolution of that part of xlog.--D
--
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.htmlQuoted 4 lines
--
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--
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
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {
fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;
I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
--D
Quoted 10 lines
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] -- 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
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
I'll add it to my todo list but FYI I have very limited time this week, it's more likely I won't actually get to it until next week.
Brian
Quoted 16 lines
--D
Quoted 10 lines
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] -- 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--
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
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
What do you guys think about something like this? It runs unconditionally, but this is partly as designed to provide a clean cache on v5 filesystems as well. It should be mostly unnoticeable if log recovery hasn't run, but we could easily add another flag to make it conditional on log recovery if desired.
Brian
--- 8< ---
fs/xfs/xfs_log.c +10 -0
@@ -780,6 +780,16 @@ 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. Do it unconditionally to make sure we're always in a clean cache +* state after mount. +*/ +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 ;
Quoted 108 lines · Brian Foster
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
What do you guys think about something like this? It runs unconditionally, but this is partly as designed to provide a clean cache on v5 filesystems as well. It should be mostly unnoticeable if log recovery hasn't run, but we could easily add another flag to make it conditional on log recovery if desired.
Brian
--- 8< ---
fs/xfs/xfs_log.c +0
-0@@ -780,6 +780,16 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 790
Quoted 10 lines
fs/xfs/xfs_log.c +8
-0evict_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. Do it unconditionally to make sure we're always in a clean cache +* state after mount. +*/ +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail);
Is it necessary to force the log & push the AIL even if recovery didn't run?
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 791
Quoted 1 line
fs/xfs/xfs_log.c +1
-0+xfs_wait_buftarg (mp->m_ddev_targp);
Otherwise, looks ok enough I'll see if it fixes the scrub failures that inspired my original email. :)
--D
Quoted 8 lines
fs/xfs/xfs_log.c +1
-1+if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ; --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
Quoted 111 lines · Darrick J. Wong
Quoted 110 lines · Brian Foster
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
What do you guys think about something like this? It runs unconditionally, but this is partly as designed to provide a clean cache on v5 filesystems as well. It should be mostly unnoticeable if log recovery hasn't run, but we could easily add another flag to make it conditional on log recovery if desired.
Brian
--- 8< ---
fs/xfs/xfs_log.c +0
-0@@ -780,6 +780,16 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;evict_inodes (mp->m_super);
Quoted 11 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 790
Quoted 8 lines
fs/xfs/xfs_log.c +8
-0+/* +* Drain the buffer LRU after log recovery. This is required for +* v4 filesystems to avoid leaving around buffers with NULL verifier +* ops. Do it unconditionally to make sure we're always in a clean cache +* state after mount. +*/ +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail);Is it necessary to force the log & push the AIL even if recovery didn't run?
Don't think so.. As noted above, I just did this because it's basically unnoticeable if log recovery didn't run and it avoids the need to add a flag. I can add one if that is preferred, or thinking about it again I suppose we could just sample for XLOG_RECOVERY_NEEDED before we call xlog_recover_finish() (which clears it).
Quoted 5 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 791
Quoted 1 line
fs/xfs/xfs_log.c +1
-0+xfs_wait_buftarg (mp->m_ddev_targp);Otherwise, looks ok enough I'll see if it fixes the scrub failures that inspired my original email. :)
Thanks, let me know.
Brian
Quoted 10 lines
--D
Quoted 8 lines
fs/xfs/xfs_log.c +1
-1+if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ; --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
Quoted 130 lines · Brian Foster
Quoted 111 lines · Darrick J. Wong
Quoted 110 lines · Brian Foster
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
What do you guys think about something like this? It runs unconditionally, but this is partly as designed to provide a clean cache on v5 filesystems as well. It should be mostly unnoticeable if log recovery hasn't run, but we could easily add another flag to make it conditional on log recovery if desired.
Brian
--- 8< ---
fs/xfs/xfs_log.c +0
-0@@ -780,6 +780,16 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;evict_inodes (mp->m_super);Quoted 11 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 790
Quoted 8 lines
fs/xfs/xfs_log.c +8
-0+/* +* Drain the buffer LRU after log recovery. This is required for +* v4 filesystems to avoid leaving around buffers with NULL verifier +* ops. Do it unconditionally to make sure we're always in a clean cache +* state after mount. +*/ +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail);Is it necessary to force the log & push the AIL even if recovery didn't run?
Don't think so.. As noted above, I just did this because it's basically unnoticeable if log recovery didn't run and it avoids the need to add a flag. I can add one if that is preferred, or thinking about it again I suppose we could just sample for
XLOG_RECOVERY_NEEDEDbefore we callxlog_recover_finish()(which clears it).
Quoted 7 lines
Quoted 5 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 791
Quoted 1 line
fs/xfs/xfs_log.c +1
-0+xfs_wait_buftarg (mp->m_ddev_targp);Otherwise, looks ok enough I'll see if it fixes the scrub failures that inspired my original email. :)
Thanks, let me know.
This seems to fix the scrub problems running generic/392 xfs/121 xfs/181 on a v4 filesystem.
(Note that you have to have built the djwong-devel branches of the kernel and xfsprogs in order for xfstests to trip over the verifier-less buffers. The particular patch is "xfs: scrub in-core metadata".)
--D
Quoted 17 lines
Brian
Quoted 10 lines
--D
Quoted 8 lines
fs/xfs/xfs_log.c +1
-1+if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ; --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--
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
Quoted 137 lines · Darrick J. Wong
Quoted 136 lines · Brian Foster
Quoted 111 lines · Darrick J. Wong
Quoted 110 lines · Brian Foster
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
What do you guys think about something like this? It runs unconditionally, but this is partly as designed to provide a clean cache on v5 filesystems as well. It should be mostly unnoticeable if log recovery hasn't run, but we could easily add another flag to make it conditional on log recovery if desired.
Brian
--- 8< ---
fs/xfs/xfs_log.c +0
-0@@ -780,6 +780,16 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;evict_inodes (mp->m_super);Quoted 11 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 790
Quoted 8 lines
fs/xfs/xfs_log.c +8
-0+/* +* Drain the buffer LRU after log recovery. This is required for +* v4 filesystems to avoid leaving around buffers with NULL verifier +* ops. Do it unconditionally to make sure we're always in a clean cache +* state after mount. +*/ +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail);Is it necessary to force the log & push the AIL even if recovery didn't run?
Don't think so.. As noted above, I just did this because it's basically unnoticeable if log recovery didn't run and it avoids the need to add a flag. I can add one if that is preferred, or thinking about it again I suppose we could just sample for
XLOG_RECOVERY_NEEDEDbefore we callxlog_recover_finish()(which clears it).Quoted 5 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 791
Quoted 1 line
fs/xfs/xfs_log.c +1
-0+xfs_wait_buftarg (mp->m_ddev_targp);Otherwise, looks ok enough I'll see if it fixes the scrub failures that inspired my original email. :)
Quoted 8 lines
Quoted 1 line
Thanks, let me know.
This seems to fix the scrub problems running generic/392 xfs/121 xfs/181 on a v4 filesystem.
(Note that you have to have built the djwong-devel branches of the kernel and xfsprogs in order for xfstests to trip over the verifier-less buffers. The particular patch is "xfs: scrub in-core metadata".)
Follow-up question: if xlog_recover_finish returns an error, there's no point in pushing the log, right? I think we're basically toast if that happens.
--D
Quoted 24 lines
--D
Quoted 9 lines
Brian
Quoted 6 lines
--D
Quoted 4 lines
fs/xfs/xfs_log.c +1
-0+if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ;Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
--
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--
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--
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
Quoted 143 lines · Darrick J. Wong
Quoted 142 lines · Darrick J. Wong
Quoted 130 lines · Brian Foster
Quoted 111 lines · Darrick J. Wong
Quoted 110 lines · Brian Foster
Quoted 87 lines · Darrick J. Wong
Quoted 79 lines · Dave Chinner
Quoted 62 lines · Darrick J. Wong
Quoted 46 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures. This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
Dave might have more historical context to confirm that... If that is still an open issue, a couple initial ideas come to mind:
Quoted 6 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
What do you think about 3) add
b_opslater if they're missing?fs/xfs/xfs_buf.c +0
-0@@ -742,6 +742,15 @@ xfs_buf_read_map(if (bp) {fs/xfs/xfs_buf.c xfs_buf_read_map( · line 752
Quoted 10 lines
fs/xfs/xfs_buf.c +8
-0trace_xfs_buf_read (bp, flags, _RET_IP_); +/* +* If this buffer is up to date and has no verifier, try +* to set one. This can happen on v4 because log +* recovery reads in the buffers for replay but does not +* set b_ops. +*/ +if ((bp->b_flags &XBF_DONE ) && !bp->b_ops) + bp->b_ops = ops;I don't really like this because it will hide bugs in the code.
It also doesn't solve the problem because a read with NULL ops will still leave a buffer with no ops attached.
Quoted 6 lines
IMO, if we've read/created a buffer without ops, then it is up to the code that created/read it to either attach the required ops before the buffer is released or to invalidate the buffer before anyone else can use it or write it. The buffer write code warns about writing buffers without verfiers, but we can't warn on read because read-with-NULL-verifier is a valid thing to do....
Quoted 3 lines
Fair 'nuff. FWIW I'm ok with approach #1 if anyone enthusiastically wants to write it up.
What do you guys think about something like this? It runs unconditionally, but this is partly as designed to provide a clean cache on v5 filesystems as well. It should be mostly unnoticeable if log recovery hasn't run, but we could easily add another flag to make it conditional on log recovery if desired.
Brian
--- 8< ---
fs/xfs/xfs_log.c +0
-0@@ -780,6 +780,16 @@ xfs_log_mount_finish( mp->m_super->s_flags &= ~MS_ACTIVE ;evict_inodes (mp->m_super);Quoted 11 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 790
Quoted 8 lines
fs/xfs/xfs_log.c +8
-0+/* +* Drain the buffer LRU after log recovery. This is required for +* v4 filesystems to avoid leaving around buffers with NULL verifier +* ops. Do it unconditionally to make sure we're always in a clean cache +* state after mount. +*/ +xfs_log_force (mp,XFS_LOG_SYNC ); +xfs_ail_push_all_sync (mp->m_ail);Is it necessary to force the log & push the AIL even if recovery didn't run?
Don't think so.. As noted above, I just did this because it's basically unnoticeable if log recovery didn't run and it avoids the need to add a flag. I can add one if that is preferred, or thinking about it again I suppose we could just sample for
XLOG_RECOVERY_NEEDEDbefore we callxlog_recover_finish()(which clears it).Quoted 7 lines
Quoted 5 lines
fs/xfs/xfs_log.c xfs_log_mount_finish( · line 791
Quoted 1 line
fs/xfs/xfs_log.c +1
-0+xfs_wait_buftarg (mp->m_ddev_targp);Otherwise, looks ok enough I'll see if it fixes the scrub failures that inspired my original email. :)
Thanks, let me know.
This seems to fix the scrub problems running generic/392 xfs/121 xfs/181 on a v4 filesystem.
Quoted 8 lines
Quoted 3 lines
(Note that you have to have built the djwong-devel branches of the kernel and xfsprogs in order for xfstests to trip over the verifier-less buffers. The particular patch is "xfs: scrub in-core metadata".)
Follow-up question: if
xlog_recover_finishreturns an error, there's no point in pushing the log, right? I think we're basically toast if that happens.
I think we want to push the log/ail in any case that we call xfs_wait_buftarg() after starting recovery to make sure that all dirty buffers can be reclaimed immediately. We could skip the whole thing from here in the error case since the mount is going to fail, but I don't think it really matters. We'll end up in xfs_log_quiesce() as part of the mount unwind anyways, which basically ends up doing the same thing.
Brian
Quoted 30 lines
--D
Quoted 16 lines
--D
Quoted 13 lines
Brian
Quoted 10 lines
--D
Quoted 8 lines
fs/xfs/xfs_log.c +1
-1+if (readonly) mp->m_flags |=XFS_MOUNT_RDONLY ; --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.htmlQuoted 8 lines
Quoted 4 lines
--
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--
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--
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
Quoted 33 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").
Quoted 6 lines
IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures.
Yes, that is one of the problems - we can get writeback of partially updated buffers mid-way through log recovery on v4 filesystems.
Quoted 4 lines
This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
In a nutshell.
Quoted 1 line
Dave might have more historical context to confirm that...
Historically it only occurred (rarely) due to memory pressure triggering writeback during recovery. However, when we changed to context specific delayed write buffer lists we started doing that writeback after every checkpoint was recovered. Hence it's now pretty trivial to trigger verifier failures during log recovery on v4 filesystems...
Quoted 5 lines
If that is
still an open issue, a couple initial ideas come to mind:1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
This might be a worthwhile thing to do, anyway. Log recovery can lead to a lot of cached metadata that won't be referenced again after reocvery is complete. Perhaps we should just clear the buffer cache after the first phase of recovery just before/after we re-read the superblock and re-init the incore space accounting...
Quoted 3 lines
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
I'd prefer "return to clean slate" than have to handle log recovery state specially in every verifier. It's simple, it's easy to maintain, and it creates a barrier between metadata recovered from the log and post-processing of intents/unlinks that ensures we've made all the recovered changes stable on disk before we move on...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 63 lines · Dave Chinner
Quoted 33 lines · Brian Foster
Quoted 18 lines · Darrick J. Wong
Hi all,
I have a question about
67dc288c("xfs: ensure verifiers are attached to recovered buffers"). I was analyzing a scrub failure on generic/392 with a v4 filesystem which stems fromxfs_scrub_buffer_recheck(it's in scrub part 4) being unable to find ab_opsattached to the AGF buffer and signalling error.The pattern I observe is that when log recovery runs on a v4 filesystem, we call some variant of
xfs_buf_readwith a NULL ops parameter. The buffer therefore gets created and read without any verifiers. Eventually,xlog_recover_validate_buf_typegets called, and on a v5 filesystem we come back and attach verifiers and all is well. However, on a v4 filesystem the function returns without doing anything, so thexfs_bufjust sits around in memory with no verifier. Subsequent read/log/relse patterns can write anything they want without write verifiers to check that.Quoted 7 lines
If the v4 fs didn't need log recovery, the buffers get created with
b_opsas you'd expect.My question is, shouldn't
xlog_recover_validate_buf_typeunconditionally setb_opsand save the "if (hascrc)" bits for the part that ensures the LSN is up to date?Seems reasonable, but I notice that the
has_crc()check around_validate_buf_type()comes in sometime after the the original commit referenced below (d75afeb3) and commit67dc288c. It appears to be due to commit9222a9cf86("xfs: don't shutdown log recovery on validation errors").Quoted 6 lines
IIRC, the problem there is that log recovery had traditionally always unconditionally replayed everything in the log over whatever resides in the fs. This actually meant that recovery could transiently corrupt buffers in certain cases if the target buffer happened to be relogged more than once and was already up to date, which leads to verification failures.
Yes, that is one of the problems - we can get writeback of partially updated buffers mid-way through log recovery on v4 filesystems.
Quoted 4 lines
This was addressed for v5 filesystems with LSN ordering rules, but the challenge for v4 filesystems was that there is no metadata LSN and thus no means to detect whether a buffer is already up to date with regard to a transaction in the log.
In a nutshell.
Quoted 1 line
Dave might have more historical context to confirm that...
Historically it only occurred (rarely) due to memory pressure triggering writeback during recovery. However, when we changed to context specific delayed write buffer lists we started doing that writeback after every checkpoint was recovered. Hence it's now pretty trivial to trigger verifier failures during log recovery on v4 filesystems...
Quoted 3 lines
If that is
still an open issue, a couple initial ideas come to mind:
Quoted 9 lines
Quoted 2 lines
1.) Do something simple/crude like reclaim all buffers after log recovery on v4 filesystems to provide a clean slate going forward.
This might be a worthwhile thing to do, anyway. Log recovery can lead to a lot of cached metadata that won't be referenced again after reocvery is complete. Perhaps we should just clear the buffer cache after the first phase of recovery just before/after we re-read the superblock and re-init the incore space accounting...
Ok. In that case, then perhaps doing something like this wouldn't need to be limited to just v4 filesystems.
Quoted 11 lines
Quoted 3 lines
2.) Unconditionally attach verifiers during recovery as originally done and wire up something generic that short circuits verifier invocations on v4 filesystems when log recovery is in progress.
I'd prefer "return to clean slate" than have to handle log recovery state specially in every verifier. It's simple, it's easy to maintain, and it creates a barrier between metadata recovered from the log and post-processing of intents/unlinks that ensures we've made all the recovered changes stable on disk before we move on...
Either of these seem reasonable to me so if there is additional reason to go with #1, then that works for me.
Just note that the intent of #2 above was not to modify every verifier to accommodate this situation. Rather, to consider a generic change such as not invoking the verifier under particular conditions. Modifying the individual verifiers might be more reasonable if we had a generic verifier abstraction as Darrick and I discussed a bit ago wrt to some unrelated changes that I don't recall, but even then I'm not sure I would consider it the most elegant option..
Brian
Quoted 10 lines
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] -- 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