From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay1.corp.sgi.com [137.38.102.111]) by oss.sgi.com (Postfix) with ESMTP id 481707F7D for ; Tue, 1 Oct 2013 17:02:37 -0500 (CDT) Message-ID: <524B4677.40200@sgi.com> Date: Tue, 1 Oct 2013 17:02:31 -0500 From: Rich Johnston MIME-Version: 1.0 Subject: Re: [PATCH] xfsrestore: fix multi stream support References: <524AF8AE.5030300@sgi.com> <524B34D4.10807@sandeen.net> <524B4119.8020005@sgi.com> In-Reply-To: <524B4119.8020005@sgi.com> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Eric Sandeen Cc: xfs-oss 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 >>> >>> 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