On Tue, Sep 01, 2015 at 03:38:29PM -0500, Rich Johnston wrote:
> Restoring an incremental level 1 dump will fail with the following error
> if the fs uuid of the most recent level 0 dump in the inventory does not
> match level 1 dump we are restoring.
>
> xfsrestore: ERROR: selected dump not based on previously applied dump
>
> This can happen when you have multiple filesystems and you are restoring
> a level 1 or greater dump of filesystem FS1 but the most recent level 0
> dump in the inventory was filesystem FS2
>
> The fix is to ensure the fs uuid of the inventory entry and the dump to
> be restored match.
>
> Signed-off-by: Rich Johnston <rjohnston@xxxxxxx>
> ---
I'm not really familiar with xfsdump, but a few comments below...
> dump/content.c | 8 ++-
> inventory/inv_api.c | 108 ++++++++++++++++++++++++++++++--------------------
> inventory/inv_mgr.c | 32 ++++++++++----
> inventory/inv_priv.h | 7 +--
> inventory/inventory.h | 5 ++
> restore/content.c | 17 +++++--
> 6 files changed, 113 insertions(+), 64 deletions(-)
>
...
> Index: b/inventory/inv_mgr.c
> ===================================================================
> --- a/inventory/inv_mgr.c
> +++ b/inventory/inv_mgr.c
> @@ -134,6 +134,7 @@ get_sesstoken( inv_idbtoken_t tok )
> /*---------------------------------------------------------------------------*/
> bool_t
> invmgr_query_all_sessions (
> + uuid_t *fsidp,
> void *inarg,
> void **outarg,
> search_callback_t func)
> @@ -169,7 +170,7 @@ invmgr_query_all_sessions (
> mlog( MLOG_NORMAL | MLOG_INV, _(
> "INV: Cant get inv-name for uuid\n")
> );
> - return BOOL_FALSE;
> + continue;
> }
> strcat( fname, INV_INVINDEX_PREFIX );
> invfd = open( fname, INV_OFLAG(forwhat) );
> @@ -178,9 +179,9 @@ invmgr_query_all_sessions (
> "INV: Open failed on %s\n"),
> fname
> );
> - return BOOL_FALSE;
> + continue;
> }
Now that the above two cases don't return false, we can end up returning
true if these error cases persist throughout the loop. Perhaps we should
define 'bool ret = false,' return that variable everywhere and only set
it true when appropriate.
> - result = search_invt( invfd, inarg, &objectfound, func );
> + result = search_invt(fsidp, invfd, inarg, &objectfound, func);
> close(invfd);
>
> /* if error return BOOL_FALSE */
...
> @@ -272,19 +274,31 @@ search_invt(
> }
> free ( scnt );
>
> - while ( nsess ) {
> + for (j = nsess - 1; j >= 0; j--) {
> + invt_session_t ses;
> +
> /* fd is kept locked until we return from the
> callback routine */
>
> /* Check to see if this session has been pruned
> * by xfsinvutil before checking it.
> */
> - if ( harr[nsess - 1].sh_pruned ) {
> - --nsess;
> + if (harr[j].sh_pruned) {
> continue;
> }
> - found = (* do_chkcriteria ) ( fd, &harr[ --nsess ],
> - arg, buf );
> +
> + /* if we need to check the fs uuid's and they don't
> + * match or we fail to get the session record,
> + * then keep looking
> + */
> + if (fsidp &&
> + (GET_REC_NOLOCK(fd, &ses, sizeof(invt_session_t),
> + harr[j].sh_sess_off) ==
> + sizeof(invt_session_t)) &&
> + uuid_compare(ses.s_fsid, *fsidp))
> + continue ;
> +
This seems like kind of a loaded check. GET_REC_NOLOCK() looks like it
returns -1 if the read doesn't return the exact size of the buffer, so
we probably don't need to check that here. It also seems like we should
return an error if an error occurs rather than just continue on. How
about something like this?
...
if (fsidp) {
ret = GET_REC_NOLOCK(fd, &ses,
sizeof(invt_session_t),
harr[j].sh_sess_off);
if (ret < 0) {
/* XXX: clean up here */
return ret;
}
ret = uuid_compare(ses.s_fsid, *fsidp);
if (ret)
continue;
}
> + found = (* do_chkcriteria ) (fd, &harr[j], arg, buf);
> if (! found ) continue;
>
> /* we found what we need; just return */
...
> Index: b/restore/content.c
> ===================================================================
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -2179,8 +2179,9 @@ content_stream_restore( ix_t thrdix )
> if ( ! drivep->d_isnamedpipepr
> &&
> ! drivep->d_isunnamedpipepr ) {
> - ok = inv_get_session_byuuid( &grhdrp->gh_dumpid,
> - &sessp );
> + ok = inv_get_session_byuuid((uuid_t *)0,
> + &grhdrp->gh_dumpid,
> + &sessp);
> if ( ok && sessp ) {
> mlog( MLOG_VERBOSE, _(
> "using online session inventory\n") );
> @@ -3736,9 +3737,11 @@ Inv_validate_cmdline( void )
> ok = BOOL_FALSE;
> sessp = 0;
> if ( tranp->t_reqdumpidvalpr ) {
> - ok = inv_get_session_byuuid( &tranp->t_reqdumpid, &sessp );
> + ok = inv_get_session_byuuid((uuid_t *)0, &tranp->t_reqdumpid,
> + &sessp );
> } else if ( tranp->t_reqdumplabvalpr ) {
> - ok = inv_get_session_bylabel( tranp->t_reqdumplab, &sessp );
> + ok = inv_get_session_bylabel((uuid_t *)0, tranp->t_reqdumplab,
> + &sessp );
Can we use NULL instead of 0 in these cases? Not sure the cast is really
necessary either..?.
Brian
> }
> rok = BOOL_FALSE;
> if ( ok && sessp ) {
> @@ -6812,11 +6815,13 @@ askinvforbaseof( uuid_t baseid, inv_sess
> /* get the base session
> */
> if ( resumedpr ) {
> - ok = inv_lastsession_level_equalto( invtok,
> + ok = inv_lastsession_level_equalto( &sessp->s_fsid,
> + invtok,
> ( u_char_t )level,
> &basesessp );
> } else {
> - ok = inv_lastsession_level_lessthan( invtok,
> + ok = inv_lastsession_level_lessthan( &sessp->s_fsid,
> + invtok,
> ( u_char_t )level,
> &basesessp );
> }
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
