xfs
[Top] [All Lists]

Re: xfs_repair deleting realtime files.

To: Anand Tiwari <tiwarikanand@xxxxxxxxx>
Subject: Re: xfs_repair deleting realtime files.
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 2 Oct 2012 10:59:06 +1000
Cc: Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <alpine.DEB.2.02.1209291645330.7208@artemis>
References: <505BF45D.5050909@xxxxxxxxxxx> <20120924075551.GF20960@dastard> <CAHt31_8rEc93vpnbbKngY4uS0kAct3Z5A+2G0LmBzv5rWKdSfA@xxxxxxxxxxxxxx> <CAHt31__s2TNhXPa9JfDLdWPqr60Te9VDPKb4ieORF8JAL07YmQ@xxxxxxxxxxxxxx> <20120926024403.GH29154@dastard> <CAHt31__FPLsDdVCVwwLO0Sh9v_KK2pzU577nywFPAmf_C-gRcQ@xxxxxxxxxxxxxx> <20120926061732.GI29154@dastard> <CAHt31__59_XJw9EF7MniiYH5sS6CX44q9gGwxiqxx43k5OFZ4A@xxxxxxxxxxxxxx> <20120928064742.GE25626@dastard> <alpine.DEB.2.02.1209291645330.7208@artemis>
User-agent: Mutt/1.5.21 (2010-09-15)
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?

> +
>       /*
>        * 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));

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>