Re: [PATCH] xfsrestore: fix multi stream support

Rich Johnston
2013-10-02T20:03:10Z

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