xfs
[Top] [All Lists]

Re: [PATCH] xfsrestore: fix multi stream support

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfsrestore: fix multi stream support
From: Rich Johnston <rjohnston@xxxxxxx>
Date: Tue, 1 Oct 2013 17:02:31 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <524B4119.8020005@xxxxxxx>
References: <524AF8AE.5030300@xxxxxxx> <524B34D4.10807@xxxxxxxxxxx> <524B4119.8020005@xxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0
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@xxxxxxx>

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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs



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

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