xfsdump support for 64K page size
PATCH 1/1 xfsdump support for 64K page size
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
--- xfsdump/dump/content.c | 2 +- xfsdump/restore/inomap.c | 6 ------ xfsprogs/libhandle/handle.c | 5 ++++- xfsprogs/libhandle/jdm.c | 4 ++++ 4 files changed, 9 insertions(+), 8 deletions(-)
xfsdump/restore/inomap.c +0 -6
@@ -197,8 +197,6 @@ inomap_restore_pers( drive_t *drivep,*/ ASSERT (INOPERSEG == (sizeof ( ((seg_t * )0 )->lobits ) *NBBY ));ASSERT (sizeof ( hnk_t ) ==HNKSZ ); -ASSERT (HNKSZ >= pgsz ); -ASSERT ( ! (HNKSZ % pgsz ));ASSERT (sizeof ( pers_t ) <=PERSSZ );/* get inomap info from media hdr
@@ -224,8 +222,6 @@ inomap_restore_pers( drive_t *drivep,/* mmap the persistent hdr and space for the map */ -ASSERT (sizeof ( hnk_t ) * (size_t )hnkcnt >= pgsz ); -ASSERT ( ! (sizeof ( hnk_t ) * (size_t )hnkcnt % pgsz )); persp = ( pers_t * ) mmap_autogrow( PERSSZ +
@@ -355,8 +351,6 @@ inomap_sync_pers( char *hkdir )/* sanity checks */ ASSERT (sizeof ( hnk_t ) ==HNKSZ ); -ASSERT (HNKSZ >= pgsz ); -ASSERT ( ! (HNKSZ % pgsz ));/* only needed once per session */
xfsdump/dump/content.c +1 -1
@@ -210,7 +210,7 @@ typedef struct extent_group_context exte/* minimum sizes for extended attributes buffers */ -#define EXTATTR_LISTBUF_SZ ( 4 * pgsz ) +#define EXTATTR_LISTBUF_SZ ( XATTR_LIST_MAX )#define EXTATTR_RTRVARRAY_LEN ( 1 * pgsz )#define EXTATTR_DUMPBUF_SZ ( 4 * pgsz )
xfsprogs/libhandle/handle.c +4 -1
@@ -360,8 +360,11 @@ attr_list_by_handle(memcpy (&alhreq.pos, cursor,sizeof (alhreq.pos)); alhreq.flags = flags; - alhreq.buflen = bufsize; alhreq.buffer = buf; + alhreq.buflen = bufsize; +/* prevent needless EINVAL from the kernel */ +if (alhreq.buflen >XATTR_LIST_MAX ) + alhreq.buflen =XATTR_LIST_MAX ; error =xfsctl (path, fd,XFS_IOC_ATTRLIST_BY_HANDLE , &alhreq);
xfsprogs/libhandle/jdm.c +4 -0
@@ -166,6 +166,10 @@ jdm_attr_list( jdm_fshandle_t *fshp,filehandle_t filehandle;int rval; +/* prevent needless EINVAL from the kernel */ +if (bufsz >XATTR_LIST_MAX ) + bufsz =XATTR_LIST_MAX ; +jdm_fill_filehandle ( &filehandle, fshandlep, statp ); rval = attr_list_by_handle (( void * )&filehandle, sizeof( filehandle ),
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 5 lines · Bill Kendall
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Signed-off-by Bill Kendall <[email protected]>
Lachlan reviewed and ack'd this on an internal list and I've committed it (on Bill's behalf) as follows :
git://oss.sgi.com/xfs/cmds/xfsdump.git
commit 9502587dbbfdd465958889a568dc2842f10b1ff9
Author: Mark Goodwin <[email protected]>
Date: Thu Jan 8 12:37:53 2009 +1100
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
and for the libhandle changes :
git://oss.sgi.com/xfs/cmds/xfsprogs.git
commit aa23a327ad7bb659f94c492119c928038d13b069
Author: Mark Goodwin <[email protected]>
Date: Thu Jan 8 12:37:11 2009 +1100
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
This introduces a dependency between xfsdump and libhandle (in xfsprogs), which may or may not be an issue now that the cmds are in split trees. I guess maybe xfsdump/restore should rightfully be part of xfsprogs?
In any case, Christoph, please pull these commits into your kernel.org -dev trees.
Thanks
-- Mark
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 21 lines · Mark Goodwin
Quoted 5 lines · Bill Kendall
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Signed-off-by Bill Kendall <[email protected]>Lachlan reviewed and ack'd this on an internal list and I've committed it (on Bill's behalf) as follows :
git://oss.sgi.com/xfs/cmds/xfsdump.git
commit9502587dbbfdd465958889a568dc2842f10b1ff9
Author: Mark Goodwin <[email protected]>
Date: Thu Jan 8 12:37:53 2009 +1100Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Quoted 5 lines
Signed-off-by Bill Kendall <[email protected]>Signed-off-by Lachlan McIlroy <[email protected]>Signed-off-by Mark Goodwin <[email protected]>and for the libhandle changes :
If you commit something on someone else's behalf please use the --author argument to git-commit so that it shows up as coming from that author in the git version history. Or just use git-am on the patch mail which will sort out all this by itself.
Quoted 2 lines
This introduces a dependency between xfsdump and libhandle (in xfsprogs), which may or may not be an issue now that the cmds are in split trees.
It's not a new depdency. xfsdump depended on libhandle for a long time (and before it did depend on dmapi IIRC). Just to actually get dump properly working with 64k pages you need the most uptodate version of both packages.
Quoted 1 line
I guess maybe xfsdump/restore should rightfully be part of xfsprogs?
I think having them split makes a lot of sense as many people don't need dump. What we should eventually do is to move xfs_fsr over to xfsprogs.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 10 lines · Mark Goodwin
Quoted 5 lines · Bill Kendall
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Signed-off-by Bill Kendall <[email protected]>Lachlan reviewed and ack'd this on an internal list and I've committed it (on Bill's behalf) as follows :
Quoted 8 lines
git://oss.sgi.com/xfs/cmds/xfsdump.git
commit9502587dbbfdd465958889a568dc2842f10b1ff9
Author: Mark Goodwin <[email protected]>
Date: Thu Jan 8 12:37:53 2009 +1100Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
I guess I don't have a real name ;)
BTW, these changes are the *exact* patches I sent back in March. I note that the change logs from those patches have been dropped on the floor. i.e.:
http://oss.sgi.com/archives/xfs/2008-05/msg00339.html
The extended attr buffer size used by xfsdump is based on page size. The maximum buffer size the kernel will accept is 64k. On a 64k page machine, the default buffer size will be rejected by the kernel, thereby breaking dump and restore.
Limit the buffer size to XATTR_LIST_MAX in dump, restore and libhandle so the kernel won't reject otherwise valid requests.
----
http://oss.sgi.com/archives/xfs/2008-05/msg00340.html
xfsrestore has assumptions about page size built into the inode hunk size in the dump format. Seems to be a stupid thing to do - this patch simply comments out the asserts to allow it to work on 64k page size machine, but probably subtly breaks the code. Nasty hack, really, but allows xfsqa tests to pass.
----
I'd also like to know what validation has been done of the second patch. e.g. is it going to break when dump and restore are done on machines of different page size? This is why I didn't sign-off on the second patch....
Quoted 2 lines
In any case, Christoph, please pull these commits into your kernel.org -dev trees.
NACK. Lets do a proper review cycle first.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 27 lines · Dave Chinner
Quoted 9 lines · Mark Goodwin
Quoted 5 lines · Bill Kendall
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Signed-off-by Bill Kendall <[email protected]>Lachlan reviewed and ack'd this on an internal list and I've committed it (on Bill's behalf) as follows :
Quoted 8 lines
git://oss.sgi.com/xfs/cmds/xfsdump.git
commit9502587dbbfdd465958889a568dc2842f10b1ff9
Author: Mark Goodwin <[email protected]>
Date: Thu Jan 8 12:37:53 2009 +1100Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
I guess I don't have a real name ;)
BTW, these changes are the *exact* patches I sent back in March. I note that the change logs from those patches have been dropped on the floor. i.e.:
Quoted 7 lines
The extended attr buffer size used by xfsdump is based on page size. The maximum buffer size the kernel will accept is 64k. On a 64k page machine, the default buffer size will be rejected by the kernel, thereby breaking dump and restore.
Limit the buffer size to
XATTR_LIST_MAXin dump, restore and libhandle so the kernel won't reject otherwise valid requests.
Christoph pointed out on that thread that this define is linux-kernel specific, so should probably be in an #ifdef, #else, too.
Quoted 9 lines
----
http://oss.sgi.com/archives/xfs/2008-05/msg00340.html
xfsrestore has assumptions about page size built into the inode hunk size in the dump format. Seems to be a stupid thing to do - this patch simply comments out the asserts to allow it to work on 64k page size machine, but probably subtly breaks the code. Nasty hack, really, but allows xfsqa tests to pass.
Heh, I totally missed this and arrived at it independently, and hackily as well:
http://oss.sgi.com/bugzilla/show_bug.cgi?id=799
but I'm not sure that counts as review; hopefully Bill confirmed that this is safe?
Quoted 6 lines
----
I'd also like to know what validation has been done of the second patch. e.g. is it going to break when dump and restore are done on machines of different page size? This is why I didn't sign-off on the second patch....
*nod* - there is a "standard" dumpfile as part of xfsqa, so at least I know it does properly restore that, still. But has anything tested the other direction?
-Eric
Quoted 8 lines
Quoted 2 lines
In any case, Christoph, please pull these commits into your kernel.org -dev trees.
NACK. Lets do a proper review cycle first.
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 19 lines · Dave Chinner
Quoted 9 lines · Mark Goodwin
Quoted 5 lines · Bill Kendall
Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Signed-off-by Bill Kendall <[email protected]>Lachlan reviewed and ack'd this on an internal list and I've committed it (on Bill's behalf) as follows :
Quoted 8 lines
git://oss.sgi.com/xfs/cmds/xfsdump.git
commit9502587dbbfdd465958889a568dc2842f10b1ff9
Author: Mark Goodwin <[email protected]>
Date: Thu Jan 8 12:37:53 2009 +1100Various fixes to allow xfsdump/xfsrestore to work with 64K page size. This is essentially Chinner's patch from a while back.
Quoted 5 lines
I guess I don't have a real name ;)
BTW, these changes are the *exact* patches I sent back in March. I note that the change logs from those patches have been dropped on the floor. i.e.:
Right, only difference is that I removed the asserts rather than just having them commented out. In my determination the asserts are totally bogus -- there isn't a dependency on the system's page size in the inomap code.
Quoted 2 lines
Sorry, didn't recall that you posted the patch to this list. I got your patch off of the internal bug db.
Quoted 19 lines
The extended attr buffer size used by xfsdump is based on page size. The maximum buffer size the kernel will accept is 64k. On a 64k page machine, the default buffer size will be rejected by the kernel, thereby breaking dump and restore.
Limit the buffer size to
XATTR_LIST_MAXin dump, restore and libhandle so the kernel won't reject otherwise valid requests.----
http://oss.sgi.com/archives/xfs/2008-05/msg00340.html
xfsrestore has assumptions about page size built into the inode hunk size in the dump format. Seems to be a stupid thing to do - this patch simply comments out the asserts to allow it to work on 64k page size machine, but probably subtly breaks the code. Nasty hack, really, but allows xfsqa tests to pass.
Quoted 6 lines
----
I'd also like to know what validation has been done of the second patch. e.g. is it going to break when dump and restore are done on machines of different page size? This is why I didn't sign-off on the second patch....
The inomap code uses xfsdump's PGSZ variable, which is fixed at 4K. There's no dependency here on the system's actual page size. I was able to dump and then restore on a system with a different page size.
Quoted 5 lines
Quoted 3 lines
In any case, Christoph, please pull these commits into your kernel.org -dev trees.
NACK. Lets do a proper review cycle first.
Once that is done, I suggest we put Dave's original patches in the -dev trees. That way it'll have proper attribution as well as commit messages with some detail.
Bill
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 3 lines · Bill Kendall
Once that is done, I suggest we put Dave's original patches in the -dev trees. That way it'll have proper attribution as well as commit messages with some detail.
The commits were merged back to the -dev trees alredy, and at least for xfsprogs we already have a commit ontop. I'd say let's use how this was handled as a bad example and get over it.
What we should do however is putting dump images of every page size we can hold of (4k/8k/16k/64k) and maybe both endianesses into the xfstests repository so we can exercise them as part of the normal QA procedure.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Bill Kendall
Quoted 3 lines · Dave Chinner
BTW, these changes are the *exact* patches I sent back in March. I note that the change logs from those patches have been dropped on the floor. i.e.:
Right, only difference is that I removed the asserts rather than just having them commented out. In my determination the asserts are totally bogus -- there isn't a dependency on the system's page size in the inomap code.
.....
Quoted 3 lines
The inomap code uses xfsdump's PGSZ variable, which is fixed at 4K. There's no dependency here on the system's actual page size. I was able to dump and then restore on a system with a different page size.
Ok, that looks fine. however, there is a dependency that HNKSZ >= PGSZ, right? And that is currently hardwired to (4 * PGSZ)?
And given that the intent of the PGSZ was to be made variable at some point, isn't this really trying to ensure that HNKSZ is always greater than the PGSZ the program was built with?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 10 lines · Dave Chinner
Quoted 8 lines · Bill Kendall
Quoted 3 lines · Dave Chinner
BTW, these changes are the *exact* patches I sent back in March. I note that the change logs from those patches have been dropped on the floor. i.e.:
Right, only difference is that I removed the asserts rather than just having them commented out. In my determination the asserts are totally bogus -- there isn't a dependency on the system's page size in the inomap code.
Quoted 8 lines
.....
Quoted 3 lines
The inomap code uses xfsdump's PGSZ variable, which is fixed at 4K. There's no dependency here on the system's actual page size. I was able to dump and then restore on a system with a different page size.
Ok, that looks fine. however, there is a dependency that HNKSZ >= PGSZ, right? And that is currently hardwired to (4 * PGSZ)?
I don't think so, or at least I'm not making the connection. HNKSZ need only be large enough to contain at least one seg_t plus the bookkeeping info in a hnk_t. Lookups are more efficient with a few large hunks compared to many small ones though, and since the list of hunks will be memory mapped it made sense to make HNKSZ at least as large as a page (the granularity of a mmap operation, IIRC).
Now with 64K pages, inomap lookups could be made more efficient by increasing HNKSZ, but at the expense of breaking the dump format. If/when it is okay to do that, I think it would be simpler to do away with the list of hnk_t's, and just have a single array (or other container) with all the seg_t's.
Quoted 4 lines
And given that the intent of the PGSZ was to be made variable at some point, isn't this really trying to ensure that HNKSZ is always greater than the PGSZ the program was built with?
Good point about PGSZ being variable. Since HNKSZ must remain constant to keep the dump format unchanged, that implies it should not be based on PGSZ. At a quick glance I see that other structures in the dump format are based on PGSZ as well, so really the whole use of PGSZ needs to be reviewed.
Bill
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs