Re: [PATCH] xfsrestore: fix multi stream support

Rich Johnston
2013-10-01T22:02:37Z

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