xfs
[Top] [All Lists]

Re: xfs_repair deleting realtime files.

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: xfs_repair deleting realtime files.
From: Anand Tiwari <tiwarikanand@xxxxxxxxx>
Date: Tue, 2 Oct 2012 10:47:18 -0600 (MDT)
Cc: Anand Tiwari <tiwarikanand@xxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:x-x-sender:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version:content-type; bh=5PX8luTRAYxUFUGFQvwzO2Px2PKYLKwfpzPbWn0RK4M=; b=Y2bGOQPz9jj+6+vKyEST9CVLvEVYNmtp3OrUVRnvvRnV0o7krQ9SiCJ+Z9uzEXn827 OQJm0ojMkvSbOfqY+5VmDj8uTZEDxGRGRSjV2wT2zFWScHIzSpaOUd+kQ4FATCTDqPft EiD+2qPhf3HqQiW/EF0/5r9gw+rjzb6T86LfbC62WCQS/6mi2VJtTN+0Lrg18oxw4601 NfeHLJhwNUur6mqlPbB4I5tmpbJt/e3wdGI/2LMrnVTVYc4ai/raC9qHGp26sbtbg1qz ujuxp/dnmC3VvY9Io9Mb2ahBzysvDeS83CMupMXaNF+Wi0c/kFlGi3IXvOfgHdDJ6vhA wM7g==
In-reply-to: <20121002005906.GL23520@dastard>
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> <20121002005906.GL23520@dastard>
User-agent: Alpine 2.02 (DEB 1266 2009-07-14)

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

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