Re: xfs_repair deleting realtime files.

[Anand Tiwari <tiwarikanand@gmail.com>]

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@linux.com>
> > ---
> >  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@fromorbit.com
>

thanks a bunch,
anand

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs