On Tue, 2 Oct 2012, Dave Chinner wrote:
> On Sat, Sep 29, 2012 at 04:49:03PM -0600, Anand Tiwari wrote:
> > Subject: [PATCH] xfs_repair: detect realtime extent shared by multiple
> > records in extent map
> >
> > XFS currently can have records in extent map, which starts from unaligned start block w.r.t rextsize.
> > xfs_repair considers this as a bug (multiple claims for a real-time extent) and deletes the file.
> > This patch addresses the issue, by comparing current and previous records and make sure they are
> > contiguous and not overlapped.
> >
> > Signed-off-by: Anand Tiwari <anand.tiwari@xxxxxxxxx>
> > ---
> > repair/dinode.c | 37 ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/repair/dinode.c b/repair/dinode.c
> > index 5a2da39..5537f1c 100644
> > --- a/repair/dinode.c
> > +++ b/repair/dinode.c
> > @@ -406,6 +406,7 @@ verify_agbno(xfs_mount_t *mp,
> > static int
> > process_rt_rec(
> > xfs_mount_t *mp,
> > + xfs_bmbt_irec_t *prev,
> > xfs_bmbt_irec_t *irec,
> > xfs_ino_t ino,
> > xfs_drfsbno_t *tot,
> > @@ -413,8 +414,11 @@ process_rt_rec(
> > {
> > xfs_dfsbno_t b;
> > xfs_drtbno_t ext;
> > + xfs_drtbno_t start_block;
> > + xfs_filblks_t block_count;
> > int state;
> > int pwe; /* partially-written extent */
> > + int rtext_remainder; /* start block is not aligned w.r.t rextsize */
> >
> > /*
> > * check numeric validity of the extent
> > @@ -461,12 +465,32 @@ _("malformed rt inode extent [%" PRIu64 " %" PRIu64 "] (fs rtext size = %u)\n"),
> > return 1;
> > }
> >
> > + /* If we have start of record unaligned w.r.t to rextsize, see
> > + * if we are sharing this realtime extent with previous record. sharing is only
> > + * allowed with previous extent. fail otherwise.
> > + * Also, we above condition is true, align start block and block count
> > + */
>
> Comment format is:
>
> /*
> * this is a
> * multiline comment
> */
>
> > + rtext_remainder = irec->br_startblock % mp->m_sb.sb_rextsize;
> > + if (rtext_remainder) {
> > + do_warn(
> > +_("data fork in rt ino %" PRIu64 " has unalinged start block %"PRIu64 "\n"),
> unaligned
>
> > + ino,
> > + irec->br_startblock);
>
> indent these a bit so the fact they are part of the do_warn function
> call is obvious. i.e.
>
> do_warn(
> <format string>
> ino, irec->br_startblock);
>
> > + if ((prev->br_startoff + prev->br_blockcount == irec->br_startoff) &&
> > + (prev->br_startblock + prev->br_blockcount == irec->br_startblock)) {
> > + start_block = irec->br_startblock + (mp->m_sb.sb_rextsize - rtext_remainder);
> > + block_count = irec->br_blockcount - (mp->m_sb.sb_rextsize - rtext_remainder);
> > + }
> > + } else {
> > + start_block = irec->br_startblock;
> > + block_count = irec->br_blockcount;
> > + }
>
> So this just changes the loop below not to check the first part of
> the new extent and therefore avoid the duplicate extent usage
> detection?
>
> Any plans to correct the bmapbt extents so that repeated repair runs
> don't keep warning about the same problem?
>
I was planning on fixing kernel code first and tackle this later. Let me
know if you guys think otherwise.
> > +
> > /*
> > * set the appropriate number of extents
> > * this iterates block by block, this can be optimised using extents
> > */
> > - for (b = irec->br_startblock; b < irec->br_startblock +
> > - irec->br_blockcount; b += mp->m_sb.sb_rextsize) {
> > + for (b = start_block; b < start_block + block_count; b += mp->m_sb.sb_rextsize) {
> > ext = (xfs_drtbno_t) b / mp->m_sb.sb_rextsize;
> > pwe = xfs_sb_version_hasextflgbit(&mp->m_sb) &&
> > irec->br_state == XFS_EXT_UNWRITTEN &&
> > @@ -548,6 +572,7 @@ process_bmbt_reclist_int(
> > int check_dups,
> > int whichfork)
> > {
> > + xfs_bmbt_irec_t prev;
> > xfs_bmbt_irec_t irec;
> > xfs_dfilblks_t cp = 0; /* prev count */
> > xfs_dfsbno_t sp = 0; /* prev start */
> > @@ -574,6 +599,11 @@ process_bmbt_reclist_int(
> > else
> > ftype = _("regular");
> >
> > + prev.br_startoff = 0;
> > + prev.br_blockcount = 0;
> > + prev.br_startblock = 0;
> > + prev.br_state = 0;
>
> memset(&prev, 0, sizeof(prev));
>
which one is more preferred in XFS land,
a) xfs_bmbt_irec_t prev = {0};
b) memset(&prev, 0, sizeof(prev));
my vote is "a". I already addressed other suggestions.
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
thanks a bunch,
anand
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
