* [PATCH 1/2] xfs: set remote symlink buffer type for recovery
2013-09-02 0:31 [PATCH 0/2] xfs: log recovery buffer fixes Dave Chinner
@ 2013-09-02 0:32 ` Dave Chinner
2013-09-02 8:16 ` Christoph Hellwig
2013-09-02 0:32 ` [PATCH 2/2] xfs: ensure we copy buffer type in da btree root splits Dave Chinner
2013-09-10 22:41 ` [PATCH 0/2] xfs: log recovery buffer fixes Ben Myers
2 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2013-09-02 0:32 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
The logging of a remote symlink block does not set the buffer type
being logged, and hence on recovery the type of buffer is not
recognised and hence CRCs are not calculated after replay. This
results in log recoery throwing:
XFS (vdc): Unknown buffer type 0
errors, and subsequent reads of the symlink failing CRC
verification. Found via fsstress + godown.
Reported by: Michael L. Semon <mlsemon35@gmail.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_symlink.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 2f2a7c0..f622a97 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -41,6 +41,7 @@
#include "xfs_trans_space.h"
#include "xfs_trace.h"
#include "xfs_symlink.h"
+#include "xfs_buf_item.h"
/* ----- Kernel only functions below ----- */
STATIC int
@@ -363,6 +364,7 @@ xfs_symlink(
pathlen -= byte_cnt;
offset += byte_cnt;
+ xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SYMLINK_BUF);
xfs_trans_log_buf(tp, bp, 0, (buf + byte_cnt - 1) -
(char *)bp->b_addr);
}
--
1.8.3.2
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: set remote symlink buffer type for recovery
2013-09-02 0:32 ` [PATCH 1/2] xfs: set remote symlink buffer type for recovery Dave Chinner
@ 2013-09-02 8:16 ` Christoph Hellwig
2013-09-02 10:05 ` Dave Chinner
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2013-09-02 8:16 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Mon, Sep 02, 2013 at 10:32:00AM +1000, Dave Chinner wrote:
> errors, and subsequent reads of the symlink failing CRC
> verification. Found via fsstress + godown.
Can you make sure xfstests triggers this case, please?
> + xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SYMLINK_BUF);
> xfs_trans_log_buf(tp, bp, 0, (buf + byte_cnt - 1) -
> (char *)bp->b_addr);
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: set remote symlink buffer type for recovery
2013-09-02 8:16 ` Christoph Hellwig
@ 2013-09-02 10:05 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2013-09-02 10:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Sep 02, 2013 at 01:16:25AM -0700, Christoph Hellwig wrote:
> On Mon, Sep 02, 2013 at 10:32:00AM +1000, Dave Chinner wrote:
> > errors, and subsequent reads of the symlink failing CRC
> > verification. Found via fsstress + godown.
>
> Can you make sure xfstests triggers this case, please?
Yeah, I've got a script that reproduced it, so i shoul dbe able to
turn it into an xfstest easily enough...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs: ensure we copy buffer type in da btree root splits
2013-09-02 0:31 [PATCH 0/2] xfs: log recovery buffer fixes Dave Chinner
2013-09-02 0:32 ` [PATCH 1/2] xfs: set remote symlink buffer type for recovery Dave Chinner
@ 2013-09-02 0:32 ` Dave Chinner
2013-09-02 8:17 ` Christoph Hellwig
2013-09-10 22:41 ` [PATCH 0/2] xfs: log recovery buffer fixes Ben Myers
2 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2013-09-02 0:32 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
When splitting the root of the da btree, we shuffled data between
buffers and the structures that track them. At one point, we copy
data and state from one buffer to another, including the ops
aasociated with the buffer. When we do this, we also need to copy
the buffer type associated with the buf log item so that the buffer
is logged correctly. If we don't do that, log recovery won't
recognise it and hence it won't recalculate the CRC on the buffer
after recovery. This leads to a directory block that can't be read
after recovery has run.
Found by inspection after finding the same problem with remote
symlink buffers.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_da_btree.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index d4e59a4..069537c 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -635,6 +635,7 @@ xfs_da3_root_split(
xfs_trans_log_buf(tp, bp, 0, size - 1);
bp->b_ops = blk1->bp->b_ops;
+ xfs_trans_buf_copy_type(bp, blk1->bp);
blk1->bp = bp;
blk1->blkno = blkno;
--
1.8.3.2
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: ensure we copy buffer type in da btree root splits
2013-09-02 0:32 ` [PATCH 2/2] xfs: ensure we copy buffer type in da btree root splits Dave Chinner
@ 2013-09-02 8:17 ` Christoph Hellwig
2013-09-02 10:09 ` Dave Chinner
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2013-09-02 8:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Mon, Sep 02, 2013 at 10:32:01AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When splitting the root of the da btree, we shuffled data between
> buffers and the structures that track them. At one point, we copy
> data and state from one buffer to another, including the ops
> aasociated with the buffer. When we do this, we also need to copy
> the buffer type associated with the buf log item so that the buffer
> is logged correctly. If we don't do that, log recovery won't
> recognise it and hence it won't recalculate the CRC on the buffer
> after recovery. This leads to a directory block that can't be read
> after recovery has run.
>
> Found by inspection after finding the same problem with remote
> symlink buffers.
It would be great to find a way to trigger this in QA as this shows
another area lacking coverage.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: ensure we copy buffer type in da btree root splits
2013-09-02 8:17 ` Christoph Hellwig
@ 2013-09-02 10:09 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2013-09-02 10:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Sep 02, 2013 at 01:17:25AM -0700, Christoph Hellwig wrote:
> On Mon, Sep 02, 2013 at 10:32:01AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > When splitting the root of the da btree, we shuffled data between
> > buffers and the structures that track them. At one point, we copy
> > data and state from one buffer to another, including the ops
> > aasociated with the buffer. When we do this, we also need to copy
> > the buffer type associated with the buf log item so that the buffer
> > is logged correctly. If we don't do that, log recovery won't
> > recognise it and hence it won't recalculate the CRC on the buffer
> > after recovery. This leads to a directory block that can't be read
> > after recovery has run.
> >
> > Found by inspection after finding the same problem with remote
> > symlink buffers.
>
> It would be great to find a way to trigger this in QA as this shows
> another area lacking coverage.
I'm pretty sure the same script I discovered the symlink problem was
triggering it. It was actually trying to track down an
assert failure in the directory code that Michael Semon had reported
to me with a rough test case that I then scripted.
I just haven't done enough testing to be certain it wasn't something
else. Also, xfs/182 was assert failures after log recovery that had
a similar signature that I also haven't seen since adding this
patch. So I think we got some coverage of it...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] xfs: log recovery buffer fixes
2013-09-02 0:31 [PATCH 0/2] xfs: log recovery buffer fixes Dave Chinner
2013-09-02 0:32 ` [PATCH 1/2] xfs: set remote symlink buffer type for recovery Dave Chinner
2013-09-02 0:32 ` [PATCH 2/2] xfs: ensure we copy buffer type in da btree root splits Dave Chinner
@ 2013-09-10 22:41 ` Ben Myers
2 siblings, 0 replies; 8+ messages in thread
From: Ben Myers @ 2013-09-10 22:41 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Mon, Sep 02, 2013 at 10:31:59AM +1000, Dave Chinner wrote:
> Folks, these two patches make sure log recovery correctly identifies
> the type of buffer being recovered. The buffer log format type field
> is not always being set correctly, and that leads to invalid remote
> symlink and directory blocks after recovery as they hav enot had the
> CRC recalculated after being recovered.
Applied these 2.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread