xfs
[Top] [All Lists]

Re: [PATCH 05/12] repair: update extent count after zapping duplicate bl

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 05/12] repair: update extent count after zapping duplicate blocks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 13 Dec 2011 13:12:07 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111202174741.904845954@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111202174619.179530033@xxxxxxxxxxxxxxxxxxxxxx> <20111202174741.904845954@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Dec 02, 2011 at 12:46:24PM -0500, Christoph Hellwig wrote:
> When we find a duplicate extent in an extern format inode we do not zap
> the whole inode, but just truncate it to the point where the duplicate
> extent was found.  But the current code only updates di_nblocks for the
> new size, but no di_nextents/di_anextents.  In most cases this isn't noticed,
> but when moving such an inode to the lost+found directoy the consistency
> check in xfs_iformat trips over it.  Fix this by updating the on-disk
> extent count as part of the inode repair.
> 
> Note that we zap btree format inodes with duplicate block completely
> at this point, so this fix doesn't apply to them.
> 
> Reported-by: Arkadiusz Mi??kiewicz <arekm@xxxxxxxx>
> Tested-by: Arkadiusz Mi??kiewicz <arekm@xxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

.....

> @@ -2003,6 +2016,12 @@ process_inode_blocks_and_extents(
>       xfs_ino_t       lino,
>       int             *dirty)
>  {
> +     if (nblocks < nextents + anextents) {
> +             do_warn(
> +_("nblocks (%" PRIu64 ") smaller than nextents for inode %" PRIu64 "\n"), 
> nblocks, lino);
> +             return 1;
> +     }
> +

I thikn that should be done after validating then block/extent
counts as otherwise it doesn't handle overflows.

It also took me a moment to realise the comparison was valid
- the confusion came from the fact you are comparing different units
(block count vs extent count). It didn't occur to me immediately
that this check is valid because an extent must contain one or
more blocks, so perhaps a comment is in order.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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