[PATCH] xfsdump support for 64K page size

[PATCH] xfsdump support for 64K page size

[Bill Kendall]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

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 <wkendall@sgi.com>

[-- Attachment #2: 64k_pagesize --]
[-- Type: text/plain, Size: 2826 bytes --]

---
 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(-)

Index: xfs-cmds/xfsdump/restore/inomap.c
===================================================================
--- xfs-cmds.orig/xfsdump/restore/inomap.c
+++ xfs-cmds/xfsdump/restore/inomap.c
@@ -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
 	 */
Index: xfs-cmds/xfsdump/dump/content.c
===================================================================
--- xfs-cmds.orig/xfsdump/dump/content.c
+++ xfs-cmds/xfsdump/dump/content.c
@@ -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 )
 
Index: xfs-cmds/xfsprogs/libhandle/handle.c
===================================================================
--- xfs-cmds.orig/xfsprogs/libhandle/handle.c
+++ xfs-cmds/xfsprogs/libhandle/handle.c
@@ -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);
 
Index: xfs-cmds/xfsprogs/libhandle/jdm.c
===================================================================
--- xfs-cmds.orig/xfsprogs/libhandle/jdm.c
+++ xfs-cmds/xfsprogs/libhandle/jdm.c
@@ -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 ),

[-- Attachment #3: Type: text/plain, Size: 121 bytes --]

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

Re: [PATCH] xfsdump support for 64K page size

[Mark Goodwin]

Bill Kendall wrote:
> 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 <wkendall@sgi.com>

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 <markgw@sgi.com>
	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.
	
	    Signed-off-by: Bill Kendall <wkendall@sgi.com>
	    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>
	    Signed-off-by: Mark Goodwin <markgw@sgi.com>

and for the libhandle changes :

git://oss.sgi.com/xfs/cmds/xfsprogs.git
	commit aa23a327ad7bb659f94c492119c928038d13b069
	Author: Mark Goodwin <markgw@sgi.com>
	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.
	
	    Signed-off-by: Bill Kendall <wkendall@sgi.com>
	    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>
	    Signed-off-by: Mark Goodwin <markgw@sgi.com>

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

Re: [PATCH] xfsdump support for 64K page size

[Christoph Hellwig]

On Thu, Jan 08, 2009 at 01:19:08PM +1100, Mark Goodwin wrote:
> 
> 
> Bill Kendall wrote:
> > 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 <wkendall@sgi.com>
> 
> 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 <markgw@sgi.com>
> 	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.
> 	
> 	    Signed-off-by: Bill Kendall <wkendall@sgi.com>
> 	    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>
> 	    Signed-off-by: Mark Goodwin <markgw@sgi.com>
> 
> 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.

> 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.

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

Re: [PATCH] xfsdump support for 64K page size

[Dave Chinner]

On Thu, Jan 08, 2009 at 01:19:08PM +1100, Mark Goodwin wrote:
> Bill Kendall wrote:
> > 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 <wkendall@sgi.com>
> 
> 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 <markgw@sgi.com>
> 	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.

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....

> In any case, Christoph, please pull these commits into your kernel.org 
> -dev trees.

NACK. Lets do a proper review cycle first.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfsdump support for 64K page size

[Eric Sandeen]

Dave Chinner wrote:
> On Thu, Jan 08, 2009 at 01:19:08PM +1100, Mark Goodwin wrote:
>> Bill Kendall wrote:
>>> 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 <wkendall@sgi.com>
>> 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 <markgw@sgi.com>
>> 	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.
> 
> 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.

Christoph pointed out on that thread that this define is linux-kernel
specific, so should probably be in an #ifdef, #else, too.

> ----
> 
> 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?

> ----
> 
> 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

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

Re: [PATCH] xfsdump support for 64K page size

[Bill Kendall]

Dave Chinner wrote:
> On Thu, Jan 08, 2009 at 01:19:08PM +1100, Mark Goodwin wrote:
>> Bill Kendall wrote:
>>> 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 <wkendall@sgi.com>
>> 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 <markgw@sgi.com>
>> 	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.
> 
> 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.

> 
> http://oss.sgi.com/archives/xfs/2008-05/msg00339.html

Sorry, didn't recall that you posted the patch to this list.
I got your patch off of the internal bug db.

> 
> 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....

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.

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

Re: [PATCH] xfsdump support for 64K page size

[Christoph Hellwig]

On Fri, Jan 09, 2009 at 01:36:30PM -0600, Bill Kendall wrote:
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump support for 64K page size

[Dave Chinner]

On Fri, Jan 09, 2009 at 01:36:30PM -0600, Bill Kendall wrote:
> Dave Chinner wrote:
> > 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.

.....

> 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.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfsdump support for 64K page size

[Bill Kendall]

On 01/14/2009 04:48 PM, Dave Chinner wrote:
> On Fri, Jan 09, 2009 at 01:36:30PM -0600, Bill Kendall wrote:
>> Dave Chinner wrote:
>>> 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.
> 
> .....
> 
>> 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.

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