Re: [PATCH] xfsrestore: fix multi stream support

[Rich Johnston <rjohnston@sgi.com>]

OOPS hit send too soon.

On 10/01/2013 04:39 PM, Rich Johnston wrote:
>
>
> On 10/01/2013 03:47 PM, Eric Sandeen wrote:
>> Hi Rich -
>>
>> On 10/1/13 11:30 AM, Rich Johnston wrote:
>>> If no extents exist, there is no need to call partial_reg() becausee
>>> entire file
>>> there is no data to split up.
>> thought based on the code alone it was self explainitory
>> Does that break something, or is this an optimization?
>
> The original code is broken, would not detect if th.
                                                    the entire file was 
a hole (no extents) regardless of the value of partialmax.

 > If partialmax != 0
> (multi-stream) and no extents exist (entire file is a hole), is there
> anything to restore? Nope so why call parial_reg().  If you do call it
> you will not find anything to restore:
>
>
> 8977         /* If not found, find a free one, fill it in and return */
> 8978         if ( ! isptr ) {
> 8979                 mlog(MLOG_NITTY | MLOG_NOLOCK,
> 8980                         "partial_reg: no entry found for %llu\n",
> ino);
> 8981                 /* find a free one */
> 8982                 for (i=0; i < partialmax; i++ ) {
> 8983                         if (persp->a.parrest[i].is_ino == 0) {
> 8984                                 int j;
> 8985
> 8986                                 isptr = &persp->a.parrest[i];
> 8987                                 isptr->is_ino = ino;
> 8988                                 persp->a.parrestcnt++;
> 8989
> 8990                                 /* Clear all endoffsets (this value is
> 8991                                  * used to decide if an entry is
> used or
> 8992                                  * not
> 8993                                  */
> 8994                                 for (j=0, bsptr=isptr->is_bs;
> 8995                                      j < drivecnt; j++, bsptr++) {
> 8996                                      bsptr->endoffset = 0;
> 8997                                 }
> 8998
> 8999                                 goto found;
> 9000                         }
> 9001                 }
> 9002
> 9003                 /* Should never get here. */
>
> And we reach the dreaded comment above :)
>
>>> Also remove the uneeded check in partial_reg() to detect if this is a
>>> multistream restore.
>>
>> Why is it unneeded?

> The check is unneeded because with my fix,  partial_reg will never be
> called if partialmax==0 which also means that . Do we really need the extra check?
Scratch the above :)
I meant to say the check was needed in the original code because of the 
bug explained above.

example:
create a directory with several files with at least 1 extent
create a file with no extents (i.e. touch empty_file)

Current code will fail for multistream dump/restore and will also fail 
for single stream if the partialmax == 0 check is removed from 
partial_reg().

In my opinion that check was just a workaround for single stream and no 
one tested an empty file with no extents, just file with one extent and 
the entire file is a hole.

--Rich

>
>>
>>  From a quick read, if partialmax == 0 that measn only 1 drive,
>> and no streams - so it does seem like partial_reg() would have
>> no work to do, so I'm a  little confused (but I'm also a total
>> n00b here).e
>>
>> This patch says it fixes multi stream support - what was broken?
>> Is there a testcase (or should there be) that shows the problem?
>>
>> I see changes but I don't know enough about xfsdump to know
>> what's broken & what's being fixed, can you explain a bit more?
>
> Sorry I thought that it was self explanatory ;)
>>
>> Thanks,
>> -Eric
>>
>>
>>> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
>>>
>>> diff --git a/restore/content.c b/restore/content.c
>>> index 54d933c..ecbcf13 100644
>>> --- a/restore/content.c
>>> +++ b/restore/content.c
>>> @@ -7494,6 +7494,7 @@ restore_extent_group( drive_t *drivep,
>>>       extenthdr_t ehdr;
>>>       off64_t bytesread;
>>>       rv_t rv;
>>> +    uint num_extents = 0; /* number of extents */
>>>
>>>       /* copy data extents from media to the file
>>>        */
>>> @@ -7518,6 +7519,7 @@ restore_extent_group( drive_t *drivep,
>>>           if ( ehdr.eh_type == EXTENTHDR_TYPE_LAST ) {
>>>               break;
>>>           }
>>> +        num_extents++;
>>>
>>>           /* if its an ALIGNment extent, discard the extent.
>>>            */
>>> @@ -7572,7 +7574,7 @@ restore_extent_group( drive_t *drivep,
>>>        * and certain extended inode flags. Register the portion
>>>        * of the file completed here in the persistent state.
>>>        */
>>> -    if (bstatp->bs_size > restoredsz) {
>>> +    if (num_extents && (bstatp->bs_size > restoredsz)) {
>>>           partial_reg(drivep->d_index,
>>>                   bstatp->bs_ino,
>>>                   bstatp->bs_size,
>>> @@ -8959,9 +8961,6 @@ partial_reg( ix_t d_index,
>>>
>>>       endoffset = offset + sz;
>>>
>>> -    if ( partialmax == 0 )
>>> -        return;
>>> -
>>>       pi_lock();
>>>
>>>       /* Search for a matching inode.  Gaps can exist so we must search
>>>
>>> _______________________________________________
>>> xfs mailing list
>>> xfs@oss.sgi.com
>>> http://oss.sgi.com/mailman/listinfo/xfs
>>>
>>
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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