xfs
[Top] [All Lists]

Re: [PATCH] xfs_copy: simplify first_agbno calculation

To: Brian Foster <bfoster@xxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] xfs_copy: simplify first_agbno calculation
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue, 11 Nov 2014 10:01:55 -0600
Cc: Zorro Lang <zlang@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141111133705.GA38867@xxxxxxxxxxxxxxx>
References: <54613907.2000007@xxxxxxxxxx> <20141111133705.GA38867@xxxxxxxxxxxxxxx>
On 11/11/14 7:37 AM, Brian Foster wrote:
> On Mon, Nov 10, 2014 at 04:15:35PM -0600, Eric Sandeen wrote:
>> After ffe9a9a xfsprogs: xfs_copy: fix data corruption of target,
>> xfs_copy started hitting an ASSERT for a 4k sector / 4k blocksize
>> filesystem:
>>
>> # dd if=/dev/zero of=test.img bs=1M count=1024
>> # mkfs.xfs -s size=4096 test.img
>> # xfs_copy test.img xfs.img
>> xfs_copy: xfs_copy.c:720: main: Assertion `((((((xfs_daddr_t)(3 << 
>> (mp)->m_sectbb_log)) + 1) * (1<<9)) + first_residue) % source_blocksize) == 
>> 0' failed.
>> Aborted
>>
>> I started digging through all the calculations below, and realized
>> that in the end, all it wants is the first filesystem block after
>> the AG header.  XFS_AGFL_BLOCK(mp) + 1 suffices for this purpose;
>> rip out the rest which seems overly complex and apparently bug-prone.
>>
>> I tested this by creating a 4g filesystem with combinations of
>> sector & block size between 512 and 4k, copying in /lib/modules,
>> running an xfs_copy of that, and running repair against the copy;
>> it all looks good.  It took a long time, but I will create a
>> simpler/shorter xfstest based on this.
>>
>> Reported-by: Zorro Lang <zlang@xxxxxxxxxx>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
> 
> Looks Ok to me:
> 
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> 
> I just noticed the bug... so the problem was basically the assumption
> that sector size == BBSIZE?

To be honest, I never quite figured out what the root cause bug
was; bad me.  The previous commit was supposed to *fix* the assumption
that sector size == BBSIZE.  But I got lost in all the gyrations.

I guess maybe I should work that out, but I'm not sure it's worth
it.  ;)

-Eric

> Brian
> 
>>
>> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
>> index 7ce5ec9..279527c 100644
>> --- a/copy/xfs_copy.c
>> +++ b/copy/xfs_copy.c
>> @@ -475,7 +475,7 @@ main(int argc, char **argv)
>>      int             open_flags;
>>      xfs_off_t       pos, end_pos;
>>      size_t          length;
>> -    int             c, first_residue, tmp_residue;
>> +    int             c;
>>      __uint64_t      size, sizeb;
>>      __uint64_t      numblocks = 0;
>>      int             wblocks = 0;
>> @@ -697,27 +697,13 @@ main(int argc, char **argv)
>>      ASSERT(source_blocksize % source_sectorsize == 0);
>>      ASSERT(source_sectorsize % BBSIZE == 0);
>>  
>> -    if (source_blocksize > source_sectorsize)  {
>> -            /* get number of leftover sectors in last block of ag header */
>> -
>> -            tmp_residue = ((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
>> -                                    % source_blocksize;
>> -            first_residue = (tmp_residue == 0) ? 0 :
>> -                    source_blocksize - tmp_residue;
>> -            ASSERT(first_residue % source_sectorsize == 0);
>> -    } else if (source_blocksize == source_sectorsize)  {
>> -            first_residue = 0;
>> -    } else  {
>> +    if (source_blocksize < source_sectorsize)  {
>>              do_log(_("Error:  filesystem block size is smaller than the"
>>                      " disk sectorsize.\nAborting XFS copy now.\n"));
>>              exit(1);
>>      }
>>  
>> -    first_agbno = (((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
>> -                            + first_residue) / source_blocksize;
>> -    ASSERT(first_agbno != 0);
>> -    ASSERT(((((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
>> -                            + first_residue) % source_blocksize) == 0);
>> +    first_agbno = XFS_AGFL_BLOCK(mp) + 1;
>>  
>>      /* now open targets */
>>  
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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