xfs
[Top] [All Lists]

Re: [PATCH 1/2] repair: fix the variable-width nlink array

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] repair: fix the variable-width nlink array
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 2 Mar 2012 20:41:08 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120228174223.GA6148@xxxxxxxxxxxxx>
References: <20120228174223.GA6148@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Feb 28, 2012 at 12:42:24PM -0500, Christoph Hellwig wrote:
> It looks like we currently never grow the variable-width nlink array
> if only the on-disk nlink size overflows 8 bits.  This leads to a major
> mess in nlink counting, and eventually an assert in phase7.
> 
> Replace the indirect all mess with a union that allows doing proper
> array arithmetics while we're at it.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Looks fairly sane to me. I can't see any obvious problems with the
change, though it might be worth adding an overflow warning to the
code:

> +     switch (irec->nlink_size) {
> +     case sizeof(__uint8_t):
> +             if (irec->ino_un.ex_data->counted_nlinks.un8[ino_offset] < 
> 0xff) {
> +                     irec->ino_un.ex_data->counted_nlinks.un8[ino_offset]++;
> +                     break;
> +             }
>               nlink_grow_8_to_16(irec);
> -             disk_nlink_16_set(irec, ino_offset, nlinks);
> -     } else
> -             irec->disk_nlinks[ino_offset] = nlinks;
> +             /*FALLTHRU*/
> +     case sizeof(__uint16_t):
> +             if (irec->ino_un.ex_data->counted_nlinks.un16[ino_offset] < 
> 0xffff) {
> +                     irec->ino_un.ex_data->counted_nlinks.un16[ino_offset]++;
> +                     break;
> +             }
> +             nlink_grow_16_to_32(irec);
> +             /*FALLTHRU*/
> +     case sizeof(__uint32_t):
> +             irec->ino_un.ex_data->counted_nlinks.un32[ino_offset]++;

To check for UINT_MAX before the increment....

> +     switch (irec->nlink_size) {
> +     case sizeof(__uint8_t):
> +             ASSERT(irec->ino_un.ex_data->counted_nlinks.un8[ino_offset] > 
> 0);
> +             refs = --irec->ino_un.ex_data->counted_nlinks.un8[ino_offset];
> +             break;
> +     case sizeof(__uint16_t):
> +             ASSERT(irec->ino_un.ex_data->counted_nlinks.un16[ino_offset] > 
> 0);
> +             refs = --irec->ino_un.ex_data->counted_nlinks.un16[ino_offset];
> +             break;
> +     case sizeof(__uint32_t):
> +             ASSERT(irec->ino_un.ex_data->counted_nlinks.un32[ino_offset] > 
> 0);
> +             refs = --irec->ino_un.ex_data->counted_nlinks.un32[ino_offset];
> +             break;
> +     default:
> +             ASSERT(0);

Similar to the underflow checks here.

Otherwise:

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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