[PATCH] xfsrestore: fix multi stream support

[PATCH] xfsrestore: fix multi stream support

[Rich Johnston]

If no extents exist, there is no need to call partial_reg() because
there is no data to split up. Also remove the uneeded check in 
partial_reg() to detect if this is a multistream restore.

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

Re: [PATCH] xfsrestore: fix multi stream support

[Eric Sandeen]

Hi Rich -

On 10/1/13 11:30 AM, Rich Johnston wrote:
> If no extents exist, there is no need to call partial_reg() because
> there is no data to split up. 

Does that break something, or is this an optimization?

> Also remove the uneeded check in partial_reg() to detect if this is a multistream restore.

Why is it unneeded?

>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).

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?

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

Re: [PATCH] xfsrestore: fix multi stream support

[Rich Johnston]

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. 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. Do we really need the extra check?

>
>  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

Re: [PATCH] xfsrestore: fix multi stream support

[Rich Johnston]

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

Re: [PATCH] xfsrestore: fix multi stream support

[Eric Sandeen]

On 10/1/13 5:02 PM, Rich Johnston wrote:
> OOPS hit send too soon.

I'll fix it up as I go . . . 

> 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 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 :)

After that comment above, there's a warning:

                mlog( MLOG_NORMAL | MLOG_WARNING, _(
                  "partial_reg: Out of records. Extend attrs applied early.\n"));

So you saw that?  Is that the bug you're fixing?

But in my tests I don't hit that, even though I can hit this function
with a purely sparse file w/ no extents.

>>>> 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)

well, no; for an empty file, bs_size == restoredsz so we won't go to partial_reg.
But if you truncate --size=20m you'll see it.  But it works fine today AFAICT.

> 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 testing it's fine for a large, sparse file w/ multistream.

w/ some printf debugs, I see:

bs_size 20971520 restoredsz 0 /* so we go to partial_reg */
partial_reg: d_index = 3, ino = 137, fsize = 20971520, offset = 0, sz = 0

and it carries along just fine.

Silly to call partial_reg only to return, perhaps, but - no out right bug?

> 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.

well, it's a workaround for the fact that the test to call partial_reg
doesn't account for sparse files at all, I think.  :(

But I'm still not totally clear on what bug you're fixing.

I suppose if you can provide the testcase or the description
of the erroneous end-result, it might be clearer.

p.s. your patches are whitespace-mangled.  ;)

Thanks,
-Eric

here's the hacky sort of test I was doing to trigger the
go-to-partial-reg-with-no-extents code:

#!/bin/bash

# paths to binaries under test
DUMP=/mnt/test2/git/xfsdump/dump/xfsdump
RESTORE=/mnt/test2/git/xfsdump/restore/xfsrestore

# what we'll create files in & dump
DUMPDIR=/mnt/test
# where we'll restore
RESTOREDIR=/mnt/test2/restore

mkdir -p $DUMPDIR/dir
mkdir -p $RESTOREDIR

clean () {
	rm -rf $DUMPDIR/dir/*
	rm -rf $RESTOREDIR/*x
}

clean

xfs_io -f -c "pwrite 4k 4km" $DUMPDIR/dir/8ksparsefront

xfs_io -f -c "pwrite 0 4k" $DUMPDIR/dir/8ksparseend
truncate --size=8k $DUMPDIR/dir/8ksparseend

xfs_io -f -c "pwrite 0 20m" $DUMPDIR/dir/20mfile

xfs_io -f -c "pwrite 20m 20m" $DUMPDIR/dir/40msparsefront

xfs_io -f -c "pwrite 0 20m" $DUMPDIR/dir/40msparseend
truncate --size=40m $DUMPDIR/dir/40msparseend

truncate --size=20m $DUMPDIR/dir/sparsefile

touch $DUMPDIR/dir/emptyfile

rm -f stream1 stream2 stream3 stream4
$DUMP -L session -M label1 -M label2 -M label3 -M label4 -f stream1 -f stream2 -f stream3 -f stream4 $DUMPDIR
$RESTORE -F -f stream1 -f stream2 -f stream3 -f stream4 $RESTOREDIR

ls -1i $DUMPDIR/dir

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

Re: [PATCH] xfsrestore: fix multi stream support

[Eric Sandeen]

On 10/1/13 10:57 PM, Eric Sandeen wrote:

> #!/bin/bash
> 
> # paths to binaries under test
> DUMP=/mnt/test2/git/xfsdump/dump/xfsdump
> RESTORE=/mnt/test2/git/xfsdump/restore/xfsrestore
> 
> # what we'll create files in & dump
> DUMPDIR=/mnt/test
> # where we'll restore
> RESTOREDIR=/mnt/test2/restore
> 
> mkdir -p $DUMPDIR/dir
> mkdir -p $RESTOREDIR
> 
> clean () {
> 	rm -rf $DUMPDIR/dir/*
> 	rm -rf $RESTOREDIR/*x
> }
> 
> clean
> 
> xfs_io -f -c "pwrite 4k 4km" $DUMPDIR/dir/8ksparsefront

whoops typo, that should be "4k 4k" 

-Eric

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

Re: [PATCH] xfsrestore: fix multi stream support

[Eric Sandeen]

On 10/1/13 5:02 PM, Rich Johnston wrote:

>> 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:
>>


Sorry for the scattered replies.

But simply getting here w/ no extents doesn't make us reach the comment
/* Should never get here */ below.

first:

        /* Search for a matching inode.  Gaps can exist so we must search
         * all entries. 
         */
        for (i=0; i < partialmax; i++ ) {
                if (persp->a.parrest[i].is_ino == ino) {
                        isptr = &persp->a.parrest[i];
                        break;
                }
        }

so first it tries to find to see if this inode has another stream (?)
which has partially restored it.

If not:

>> 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) {

find a stream which doesn't have is_ino set

>> 8984                                 int j;
>> 8985
>> 8986                                 isptr = &persp->a.parrest[i];
>> 8987                                 isptr->is_ino = ino;

set 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

etc, and go to found, and everything's fine:

>> 8999                                 goto found;
>> 9000                         }
>> 9001                 }
>> 9002
>> 9003                 /* Should never get here. */
>>
>> And we reach the dreaded comment above :)

the only way to reach that comment & the associated warning is if the first
loop I pasted finds no matching is_ino on any stream, but the 2nd loop finds
no is_ino == 0.  i.e. persp->a.parrest[i].is_ino for every stream ("i") has
a different, non-zero is_ino.  How can that happen?  (I'm not sure...)

-Eric 

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

Re: [PATCH] xfsrestore: fix multi stream support

[Eric Sandeen]

Ok me again.  ;)  Here's a testcase:

#!/bin/bash

# paths to binaries under test
DUMP=/mnt/test2/git/xfsdump/dump/xfsdump
RESTORE=/mnt/test2/git/xfsdump/restore/xfsrestore

# dir we'll create files in & dump
DUMPDIR=/mnt/test
# dir where we'll restore
RESTOREDIR=/mnt/test2/restore

mkdir -p $DUMPDIR/dir
mkdir -p $RESTOREDIR
rm -rf $DUMPDIR/dir/*
rm -rf $RESTOREDIR/*

truncate --size=1t $DUMPDIR/dir/sparsefile1
truncate --size=1t $DUMPDIR/dir/sparsefile2
truncate --size=1t $DUMPDIR/dir/sparsefile3
truncate --size=1t $DUMPDIR/dir/sparsefile4

rm -f stream1 stream2
$DUMP -L session -M label1 -M label2 -f stream1 -f stream2 $DUMPDIR
$RESTORE -F -f stream1 -f stream2 $RESTOREDIR

---

so we go down this path:

restore_extent_group
	<loop over extent headers adding up restoredsz>
        if (bstatp->bs_size > restoredsz) {
                partial_reg()

In that loop, if we find DATA or HOLE, it advances "restoredsz" so it
generally does handle sparse files properly.

But a wholly sparse file has only the LAST header type, and resetoredsz
never moves.  This is important.  ;)  That's why the condition necessary
to go to partial_reg() is true.

in partial_reg(), with multiple streams, we check persp->a.parrest[i].is_ino
for each stream ("i") to see if the inode we're restoring i in any is_ino:

        /* Search for a matching inode.  Gaps can exist so we must search
         * all entries. 
         */
        for (i=0; i < partialmax; i++ ) {
                if (persp->a.parrest[i].is_ino == ino) {
                        isptr = &persp->a.parrest[i];
                        break;
                }
        }

If this is the first time we've hit this inode we won't find it, and so we fill
it in on one slot:

        /* If not found, find a free one, fill it in and return */
        if ( ! isptr ) {
                /* find a free one */
                for (i=0; i < partialmax; i++ ) {
                        if (persp->a.parrest[i].is_ino == 0) {
                                isptr->is_ino = ino;
				<snip>
                                goto found;
                        }
                }
                /* Should never get here. */
                pi_unlock();
                mlog( MLOG_NORMAL | MLOG_WARNING, _(
                  "partial_reg: Out of records. Extend attrs applied early.\n"));
#ifdef DEBUGPARTIALS
	}

Otherwise, we go to found:  which calls partial_check2().

So the only way we can not "find a free one" is if every a.parrest[i].is_ino
is set to something.  Well, we only have a few of them based on the number of 
streams; what clears it?  partial_check2(), which is looking to see if the file
is wholly restored:

        /* Check if all bytes are accounted for.  */
        if (curoffset >= fsize) {
                isptr->is_ino = 0;  /* clear the entry */

But since the wholly-sparse file had only a LAST record, and no HOLE, nothing
advanced, and it doesn't look "done" - so partial_check2() fails, we fill all
the slots, and we hit the dreaded "partial_reg: Out of records."

Bleah.

So I agree, this does seem to only happen with wholly-sparse files.

Adding a HOLE record for them would fix it, but that doesn't fix old dumps.

So I thought about doing something like this:


[PATCH] xfsdump: handle large, wholly-sparse files

In restore_extent_group(), we loop over all extent headers for an inode
in the stream, and add up the cumulatively restored size, accounting
for both HOLE and DATA records and advancing restoredsz as we go.

But for a wholly-sparse file, we have no HOLE header, only
a LAST header, and restoredsz remains at 0.

This makes it look like it's a partially-restored file, split
across streams because the final restoredsz for this stream is
less than the file size, and we go to partial_reg(), which
allocates one slot in persp->a.parrest[] for this inode.  But
we've also called partial_reg() with offset/sz of 0/0, which is
less than the file size so this inode never looks "done."

Normally partial_check2() would clear the persp->a.parrest[]
slot in the array when the file is fully restored, but in
this case, that is never satisfied.  So all stream slots
get filled as we encounter more inodes like this, and we
eventually get:

"partial_reg: Out of records. Extend attrs applied early."

Fix this by recognizing that if we hit a LAST header with
no restoredsz set (i.e. the LAST header is the only header),
set restoredsz to EOF (bstatp->bs_size) to indicate that
restoration of this file is complete, skip the call to
partial_reg(), and all is well.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/restore/content.c b/restore/content.c
index 54d933c..8949a7e 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -7516,6 +7516,11 @@ restore_extent_group( drive_t *drivep,
 		 * we are done.
 		 */
 		if ( ehdr.eh_type == EXTENTHDR_TYPE_LAST ) {
+			/* For a wholly sparse file, there is no HOLE
+			 * record; advance restoredsz to EOF.
+			 */
+			if (!restoredsz)
+				restoredsz = bstatp->bs_size;
 			break;
 		}
 

So, ok, fine - that's essentially what your patch did.  ;)  But
now I understand it, and the above to me seems to keep more in line
with the original logic, for better or worse.

What do you think?

-Eric

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

Re: [PATCH] xfsrestore: fix multi stream support

[Rich Johnston]

On 10/02/2013 01:41 PM, Eric Sandeen wrote:
> Ok me again.  ;)  Here's a testcase:
>
> #!/bin/bash
>
> # paths to binaries under test
> DUMP=/mnt/test2/git/xfsdump/dump/xfsdump
> RESTORE=/mnt/test2/git/xfsdump/restore/xfsrestore
>
> # dir we'll create files in & dump
> DUMPDIR=/mnt/test
> # dir where we'll restore
> RESTOREDIR=/mnt/test2/restore
>
> mkdir -p $DUMPDIR/dir
> mkdir -p $RESTOREDIR
> rm -rf $DUMPDIR/dir/*
> rm -rf $RESTOREDIR/*
>
> truncate --size=1t $DUMPDIR/dir/sparsefile1
> truncate --size=1t $DUMPDIR/dir/sparsefile2
> truncate --size=1t $DUMPDIR/dir/sparsefile3
> truncate --size=1t $DUMPDIR/dir/sparsefile4
>
> rm -f stream1 stream2
> $DUMP -L session -M label1 -M label2 -f stream1 -f stream2 $DUMPDIR
> $RESTORE -F -f stream1 -f stream2 $RESTOREDIR
>
> ---
>
> so we go down this path:
>
> restore_extent_group
> 	<loop over extent headers adding up restoredsz>
>          if (bstatp->bs_size > restoredsz) {
>                  partial_reg()
>
> In that loop, if we find DATA or HOLE, it advances "restoredsz" so it
> generally does handle sparse files properly.
>
> But a wholly sparse file has only the LAST header type, and resetoredsz
> never moves.  This is important.  ;)  That's why the condition necessary
> to go to partial_reg() is true.
>
> in partial_reg(), with multiple streams, we check persp->a.parrest[i].is_ino
> for each stream ("i") to see if the inode we're restoring i in any is_ino:
>
>          /* Search for a matching inode.  Gaps can exist so we must search
>           * all entries.
>           */
>          for (i=0; i < partialmax; i++ ) {
>                  if (persp->a.parrest[i].is_ino == ino) {
>                          isptr = &persp->a.parrest[i];
>                          break;
>                  }
>          }
>
> If this is the first time we've hit this inode we won't find it, and so we fill
> it in on one slot:
>
>          /* If not found, find a free one, fill it in and return */
>          if ( ! isptr ) {
>                  /* find a free one */
>                  for (i=0; i < partialmax; i++ ) {
>                          if (persp->a.parrest[i].is_ino == 0) {
>                                  isptr->is_ino = ino;
> 				<snip>
>                                  goto found;
>                          }
>                  }
>                  /* Should never get here. */
>                  pi_unlock();
>                  mlog( MLOG_NORMAL | MLOG_WARNING, _(
>                    "partial_reg: Out of records. Extend attrs applied early.\n"));
> #ifdef DEBUGPARTIALS
> 	}
>
> Otherwise, we go to found:  which calls partial_check2().
>
> So the only way we can not "find a free one" is if every a.parrest[i].is_ino
> is set to something.  Well, we only have a few of them based on the number of
> streams; what clears it?  partial_check2(), which is looking to see if the file
> is wholly restored:
>
>          /* Check if all bytes are accounted for.  */
>          if (curoffset >= fsize) {
>                  isptr->is_ino = 0;  /* clear the entry */
>
> But since the wholly-sparse file had only a LAST record, and no HOLE, nothing
> advanced, and it doesn't look "done" - so partial_check2() fails, we fill all
> the slots, and we hit the dreaded "partial_reg: Out of records."
>
> Bleah.
>
> So I agree, this does seem to only happen with wholly-sparse files.
>
> Adding a HOLE record for them would fix it, but that doesn't fix old dumps.
>
> So I thought about doing something like this:
>
>
> [PATCH] xfsdump: handle large, wholly-sparse files
>
> In restore_extent_group(), we loop over all extent headers for an inode
> in the stream, and add up the cumulatively restored size, accounting
> for both HOLE and DATA records and advancing restoredsz as we go.
>
> But for a wholly-sparse file, we have no HOLE header, only
> a LAST header, and restoredsz remains at 0.
>
> This makes it look like it's a partially-restored file, split
> across streams because the final restoredsz for this stream is
> less than the file size, and we go to partial_reg(), which
> allocates one slot in persp->a.parrest[] for this inode.  But
> we've also called partial_reg() with offset/sz of 0/0, which is
> less than the file size so this inode never looks "done."
>
> Normally partial_check2() would clear the persp->a.parrest[]
> slot in the array when the file is fully restored, but in
> this case, that is never satisfied.  So all stream slots
> get filled as we encounter more inodes like this, and we
> eventually get:
>
> "partial_reg: Out of records. Extend attrs applied early."
>
> Fix this by recognizing that if we hit a LAST header with
> no restoredsz set (i.e. the LAST header is the only header),
> set restoredsz to EOF (bstatp->bs_size) to indicate that
> restoration of this file is complete, skip the call to
> partial_reg(), and all is well.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/restore/content.c b/restore/content.c
> index 54d933c..8949a7e 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -7516,6 +7516,11 @@ restore_extent_group( drive_t *drivep,
>   		 * we are done.
>   		 */
>   		if ( ehdr.eh_type == EXTENTHDR_TYPE_LAST ) {
> +			/* For a wholly sparse file, there is no HOLE
> +			 * record; advance restoredsz to EOF.
> +			 */
> +			if (!restoredsz)
> +				restoredsz = bstatp->bs_size;
>   			break;
>   		}
>
>
> So, ok, fine - that's essentially what your patch did.  ;)  But
> now I understand it, and the above to me seems to keep more in  line
> with the original logic, for better or worse.
>
> What ,do you think?

Sure go for it. That was one of my test programs but obviously I choose 
the wrong one. ;) Its really sixes to me.

I still think the the check in partial_reg is not needed. I never saw a 
case where single stream restore hits that check except when there are 
no extents. Do you have an case/example?

We saw this issue with DMF offline files because DMF removes the extents 
and the file has an attribute which is not restored with the current 
code using multistream.

So I thinks a simple test case is:

Create a file with no extents.
Give that file an attribute
dump and restore it (both single and multistream)
verify the file still has the attribute.

Your thoughts?
--Rich

>
> -Eric
>

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

Re: [PATCH] xfsrestore: fix multi stream support

[Eric Sandeen]

On 10/2/13 3:03 PM, Rich Johnston wrote:
> 
> 
> On 10/02/2013 01:41 PM, Eric Sandeen wrote:

...

>> What ,do you think?
> 
> Sure go for it. That was one of my test programs but obviously I
> choose the wrong one. ;) Its really sixes to me.
> 
> I still think the the check in partial_reg is not needed. I never saw
> a case where single stream restore hits that check except when there
> are no extents. Do you have an case/example?

I don't.  It seems like reasonable defensive programming, though,
so I'm not anxious to remove it, given that nobody really groks
this code too well.  Could turn it into a warning, maybe, so
it fires if we do ever get there.

Can you look back in ptools & see when/why it was added?

> We saw this issue with DMF offline files because DMF removes the
> extents and the file has an attribute which is not restored with the
> current code using multistream.

Ah, I think there's something about not restoring attributes until
all of the file has been restored.  So again in this case, the file
never looks "restored" and it never gets to the attribute restoration?

Oh right, like the comment says:

/* partial_reg - Registers files that are only partially restored by
 * a dump stream into the persistent state.
 *
 * This is done because DMAPI extended attributes must not be set until
 * the entire file has been restored in order to co-ordinate with the 
 * Data Migration Facility (DMF) daemons.  Since extended attributes are
 * recorded with each extent group in the dump, this registry is used to
 * make sure only the final dump stream applies the extended attributes.
 *
 * Likewise, certain extended inode flags (e.g. XFS_XFLAG_IMMUTABLE)
 * should only be set after all data for a file has been restored.
 */

> So I thinks a simple test case is:
> 
> Create a file with no extents. Give that file an attribute dump and
> restore it (both single and multistream) verify the file still has
> the attribute.

An extended attribute you mean?

> Your thoughts?

Yeah, go for it w/ a testcase. :)

I could see where even if we didn't get the dreaded "Out of records"
message, it might still skip the attribute restore if the file
never looks "done?"

-Eric

 --Rich
> 
>> 
>> -Eric
>> 
> 

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

Re: [PATCH] xfsrestore: fix multi stream support

[Rich Johnston]

On 10/02/2013 03:13 PM, Eric Sandeen wrote:
> On 10/2/13 3:03 PM, Rich Johnston wrote:
>>
>>
>> On 10/02/2013 01:41 PM, Eric Sandeen wrote:
>
> ...
>
>>> What ,do you think?
>>
>> Sure go for it. That was one of my test programs but obviously I
>> choose the wrong one. ;) Its really sixes to me.
>>
>> I still think the the check in partial_reg is not needed. I never saw
>> a case where single stream restore hits that check except when there
>> are no extents. Do you have an case/example?
>
> I don't.  It seems like reasonable defensive programming, though,
> so I'm not anxious to remove it, given that nobody really groks
> this code too well.  Could turn it into a warning, maybe, so
> it fires if we do ever get there.
>
> Can you look back in ptools & see when/why it was added?

Looks like it was added Jun 11 1998

xfsrestore can corrupt DMAPI files split across media.

Description
   When a file dumped by xfsdump spans media, the xfs extended
   attributes are saved at the end of each section. xfsrestore
   restores the xfs extended attributes each time that is sees
   them.  This causes a DMAPI file (e.g., a file saved by the
   DMF system) to appear completely restored when in fact there
   are more pieces to restore.

This looks like it can be removed safely becaus this was added for 
multistream support (dumped by xfsdump spans media).
Do you agree?

>
>> We saw this issue with DMF offline files because DMF removes the
>> extents and the file has an attribute which is not restored with the
>> current code using multistream.
>
> Ah, I think there's something about not restoring attributes until
> all of the file has been restored.  So again in this case, the file
> never looks "restored" and it never gets to the attribute restoration?
>
> Oh right, like the comment says:
>
> /* partial_reg - Registers files that are only partially restored by
>   * a dump stream into the persistent state.
>   *
>   * This is done because DMAPI extended attributes must not be set until
>   * the entire file has been restored in order to co-ordinate with the
>   * Data Migration Facility (DMF) daemons.  Since extended attributes are
>   * recorded with each extent group in the dump, this registry is used to
>   * make sure only the final dump stream applies the extended attributes.
>   *
>   * Likewise, certain extended inode flags (e.g. XFS_XFLAG_IMMUTABLE)
>   * should only be set after all data for a file has been restored.
>   */
>
>> So I thinks a simple test case is:
>>
>> Create a file with no extents. Give that file an attribute dump and
>> restore it (both single and multistream) verify the file still has
>> the attribute.
>
> An extended attribute you mean?
Yes
>
>> Your thoughts?
>
> Yeah, go for it w/ a testcase. :)

Looks like multistream is not tested at all.

I would more or less modify 296 to test multistream and include more 
types of files to test (files with extents, with attributes, with both etc).

I will resubmit with your changes (giving credit where credit is do)

>
> I could see where even if we didn't get the dreaded "Out of records"
> message, it might still skip the attribute restore if the file
> never looks "done?"
Yes but only when the data is split (multi stream).


>
> -Eric
>
>   --Rich
>>
>>>
>>> -Eric
>>>
>>
>

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

Re: [PATCH] xfsdump: handle large, wholly-sparse files

[Eric Sandeen]

On 10/3/13 4:20 PM, rjohnston@sgi.com wrote:
> [PATCH] xfsdump: handle large, wholly-sparse files
>  
> In restore_extent_group(), we loop over all extent headers for an inode
> in the stream, and add up the cumulatively restored size, accounting
> for both HOLE and DATA records and advancing restoredsz as we go.
> 
> But for a wholly-sparse file, we have no HOLE header, only
> a LAST header, and restoredsz remains at 0.
> 
> This makes it look like it's a partially-restored file, split
> across streams because the final restoredsz for this stream is
> less than the file size, and we go to partial_reg(), which
> allocates one slot in persp->a.parrest[] for this inode.  But
> we've also called partial_reg() with offset/sz of 0/0, which is
> less than the file size so this inode never looks "done."
>  
> Normally partial_check2() would clear the persp->a.parrest[]
> slot in the array when the file is fully restored, but in
> this case, that is never satisfied.  So all stream slots
> get filled as we encounter more inodes like this, and we
> eventually get:
> 
> "partial_reg: Out of records. Extend attrs applied early."
> 
> Fix this by recognizing that if we hit a LAST header with
> no restoredsz set (i.e. the LAST header is the only header),
> set restoredsz to EOF (bstatp->bs_size) to indicate that
> restoration of this file is complete, skip the call to
> partial_reg(), and all is well.
> 
> [rjohnston: partial_reg() was added Jun 11 1998 to fix a multi-stream
>   bug. With Eric's patch above, the multi-stream check in partial_reg 
>   can be removed because single stream restores will never be partially
>   restored.]

No, please don't add this to my patch; that part was still under discussion
AFAIK.

The [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h]

type stuff is for minor changes:

"If you are a subsystem or branch maintainer, sometimes you need to slightly
modify patches you receive in order to merge them, because the code is not
exactly the same in your tree and the submitters'. If you stick strictly to
rule (c), you should ask the submitter to rediff, but this is a totally
counter-productive waste of time and energy. Rule (b) allows you to adjust
the code, but then it is very impolite to change one submitter's code and
make him endorse your bugs. To solve this problem, it is recommended that
you add a line between the last Signed-off-by header and yours, indicating
the nature of your changes."

Aside from that, Signed-off-by: from a maintainer means:

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

Removing that non-multistream test is (possibly) a cleanup pretty much
unrelated to the bug at hand, and if you _really_ want to make the change,
just submit a patch after the bugfix patch, and we can evaluate it on its
merits.

IOWS: I understand the bugfix part of the patch.  Whether it's safe to
remove the early return from partial_reg() has never actually been reviewed;
you wrote it, I at a minimum still had questions about it, and I think that's
where it stands.

I know it got complicated when I rewrote things, and so we had 2 patches out
there, but it only gets more complicated if you mush them together & add both
our signed-off-bys to the result.  :)

Thanks,
-Eric


> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
> 
> ---
> Change History
> 
> Original patch  "[PATCH] xfsrestore: fix multi stream support"
> - Patch rename, fixes to code and commit messages by sandeen.
> - Additional changes by rjohnston
> 
> ---
>  restore/content.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> Index: b/restore/content.c
> ===================================================================
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -7516,6 +7516,11 @@ restore_extent_group( drive_t *drivep,
>  		 * we are done.
>  		 */
>  		if ( ehdr.eh_type == EXTENTHDR_TYPE_LAST ) {
> +			/* For a wholly sparse file, there is no HOLE
> +			 * record; advance restoredsz to EOF.
> +			 */
> +			if (!restoredsz)
> +				restoredsz = bstatp->bs_size;
>  			break;
>  		}
>  
> @@ -8959,9 +8964,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

[PATCH V2] xfsdump: handle large, wholly-sparse files

[Rich Johnston]

In restore_extent_group(), we loop over all extent headers for an inode
in the stream, and add up the cumulatively restored size, accounting
for both HOLE and DATA records and advancing restoredsz as we go.

But for a wholly-sparse file, we have no HOLE header, only
a LAST header, and restoredsz remains at 0.

This makes it look like it's a partially-restored file, split
across streams because the final restoredsz for this stream is
less than the file size, and we go to partial_reg(), which
allocates one slot in persp->a.parrest[] for this inode.  But
we've also called partial_reg() with offset/sz of 0/0, which is
less than the file size so this inode never looks "done."
  Normally partial_check2() would clear the persp->a.parrest[]
slot in the array when the file is fully restored, but in
this case, that is never satisfied.  So all stream slots
get filled as we encounter more inodes like this, and we
eventually get:

"partial_reg: Out of records. Extend attrs applied early."

Fix this by recognizing that if we hit a LAST header with
no restoredsz set (i.e. the LAST header is the only header),
set restoredsz to EOF (bstatp->bs_size) to indicate that
restoration of this file is complete, skip the call to
partial_reg(), and all is well.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Acked-by: Rich Johnston <rjohnston@sgi.com>

---
Test will be submitted later.

Change History

Original patch  "[PATCH] xfsrestore: fix multi stream support"
- Patch rename, fixes to code and commit messages by sandeen.
- Additional changes by rjohnston
V2
- Remove changes by rjohnston.

---
  restore/content.c |    5 +++++
  1 file changed, 5 insertions(+), 0 deletions(-)

Index: b/restore/content.c
===================================================================
--- a/restore/content.c
+++ b/restore/content.c
@@ -7516,6 +7516,11 @@ restore_extent_group( drive_t *drivep,
  		 * we are done.
  		 */
  		if ( ehdr.eh_type == EXTENTHDR_TYPE_LAST ) {
+			/* For a wholly sparse file, there is no HOLE
+			 * record; advance restoredsz to EOF.
+			 */
+			if (!restoredsz)
+				restoredsz = bstatp->bs_size;
  			break;
  		}

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

Re: [PATCH V2] xfsdump: handle large, wholly-sparse files

[Eric Sandeen]

Thanks Rich.  I think we need a 3rd party reviewer now.  ;)

> On Oct 3, 2013, at 6:11 PM, Rich Johnston <rjohnston@sgi.com> wrote:
> 
> In restore_extent_group(), we loop over all extent headers for an inode
> in the stream, and add up the cumulatively restored size, accounting
> for both HOLE and DATA records and advancing restoredsz as we go.
> 
> But for a wholly-sparse file, we have no HOLE header, only
> a LAST header, and restoredsz remains at 0.
> 
> This makes it look like it's a partially-restored file, split
> across streams because the final restoredsz for this stream is
> less than the file size, and we go to partial_reg(), which
> allocates one slot in persp->a.parrest[] for this inode.  But
> we've also called partial_reg() with offset/sz of 0/0, which is
> less than the file size so this inode never looks "done."
> Normally partial_check2() would clear the persp->a.parrest[]
> slot in the array when the file is fully restored, but in
> this case, that is never satisfied.  So all stream slots
> get filled as we encounter more inodes like this, and we
> eventually get:
> 
> "partial_reg: Out of records. Extend attrs applied early."
> 
> Fix this by recognizing that if we hit a LAST header with
> no restoredsz set (i.e. the LAST header is the only header),
> set restoredsz to EOF (bstatp->bs_size) to indicate that
> restoration of this file is complete, skip the call to
> partial_reg(), and all is well.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Acked-by: Rich Johnston <rjohnston@sgi.com>
> 
> ---
> Test will be submitted later.
> 
> Change History
> 
> Original patch  "[PATCH] xfsrestore: fix multi stream support"
> - Patch rename, fixes to code and commit messages by sandeen.
> - Additional changes by rjohnston
> V2
> - Remove changes by rjohnston.
> 
> ---
> restore/content.c |    5 +++++
> 1 file changed, 5 insertions(+), 0 deletions(-)
> 
> Index: b/restore/content.c
> ===================================================================
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -7516,6 +7516,11 @@ restore_extent_group( drive_t *drivep,
>         * we are done.
>         */
>        if ( ehdr.eh_type == EXTENTHDR_TYPE_LAST ) {
> +            /* For a wholly sparse file, there is no HOLE
> +             * record; advance restoredsz to EOF.
> +             */
> +            if (!restoredsz)
> +                restoredsz = bstatp->bs_size;
>            break;
>        }
> 

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

[PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[rjohnston]

Verify extended attributes are not lost after multi-stream
xfsdump/xfsrestore of wholly-sparse files.  xfsrestore did not
recognize that if the LAST header was reached with no restoredsz set,
(i.e the LAST header is the only header), the following warning is
displayed:

  "partial_reg: Out of records. Extend attrs applied early."

and the extended attributes on the current and following restored
files are lost.

Signed-off-by: Rich Johnston <rjohnston@sgi.com>

---
 tests/xfs/350     |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/350.out |    2 
 tests/xfs/group   |    1 
 3 files changed, 137 insertions(+)

Index: b/tests/xfs/350
===================================================================
--- /dev/null
+++ b/tests/xfs/350
@@ -0,0 +1,134 @@
+#! /bin/bash
+# FS QA Test No. 350
+#
+# Verify extended attributes are not lost after multi-stream
+# xfsdump/xfsrestore of wholly-sparse files.
+
+#-----------------------------------------------------------------------
+# Copyright (c) 2013 SGI.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=0	# success is the default!
+trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/dump
+
+# real QA test starts here
+_supported_fs xfs
+_supported_os Linux
+
+_require_multi_stream
+
+# dir where we'll restore
+RESTOREDIR=$SCRATCH_MNT/restore
+
+# subdir used for dump/restore
+DUMPDIR=dumpsrc
+
+# test extended attributes
+attr_name=testattr
+attr_value=1234
+
+# setup for test
+rm -rf $seqres.full
+mkdir -p $RESTOREDIR
+mkdir -p $SCRATCH_MNT/$DUMPDIR
+
+# cleanup for next dump/restore.
+_clean_dirs()
+{
+	rm -rf $SCRATCH_MNT/$DUMPDIR/*
+	rm -rf $RESTOREDIR/*
+	rm -f $tmp.stream?
+}
+
+# set the extended attributes on the test files.
+_set_attrs()
+{
+	for fname in $(ls -dD $SCRATCH_MNT/$DUMPDIR/*); do
+		attr -Rs $attr_name -V $attr_value $fname \
+		2>&1 >> $seqres.full || _fail "could not set ATTR for $fname"
+	done
+}
+
+# perform a dump and restore.
+_do_dump_restore()
+{
+	$XFSDUMP_PROG -L session -M label1 -M label2 -f $tmp.stream1 \
+		-f $tmp.stream2 $SCRATCH_MNT -s $DUMPDIR \
+		2>&1 >> $seqres.full || _fail "dump failed"
+	$XFSRESTORE_PROG -F -f $tmp.stream1 -f $tmp.stream2 $RESTOREDIR \
+		2>&1 >> $seqres.full || _fail "restore failed"
+}
+
+# verify the restored files extended attributes and
+# echo the error (if any) so the test will continue
+_verify_attrs()
+{
+	for fname in $(ls -dD $RESTOREDIR/$DUMPDIR/*); do
+		attr -Rg $attr_name $fname 2>&1 | tee -a $seqres.full | \
+			grep $attr_value 2>&1 >> $seqres.full || \
+			echo "ATTR for $fname  DOES NOT match"
+	done
+}
+
+# create files for test 1, a large file so the sparse file
+# is in the next stream.
+_create_test1_files()
+{
+
+	dd if=/dev/zero of=$SCRATCH_MNT/$DUMPDIR/10MB bs=1MB \
+		count=10 2>&1 >> $seqres.full | _filter_dd
+	truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse0 2>&1 \
+		>> $seqres.full
+}
+
+# create 4 sparse files for test 2
+_create_test2_files()
+{
+	for i in `seq 1 4`; do
+		truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse$i 2>&1 >> \
+		$seqres.full || _fail "failed to create sparse \"$i\""
+	done
+}
+echo "Silence is golden."
+echo "Starting Test 1" >> $seqres.full
+_clean_dirs
+_create_test1_files
+_set_attrs
+_do_dump_restore
+_verify_attrs
+
+echo "Starting Test 2" >> $seqres.full
+_clean_dirs
+_create_test2_files
+_set_attrs
+_do_dump_restore
+_verify_attrs
+
+# success, all done
+exit
Index: b/tests/xfs/350.out
===================================================================
--- /dev/null
+++ b/tests/xfs/350.out
@@ -0,0 +1,2 @@
+QA output created by 350
+Silence is golden.
Index: b/tests/xfs/group
===================================================================
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -179,3 +179,4 @@
 297 auto freeze
 298 auto attr symlink quick
 299 auto quota
+350 dump auto


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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Eric Sandeen]

On 10/7/13 2:38 PM, rjohnston@sgi.com wrote:
> Verify extended attributes are not lost after multi-stream
> xfsdump/xfsrestore of wholly-sparse files.  xfsrestore did not
> recognize that if the LAST header was reached with no restoredsz set,
> (i.e the LAST header is the only header), the following warning is
> displayed:
> 
>   "partial_reg: Out of records. Extend attrs applied early."
> 
> and the extended attributes on the current and following restored
> files are lost.

and restore segfaults too, IIRC.  ;)

So I'm trying to understand - are attrs not applied because xfs_restore
terminates, or is everything fine other than the attrs missing when
it completes successfully?

iows, I get this when it fails:

 QA output created by 350
 Silence is golden.
+ATTR for /mnt/scratch/restore/dumpsrc/sparse0  DOES NOT match
+./tests/xfs/350: line 80: 18231 Segmentation fault      (core dumped) $XFSRESTORE_PROG -F -f $tmp.stream1 -f $tmp.stream2 $RESTOREDIR 2>&1 >> $seqres.full
+restore failed
+(see /mnt/test2/git/xfstests/results//xfs/350.full for details)

and never get to the point of seeing if attrs are missing.

Anyway, a few other things below for the record...

> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
> 
> ---
>  tests/xfs/350     |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/350.out |    2 
>  tests/xfs/group   |    1 
>  3 files changed, 137 insertions(+)
> 
> Index: b/tests/xfs/350
> ===================================================================
> --- /dev/null
> +++ b/tests/xfs/350
> @@ -0,0 +1,134 @@
> +#! /bin/bash
> +# FS QA Test No. 350

Big jump!

> +#
> +# Verify extended attributes are not lost after multi-stream
> +# xfsdump/xfsrestore of wholly-sparse files.
> +
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2013 SGI.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=0	# success is the default!
> +trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dump
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_supported_os Linux
> +
> +_require_multi_stream

_require_scratch
_scratch_mkfs
_scratch_mount

> +
> +# dir where we'll restore
> +RESTOREDIR=$SCRATCH_MNT/restore

so you need _require_scratch & _scratch_mkfs before this...

> +
> +# subdir used for dump/restore
> +DUMPDIR=dumpsrc
> +
> +# test extended attributes
> +attr_name=testattr
> +attr_value=1234
> +
> +# setup for test
> +rm -rf $seqres.full

no need to recursively remove a file, but *shrug* :)

> +mkdir -p $RESTOREDIR
> +mkdir -p $SCRATCH_MNT/$DUMPDIR
> +
> +# cleanup for next dump/restore.
> +_clean_dirs()
> +{
> +	rm -rf $SCRATCH_MNT/$DUMPDIR/*
> +	rm -rf $RESTOREDIR/*
> +	rm -f $tmp.stream?
> +}
> +
> +# set the extended attributes on the test files.
> +_set_attrs()
> +{
> +	for fname in $(ls -dD $SCRATCH_MNT/$DUMPDIR/*); do
> +		attr -Rs $attr_name -V $attr_value $fname \
> +		2>&1 >> $seqres.full || _fail "could not set ATTR for $fname"
> +	done

Just out of curiosity, is the root (-R) namespace relevant to the problem?

> +}
> +
> +# perform a dump and restore.
> +_do_dump_restore()
> +{
> +	$XFSDUMP_PROG -L session -M label1 -M label2 -f $tmp.stream1 \
> +		-f $tmp.stream2 $SCRATCH_MNT -s $DUMPDIR \
> +		2>&1 >> $seqres.full || _fail "dump failed"
> +	$XFSRESTORE_PROG -F -f $tmp.stream1 -f $tmp.stream2 $RESTOREDIR \
> +		2>&1 >> $seqres.full || _fail "restore failed"
> +}
> +
> +# verify the restored files extended attributes and
> +# echo the error (if any) so the test will continue
> +_verify_attrs()
> +{
> +	for fname in $(ls -dD $RESTOREDIR/$DUMPDIR/*); do
> +		attr -Rg $attr_name $fname 2>&1 | tee -a $seqres.full | \
> +			grep $attr_value 2>&1 >> $seqres.full || \
> +			echo "ATTR for $fname  DOES NOT match"
> +	done
> +}
> +
> +# create files for test 1, a large file so the sparse file
> +# is in the next stream.
> +_create_test1_files()
> +{
> +
> +	dd if=/dev/zero of=$SCRATCH_MNT/$DUMPDIR/10MB bs=1MB \
> +		count=10 2>&1 >> $seqres.full | _filter_dd
> +	truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse0 2>&1 \
> +		>> $seqres.full
> +}
> +
> +# create 4 sparse files for test 2
> +_create_test2_files()
> +{
> +	for i in `seq 1 4`; do
> +		truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse$i 2>&1 >> \
> +		$seqres.full || _fail "failed to create sparse \"$i\""
> +	done
> +}
> +echo "Silence is golden."
> +echo "Starting Test 1" >> $seqres.full
> +_clean_dirs
> +_create_test1_files
> +_set_attrs
> +_do_dump_restore
> +_verify_attrs
> +
> +echo "Starting Test 2" >> $seqres.full
> +_clean_dirs
> +_create_test2_files
> +_set_attrs
> +_do_dump_restore
> +_verify_attrs
> +
> +# success, all done
> +exit
> Index: b/tests/xfs/350.out
> ===================================================================
> --- /dev/null
> +++ b/tests/xfs/350.out
> @@ -0,0 +1,2 @@
> +QA output created by 350
> +Silence is golden.
> Index: b/tests/xfs/group
> ===================================================================
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -179,3 +179,4 @@
>  297 auto freeze
>  298 auto attr symlink quick
>  299 auto quota
> +350 dump auto
> 
> 
> _______________________________________________
> 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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Rich Johnston]

On 10/07/2013 03:32 PM, Eric Sandeen wrote:
> On 10/7/13 2:38 PM, rjohnston@sgi.com wrote:
>> Verify extended attributes are not lost after multi-stream
>> xfsdump/xfsrestore of wholly-sparse files.  xfsrestore did not
>> recognize that if the LAST header was reached with no restoredsz set,
>> (i.e the LAST header is the only header), the following warning is
>> displayed:
>>
>>    "partial_reg: Out of records. Extend attrs applied early."
>>
>> and the extended attributes on the current and following restored
>> files are lost.
>
> and restore segfaults too, IIRC.  ;)

For test 2 yes you are correct, does not segfault for test 1.

>
> So I'm trying to understand - are attrs not applied because xfs_restore
> terminates, or is everything fine other than the attrs missing when
> it completes successfully?

Everything fine other than the attrs missing when it completes 
successfully, which is how this bug was originally reported to me.
In DMF land this meant OFFLINE files were restored as NON-MIGRATABLE 
(iow Extended attributes removed )

>
> iows, I get this when it fails:
>
>   QA output created by 350
>   Silence is golden.
> +ATTR for /mnt/scratch/restore/dumpsrc/sparse0  DOES NOT match

This is from the first test, and I purposely just echo the error so I 
hit the second case too.
>
> and never get to the point of seeing if attrs are missing.

Forgot I changed the echo "restore failed" to _fail "restore failed".
That's why you don't see the attrs are missing.

>
> Anyway, a few other things below for the record...
>
>> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
>>
>> ---
>>   tests/xfs/350     |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/xfs/350.out |    2
>>   tests/xfs/group   |    1
>>   3 files changed, 137 insertions(+)
>>
>> Index: b/tests/xfs/350
>> ===================================================================
>> --- /dev/null
>> +++ b/tests/xfs/350
>> @@ -0,0 +1,134 @@
>> +#! /bin/bash
>> +# FS QA Test No. 350
>
> Big jump!

Taken care of at commit time. ;) Big jump as to not interfere with other 
peoples development.

>
>> +#
>> +# Verify extended attributes are not lost after multi-stream
>> +# xfsdump/xfsrestore of wholly-sparse files.
>> +
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2013 SGI.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=0	# success is the default!
>> +trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/dump
>> +
>> +# real QA test starts here
>> +_supported_fs xfs
>> +_supported_os Linux
>> +
>> +_require_multi_stream
>

Dooh good catch.

> _require_scratch
> _scratch_mkfs
> _scratch_mount
>
>> +
>> +# dir where we'll restore
>> +RESTOREDIR=$SCRATCH_MNT/restore
>
> so you need _require_scratch & _scratch_mkfs before this...

Gottcha

>
>> +
>> +# subdir used for dump/restore
>> +DUMPDIR=dumpsrc
>> +
>> +# test extended attributes
>> +attr_name=testattr
>> +attr_value=1234
>> +
>> +# setup for test
>> +rm -rf $seqres.full
>
> no need to recursively remove a file, but *shrug* :)

Yup typo.

>
>> +mkdir -p $RESTOREDIR
>> +mkdir -p $SCRATCH_MNT/$DUMPDIR
>> +
>> +# cleanup for next dump/restore.
>> +_clean_dirs()
>> +{
>> +	rm -rf $SCRATCH_MNT/$DUMPDIR/*
>> +	rm -rf $RESTOREDIR/*
>> +	rm -f $tmp.stream?
>> +}
>> +
>> +# set the extended attributes on the test files.
>> +_set_attrs()
>> +{
>> +	for fname in $(ls -dD $SCRATCH_MNT/$DUMPDIR/*); do
>> +		attr -Rs $attr_name -V $attr_value $fname \
>> +		2>&1 >> $seqres.full || _fail "could not set ATTR for $fname"
>> +	done
>
> Just out of curiosity, is the root (-R) namespace relevant to the problem?

Don't think so I can remove th -R.>

>> +}
>> +
>> +# perform a dump and restore.
>> +_do_dump_restore()
>> +{
>> +	$XFSDUMP_PROG -L session -M label1 -M label2 -f $tmp.stream1 \
>> +		-f $tmp.stream2 $SCRATCH_MNT -s $DUMPDIR \
>> +		2>&1 >> $seqres.full || _fail "dump failed"
>> +	$XFSRESTORE_PROG -F -f $tmp.stream1 -f $tmp.stream2 $RESTOREDIR \
>> +		2>&1 >> $seqres.full || _fail "restore failed"
>> +}
>> +
>> +# verify the restored files extended attributes and
>> +# echo the error (if any) so the test will continue
>> +_verify_attrs()
>> +{
>> +	for fname in $(ls -dD $RESTOREDIR/$DUMPDIR/*); do
>> +		attr -Rg $attr_name $fname 2>&1 | tee -a $seqres.full | \
>> +			grep $attr_value 2>&1 >> $seqres.full || \
>> +			echo "ATTR for $fname  DOES NOT match"
>> +	done
>> +}
>> +
>> +# create files for test 1, a large file so the sparse file
>> +# is in the next stream.
>> +_create_test1_files()
>> +{
>> +
>> +	dd if=/dev/zero of=$SCRATCH_MNT/$DUMPDIR/10MB bs=1MB \
>> +		count=10 2>&1 >> $seqres.full | _filter_dd
>> +	truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse0 2>&1 \
>> +		>> $seqres.full
>> +}
>> +
>> +# create 4 sparse files for test 2
>> +_create_test2_files()
>> +{
>> +	for i in `seq 1 4`; do
>> +		truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse$i 2>&1 >> \
>> +		$seqres.full || _fail "failed to create sparse \"$i\""
>> +	done
>> +}
>> +echo "Silence is golden."
>> +echo "Starting Test 1" >> $seqres.full
>> +_clean_dirs
>> +_create_test1_files
>> +_set_attrs
>> +_do_dump_restore
>> +_verify_attrs
>> +
>> +echo "Starting Test 2" >> $seqres.full
>> +_clean_dirs
>> +_create_test2_files
>> +_set_attrs
>> +_do_dump_restore
>> +_verify_attrs
>> +
>> +# success, all done
>> +exit
>> Index: b/tests/xfs/350.out
>> ===================================================================
>> --- /dev/null
>> +++ b/tests/xfs/350.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 350
>> +Silence is golden.
>> Index: b/tests/xfs/group
>> ===================================================================
>> --- a/tests/xfs/group
>> +++ b/tests/xfs/group
>> @@ -179,3 +179,4 @@
>>   297 auto freeze
>>   298 auto attr symlink quick
>>   299 auto quota
>> +350 dump auto
>>
>>
>> _______________________________________________
>> 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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Eric Sandeen]

On 10/7/13 3:54 PM, Rich Johnston wrote:
> 
> 
> On 10/07/2013 03:32 PM, Eric Sandeen wrote:
>> On 10/7/13 2:38 PM, rjohnston@sgi.com wrote:
>>> Verify extended attributes are not lost after multi-stream
>>> xfsdump/xfsrestore of wholly-sparse files.  xfsrestore did not
>>> recognize that if the LAST header was reached with no restoredsz set,
>>> (i.e the LAST header is the only header), the following warning is
>>> displayed:
>>>
>>>    "partial_reg: Out of records. Extend attrs applied early."
>>>
>>> and the extended attributes on the current and following restored
>>> files are lost.
>>
>> and restore segfaults too, IIRC.  ;)
> 
> For test 2 yes you are correct, does not segfault for test 1.


Ah, I had missed that.  Makes sense now.

>>
>> So I'm trying to understand - are attrs not applied because xfs_restore
>> terminates, or is everything fine other than the attrs missing when
>> it completes successfully?
> 
> Everything fine other than the attrs missing when it completes successfully, which is how this bug was originally reported to me.
> In DMF land this meant OFFLINE files were restored as NON-MIGRATABLE (iow Extended attributes removed )
> 
>>
>> iows, I get this when it fails:
>>
>>   QA output created by 350
>>   Silence is golden.
>> +ATTR for /mnt/scratch/restore/dumpsrc/sparse0  DOES NOT match
> 
> This is from the first test, and I purposely just echo the error so I hit the second case too.
>>
>> and never get to the point of seeing if attrs are missing.
> 
> Forgot I changed the echo "restore failed" to _fail "restore failed".
> That's why you don't see the attrs are missing.

ok.

>>
>> Anyway, a few other things below for the record...
>>
>>> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
>>>
>>> ---
>>>   tests/xfs/350     |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/xfs/350.out |    2
>>>   tests/xfs/group   |    1
>>>   3 files changed, 137 insertions(+)
>>>
>>> Index: b/tests/xfs/350
>>> ===================================================================
>>> --- /dev/null
>>> +++ b/tests/xfs/350
>>> @@ -0,0 +1,134 @@
>>> +#! /bin/bash
>>> +# FS QA Test No. 350
>>
>> Big jump!
> 
> Taken care of at commit time. ;) Big jump as to not interfere with other peoples development.

fine by me!

...

>> Just out of curiosity, is the root (-R) namespace relevant to the problem?
> 
> Don't think so I can remove th -R.

doesn't really matter to me except then you'd need _require_root if you want
to use -R.

-Eric

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Dave Chinner]

On Mon, Oct 07, 2013 at 02:38:35PM -0500, rjohnston@sgi.com wrote:
> Verify extended attributes are not lost after multi-stream
> xfsdump/xfsrestore of wholly-sparse files.  xfsrestore did not
> recognize that if the LAST header was reached with no restoredsz set,
> (i.e the LAST header is the only header), the following warning is
> displayed:
> 
>   "partial_reg: Out of records. Extend attrs applied early."
> 
> and the extended attributes on the current and following restored
> files are lost.
> 
> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
> 
> ---
>  tests/xfs/350     |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/350.out |    2 
>  tests/xfs/group   |    1 
>  3 files changed, 137 insertions(+)
> 
> Index: b/tests/xfs/350
> ===================================================================
> --- /dev/null
> +++ b/tests/xfs/350
....
> +here=`pwd`
> +tmp=/tmp/$$
> +status=0	# success is the default!

status=1

Test failure should always be the default.


> +}
> +echo "Silence is golden."
> +echo "Starting Test 1" >> $seqres.full
> +_clean_dirs
> +_create_test1_files
> +_set_attrs
> +_do_dump_restore
> +_verify_attrs
> +
> +echo "Starting Test 2" >> $seqres.full
> +_clean_dirs
> +_create_test2_files
> +_set_attrs
> +_do_dump_restore
> +_verify_attrs

Two tests, please. move all the common parts into common/dump, and
write them as two separate tests. That way we can easily track what
test is failing just by looking at what harness test is failing...

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Eric Sandeen]

On 10/7/13 7:53 PM, Dave Chinner wrote:
> Two tests, please. move all the common parts into common/dump, and
> write them as two separate tests. That way we can easily track what
> test is failing just by looking at what harness test is failing...

I'm not quite convinced that it's 2 separate tests, TBH.

It's the same root cause; I guess there is a slightly different
outcome because if you hit the same root cause enough times,
you'll segfault.  That's the only difference in test #2.
(and the segfault isn't fixed AFAIK).

-Eric

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Eric Sandeen]

On 10/7/13 7:57 PM, Eric Sandeen wrote:
> On 10/7/13 7:53 PM, Dave Chinner wrote:
>> Two tests, please. move all the common parts into common/dump, and
>> write them as two separate tests. That way we can easily track what
>> test is failing just by looking at what harness test is failing...
> 
> I'm not quite convinced that it's 2 separate tests, TBH.
> 
> It's the same root cause; I guess there is a slightly different
> outcome because if you hit the same root cause enough times,
> you'll segfault.  That's the only difference in test #2.
> (and the segfault isn't fixed AFAIK).

Or to be clearer - I'm not sure that test1 is testing anything
fundamentally different from test2.

-Eric

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Rich Johnston]

On 10/07/2013 07:57 PM, Eric Sandeen wrote:
> On 10/7/13 7:53 PM, Dave Chinner wrote:
>> Two tests, please. move all the common parts into common/dump, and
>> write them as two separate tests. That way we can easily track what
>> test is failing just by looking at what harness test is failing...
>
> I'm not quite convinced that it's 2 separate tests, TBH.
>
> It's the same root cause; I guess there is a slightly different
> outcome because if you hit the same root cause enough times,
> you'll segfault.


Multiple DMF offline files are successfully restored but the attrs are 
lost. I wanted to show/test that case.

I agree with Eric that it is the same  root cause but because can occur 
with successful dumps and does not segfault,  Thats why the 2 tests.

> That's the only difference in test #2.
> (and the segfault isn't fixed AFAIK).

Agreed and I will note that in the test description.

>
> -Eric
>

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Dave Chinner]

On Tue, Oct 08, 2013 at 09:21:13AM -0500, Rich Johnston wrote:
> On 10/07/2013 07:57 PM, Eric Sandeen wrote:
> >On 10/7/13 7:53 PM, Dave Chinner wrote:
> >>Two tests, please. move all the common parts into common/dump, and
> >>write them as two separate tests. That way we can easily track what
> >>test is failing just by looking at what harness test is failing...
> >
> >I'm not quite convinced that it's 2 separate tests, TBH.
> >
> >It's the same root cause; I guess there is a slightly different
> >outcome because if you hit the same root cause enough times,
> >you'll segfault.
> 
> 
> Multiple DMF offline files are successfully restored but the attrs
> are lost. I wanted to show/test that case.
> 
> I agree with Eric that it is the same  root cause but because can
> occur with successful dumps and does not segfault,  Thats why the 2
> tests.

Ok, the problem might be triggering the same root cause, but in the
case of unit tests that is usually irrelevant. That is each individual test
should be independently tracked by the test harness regardless of
the bug it triggers.

And reading on #xfs, the problem isn't clearly understood yet as
both you and Eric are not sure exactly why there are differences in
behaviour between different tests yet. e.g:

[09/10/13 02:13] <rjohnston1> Ahh OK but my DMF test case had several wholly-sparse (offline files) and the dump succeeded. 
[09/10/13 02:18] <sandeen> tbh there is one thing I'm not clear on here, why a 1t sparse file behaves differently from a 1k sparse file
[09/10/13 02:18] <sandeen> that seems . .  wrong
[09/10/13 02:19] <sandeen> but I guess it must just key on i_size, not blocks
[09/10/13 02:19] <sandeen> so anyway, maybe your dmf testcase had smaller file sizes?
[09/10/13 02:19] <sandeen> sorry, I have to run & get missed homework to my kid @ school, bbiab.  Grr.
[09/10/13 02:20] <rjohnston1> NP, yes they were smaller.
[09/10/13 02:22] <rjohnston1> 100 10MB files no segfault, just trashed attrs.

SO, there's different behaviour dependent on file sizes, and that's
not understood yet. IOWs, there's yet another test case that needs
to be exercised here to demonstrate the different failure cases
that are being seen.

And that brings it further into the realm of multiple tests, in
which case we might have:

	- 320 - multistream with wholly sparse files
	- 321 - multistream with small sparse files
	- 322 - multistream with large sparse files

This is the point I'm trying to make - from a test harness
perspective, we don't really care what the bugs are that are being
triggered by the tests - what we are trying to do is get coverage of
different behaviours and test cases and track which ones fail. What
i see from the above woul dbe:

	- 320 = pass
	- 321 = fail, corrupt attrs
	- 322 = fail, SEGV

Different tests, different failure modes, easy to tell them apart.

> >That's the only difference in test #2.
> >(and the segfault isn't fixed AFAIK).

Exactly my point. With the fix you have made, we'll get:

	- 320 = pass
	- 321 = pass
	- 322 = fail, segv.

We can clearly state at this point that your patch has fixed an
attribute corruption problem because it makes a specific unit go
from fail to pass. If we see that unit test fail on other distros,
we know exactly what patch is needed to fix it. And the same can be
said for when we find the root cause of the SEGV failure....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Eric Sandeen]

On 10/8/13 2:27 PM, Dave Chinner wrote:
> On Tue, Oct 08, 2013 at 09:21:13AM -0500, Rich Johnston wrote:
>> On 10/07/2013 07:57 PM, Eric Sandeen wrote:
>>> On 10/7/13 7:53 PM, Dave Chinner wrote:
>>>> Two tests, please. move all the common parts into common/dump, and
>>>> write them as two separate tests. That way we can easily track what
>>>> test is failing just by looking at what harness test is failing...
>>>
>>> I'm not quite convinced that it's 2 separate tests, TBH.
>>>
>>> It's the same root cause; I guess there is a slightly different
>>> outcome because if you hit the same root cause enough times,
>>> you'll segfault.
>>
>>
>> Multiple DMF offline files are successfully restored but the attrs
>> are lost. I wanted to show/test that case.
>>
>> I agree with Eric that it is the same  root cause but because can
>> occur with successful dumps and does not segfault,  Thats why the 2
>> tests.
> 
> Ok, the problem might be triggering the same root cause, but in the
> case of unit tests that is usually irrelevant. That is each individual test
> should be independently tracked by the test harness regardless of
> the bug it triggers.
> 
> And reading on #xfs, the problem isn't clearly understood yet as
> both you and Eric are not sure exactly why there are differences in
> behaviour between different tests yet. e.g:
> 
> [09/10/13 02:13] <rjohnston1> Ahh OK but my DMF test case had several wholly-sparse (offline files) and the dump succeeded. 
> [09/10/13 02:18] <sandeen> tbh there is one thing I'm not clear on here, why a 1t sparse file behaves differently from a 1k sparse file
> [09/10/13 02:18] <sandeen> that seems . .  wrong
> [09/10/13 02:19] <sandeen> but I guess it must just key on i_size, not blocks
> [09/10/13 02:19] <sandeen> so anyway, maybe your dmf testcase had smaller file sizes?
> [09/10/13 02:19] <sandeen> sorry, I have to run & get missed homework to my kid @ school, bbiab.  Grr.
> [09/10/13 02:20] <rjohnston1> NP, yes they were smaller.
> [09/10/13 02:22] <rjohnston1> 100 10MB files no segfault, just trashed attrs.

But that's a testcase not yet written.  ;)  

I do understand why there is a difference between Rich's 1-file test and the 4-file test.  If you'd like to review the patch that fixes the root cause it might be more cleaer.

Between the 1 file & 4 files, the difference is that if we hit the root bug enough times, it will fill the partial-completion array, run out of slots, and return an error.  That error isn't handled and we get a segfault; I guess that's enough of a separate bug to warrant 2 tests.  One to be sure we handle the sparse files, and a second to test the error handling from this function if we hit the first bug enough times & return an error.

I _don't_ know how Rich/SGI managed to hit it with only 10MB files - I'm not clear on when xfsdump splits across streams.
Since that's Rich's bug I'll let him work that out.  ;)

-Eric

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Dave Chinner]

On Mon, Oct 07, 2013 at 02:38:35PM -0500, rjohnston@sgi.com wrote:
> +# create files for test 1, a large file so the sparse file
> +# is in the next stream.
> +_create_test1_files()
> +{
> +
> +	dd if=/dev/zero of=$SCRATCH_MNT/$DUMPDIR/10MB bs=1MB \
> +		count=10 2>&1 >> $seqres.full | _filter_dd
> +	truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse0 2>&1 \
> +		>> $seqres.full

I just noticed that 'truncate' is used here - we don't use that
anywhere else in xfstests, so either you need test for it in
common/config and use $TRUNCATE_PROG, or do like every other test
does and use:

	$XFS_IO_PROG -c "truncate 1t" $SCRATCH_MNT/$DUMPDIR/sparse0 

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore

[Rich Johnston]

On 10/07/2013 08:08 PM, Dave Chinner wrote:
> On Mon, Oct 07, 2013 at 02:38:35PM -0500, rjohnston@sgi.com wrote:
>> +# create files for test 1, a large file so the sparse file
>> +# is in the next stream.
>> +_create_test1_files()
>> +{
>> +
>> +	dd if=/dev/zero of=$SCRATCH_MNT/$DUMPDIR/10MB bs=1MB \
>> +		count=10 2>&1 >> $seqres.full | _filter_dd
>> +	truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse0 2>&1 \
>> +		>> $seqres.full
>
> I just noticed that 'truncate' is used here - we don't use that
> anywhere else in xfstests, so either you need test for it in
> common/config and use $TRUNCATE_PROG, or do like every other test
> does and use:
>
> 	$XFS_IO_PROG -c "truncate 1t" $SCRATCH_MNT/$DUMPDIR/sparse0
Good point I will make the changes.
>
> Cheers,
>
> Dave.
>

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

[PATCH V2] xfstests XFS: verify extended attributes after multi-stream xfsdump/xfsrestore are not lost

[rjohnston]

Verify extended attributes are not lost after multi-stream
xfsdump/xfsrestore of wholly-sparse files.  xfsrestore did not
recognize that if the LAST header was reached with no restoredsz set,
(i.e the LAST header is the only header), the following warning is
displayed:

  "partial_reg: Out of records. Extend attrs applied early."

and the extended attributes on the current and following restored
files are lost.

Signed-off-by: Rich Johnston <rjohnston@sgi.com>

---
V2:
Updated test description and comments
Changed test status to default to failure
Added missing _requires_XXXX statements
Removed the attr root (-R) namespace argument

diff --git a/tests/xfs/350 b/tests/xfs/350
new file mode 100644
index 0000000..47e0b7d
--- /dev/null
+++ b/tests/xfs/350
@@ -0,0 +1,139 @@
+#! /bin/bash
+# FS QA Test No. 350
+#
+# Verify extended attributes are not lost after multi-stream
+# xfsdump/xfsrestore of wholly-sparse files. The same root cause
+# occurs with successful dumps (test 1) and also segfaults (test 2). 
+
+#-----------------------------------------------------------------------
+# Copyright (c) 2013 SGI.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/dump
+
+# real QA test starts here
+_supported_fs xfs
+_supported_os Linux
+_require_scratch
+_scratch_mkfs
+_scratch_mount
+
+_require_multi_stream
+
+# dir where we'll restore
+RESTOREDIR=$SCRATCH_MNT/restore
+
+# subdir used for dump/restore
+DUMPDIR=dumpsrc
+
+# test extended attributes
+attr_name=testattr
+attr_value=1234
+
+# setup for test
+rm -r $seqres.full
+mkdir -p $RESTOREDIR
+mkdir -p $SCRATCH_MNT/$DUMPDIR
+
+# cleanup for next dump/restore.
+_clean_dirs()
+{
+	rm -rf $SCRATCH_MNT/$DUMPDIR/*
+	rm -rf $RESTOREDIR/*
+	rm -f $tmp.stream?
+}
+
+# set the extended attributes on the test files.
+_set_attrs()
+{
+	for fname in $(ls -dD $SCRATCH_MNT/$DUMPDIR/*); do
+		attr -s $attr_name -V $attr_value $fname \
+		2>&1 >> $seqres.full || _fail "could not set ATTR for $fname"
+	done
+}
+
+# perform a dump and restore.
+_do_dump_restore()
+{
+	$XFSDUMP_PROG -L session -M label1 -M label2 -f $tmp.stream1 \
+		-f $tmp.stream2 $SCRATCH_MNT -s $DUMPDIR \
+		2>&1 >> $seqres.full || _fail "dump failed"
+	$XFSRESTORE_PROG -F -f $tmp.stream1 -f $tmp.stream2 $RESTOREDIR \
+		2>&1 >> $seqres.full || _fail "restore failed"
+}
+
+# verify the restored files extended attributes and echo the error
+# (if any) so the test will continue on to test 2. If the restore is
+# not successful this will not be called to show the attributes are lost.
+_verify_attrs()
+{
+	for fname in $(ls -dD $RESTOREDIR/$DUMPDIR/*); do
+		attr -g $attr_name $fname 2>&1 | tee -a $seqres.full | \
+			grep $attr_value 2>&1 >> $seqres.full || \
+			echo "ATTR for $fname  DOES NOT match"
+	done
+}
+
+# create files for test 1, a large file so the sparse file
+# is in the next stream.
+_create_test1_files()
+{
+
+	dd if=/dev/zero of=$SCRATCH_MNT/$DUMPDIR/10MB bs=1MB \
+		count=10 2>&1 >> $seqres.full | _filter_dd
+	$XFS_IO_PROG -c "truncate --size=1t" $SCRATCH_MNT/$DUMPDIR/sparse0 \
+		2>&1 >> $seqres.full
+}
+
+# create 4 sparse files for test 2
+_create_test2_files()
+{
+	for i in `seq 1 4`; do
+		truncate --size=1t $SCRATCH_MNT/$DUMPDIR/sparse$i 2>&1 >> \
+		$seqres.full || _fail "failed to create sparse \"$i\""
+	done
+}
+echo "Silence is golden."
+echo "Starting Test 1" >> $seqres.full
+_clean_dirs
+_create_test1_files
+_set_attrs
+_do_dump_restore
+_verify_attrs
+
+echo "Starting Test 2" >> $seqres.full
+_clean_dirs
+_create_test2_files
+_set_attrs
+_do_dump_restore
+_verify_attrs
+
+# success, all done
+exit
diff --git a/tests/xfs/350.out b/tests/xfs/350.out
new file mode 100644
index 0000000..c725135
--- /dev/null
+++ b/tests/xfs/350.out
@@ -0,0 +1,2 @@
+QA output created by 350
+Silence is golden.
diff --git a/tests/xfs/group b/tests/xfs/group
index 352a4c5..2cd3cf0 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -179,3 +179,4 @@
 297 auto freeze
 298 auto attr symlink quick
 299 auto quota
+350 dump auto


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