xfsprogs: libxfs-4.1-update branch created

A good example of this is Christoph's xfs_trans_commit() patchset. There's 42 xfs_trans_commit() calls in userspace and it has a completely different implementation in userspace. To put that in context, there are 61 xfs_trans_commit() in the kernel code. If I merge that changeset into the kernel code, then Christoph walks away and it has become the Maintainer's problem to make userspace work with the API change.

Hi folks,

I' ve just added a branch to the xfsprogs repository on kernel.org that is up to date with the current 4.1-rc2 kernel code. It's smoke tested, so I don't think there's any major problems with it. Can everyone with development patches (i.e. things that change on disk formats) for xfsprogs please rebase their work against this branch, retest and repost? This branch will end up being the 3.3 release tree...

FWIW, now that I've done this update, I'm considered how we keep this up to date in future. Keeping the userspace code up to date and working with all the internal kernel API changes takes quite a bit of effort and that currently is all being done by the Maintainer. i.e. me. This is the reason that userspace is not updated frequently.

With this update, libxfs in the kernel and userspace are mostly identical, so I'm thinking that a new rule for XFS developers needs to be put in place: if you modify a libxfs kernel API, then you need to make the same change to userspace. This means all the changes needed for xfs_repair, xfs_db, etc, need to be done as well, because sometimes these things are non-trivial.

A good example of this is Christoph's xfs_trans_commit() patchset. There's 42 xfs_trans_commit() calls in userspace and it has a completely different implementation in userspace. To put that in context, there are 61 xfs_trans_commit() in the kernel code. If I merge that changeset into the kernel code, then Christoph walks away and it has become the Maintainer's problem to make userspace work with the API change.

This *sucks* from a maintainer's POV - - it simply does not scale. This is one of the reasons why my productivity as an XFS developer has cratered over the last year. This userspace update work needs to be distributed over the developers making the API and functionality changes, so I'm throwing this out there to see what people think about solving this problem.

I'm thinking that we need a userspace dev branch similar to the for-next branch for the kernel code, where we aggregate topic branches that contain each set of userspace patches that add/change functionality that is shared with the kernel. If the developer hasn't done the work to update userspace and test the changes there, then the change is incomplete.

Note that I'm only suggesting this for the main XFS developers making internal API or on-disk format changes to XFS - the typical drive-by patches and bug fixes from random people we commit to the kernel code will still need the maintainer to push them across to userspace.

This will make my life easier from both an update and maintenance point of view, and will make it easier for us as developers to get the latest dev code faster. It will also make porting kernel changes over to userspace faster, as the patches will mostly apply cleanly to the userspace libxfs code base. And if it gets merged quicker, tehn we'll get better test coverage of the dev tree because we'll be testing it sooner. And as developers, we won't have to be working out what changed and needs forward porting from the kernel to userspace to make stuff work....

So, what do the people I'm asking to do more work so I don't have to spend so much time on time consuming maintenance tasks think?

Cheers,

Dave.

Signature · 9 lines

Dave Chinner
[email protected]



_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 62 lines · Dave Chinner

Hi folks,

I' ve just added a branch to the xfsprogs repository on kernel.org that is up to date with the current 4.1-rc2 kernel code. It's smoke tested, so I don't think there's any major problems with it. Can everyone with development patches (i.e. things that change on disk formats) for xfsprogs please rebase their work against this branch, retest and repost? This branch will end up being the 3.3 release tree...

FWIW, now that I've done this update, I'm considered how we keep this up to date in future. Keeping the userspace code up to date and working with all the internal kernel API changes takes quite a bit of effort and that currently is all being done by the Maintainer. i.e. me. This is the reason that userspace is not updated frequently.

With this update, libxfs in the kernel and userspace are mostly identical, so I'm thinking that a new rule for XFS developers needs to be put in place: if you modify a libxfs kernel API, then you need to make the same change to userspace. This means all the changes needed for xfs_repair, xfs_db, etc, need to be done as well, because sometimes these things are non-trivial.

A good example of this is Christoph's xfs_trans_commit() patchset. There's 42 xfs_trans_commit() calls in userspace and it has a completely different implementation in userspace. To put that in context, there are 61 xfs_trans_commit() in the kernel code. If I merge that changeset into the kernel code, then Christoph walks away and it has become the Maintainer's problem to make userspace work with the API change.

This *sucks* from a maintainer's POV - - it simply does not scale. This is one of the reasons why my productivity as an XFS developer has cratered over the last year. This userspace update work needs to be distributed over the developers making the API and functionality changes, so I'm throwing this out there to see what people think about solving this problem.

I'm thinking that we need a userspace dev branch similar to the for-next branch for the kernel code, where we aggregate topic branches that contain each set of userspace patches that add/change functionality that is shared with the kernel. If the developer hasn't done the work to update userspace and test the changes there, then the change is incomplete.

Note that I'm only suggesting this for the main XFS developers making internal API or on-disk format changes to XFS - the typical drive-by patches and bug fixes from random people we commit to the kernel code will still need the maintainer to push them across to userspace.

This will make my life easier from both an update and maintenance point of view, and will make it easier for us as developers to get the latest dev code faster. It will also make porting kernel changes over to userspace faster, as the patches will mostly apply cleanly to the userspace libxfs code base. And if it gets merged quicker, tehn we'll get better test coverage of the dev tree because we'll be testing it sooner. And as developers, we won't have to be working out what changed and needs forward porting from the kernel to userspace to make stuff work....

Quoted 3 lines

So, what do the people I'm asking to do more work so I don't have to spend so much time on time consuming maintenance tasks think?

This seems reasonable to me. As it is, I'd expect to do this anyways for anything that changes on-disk format. Such a change isn't really complete if mkfs/xfs_repair/xfs_db can't handle it appropriately. Provided we keep the development branch up to date, I don't see much harm in extending that to libxfs API level changes as well.

What's the proposition with regard to submission/review process? I don't think we necessarily need the userspace bits until there is some review feedback on the kernel bits because that just increases development and review overhead (though nothing precludes posting both, of course). Also

(and I think we discussed this briefly at LSF), I assume it is reasonable to condense a kernel patch series to a single userspace "sync XYZ feature to xfsprogs" patch for the bits that port directly over, since we have the kernel git log for finer grained history..?

Case in
point: I could squash the sparse inode kernel patches into a single xfsprogs patch. The functional xfsprogs bits on top of that (e.g., mkfs, repair, etc.) would of course remain as independent patches that require indepenent review.

Something to note here is that I think this might warrant a bit more positive feedback with regard to when things are considered reviewed on the kernel side (i.e., merge is imminent). Perhaps we could create the kernel topic branch but not merge it until the userspace bits are available..? Just a thought, whatever works best for you is probably fine provided the developer is made clear where things stand.

Quoted 67 lines · Brian Foster

Quoted 62 lines · Dave Chinner

Hi folks,

I' ve just added a branch to the xfsprogs repository on kernel.org that is up to date with the current 4.1-rc2 kernel code. It's smoke tested, so I don't think there's any major problems with it. Can everyone with development patches (i.e. things that change on disk formats) for xfsprogs please rebase their work against this branch, retest and repost? This branch will end up being the 3.3 release tree...

FWIW, now that I've done this update, I'm considered how we keep this up to date in future. Keeping the userspace code up to date and working with all the internal kernel API changes takes quite a bit of effort and that currently is all being done by the Maintainer. i.e. me. This is the reason that userspace is not updated frequently.

With this update, libxfs in the kernel and userspace are mostly identical, so I'm thinking that a new rule for XFS developers needs to be put in place: if you modify a libxfs kernel API, then you need to make the same change to userspace. This means all the changes needed for xfs_repair, xfs_db, etc, need to be done as well, because sometimes these things are non-trivial.

A good example of this is Christoph's xfs_trans_commit() patchset. There's 42 xfs_trans_commit() calls in userspace and it has a completely different implementation in userspace. To put that in context, there are 61 xfs_trans_commit() in the kernel code. If I merge that changeset into the kernel code, then Christoph walks away and it has become the Maintainer's problem to make userspace work with the API change.

This *sucks* from a maintainer's POV - - it simply does not scale. This is one of the reasons why my productivity as an XFS developer has cratered over the last year. This userspace update work needs to be distributed over the developers making the API and functionality changes, so I'm throwing this out there to see what people think about solving this problem.

I'm thinking that we need a userspace dev branch similar to the for-next branch for the kernel code, where we aggregate topic branches that contain each set of userspace patches that add/change functionality that is shared with the kernel. If the developer hasn't done the work to update userspace and test the changes there, then the change is incomplete.

Note that I'm only suggesting this for the main XFS developers making internal API or on-disk format changes to XFS - the typical drive-by patches and bug fixes from random people we commit to the kernel code will still need the maintainer to push them across to userspace.

This will make my life easier from both an update and maintenance point of view, and will make it easier for us as developers to get the latest dev code faster. It will also make porting kernel changes over to userspace faster, as the patches will mostly apply cleanly to the userspace libxfs code base. And if it gets merged quicker, tehn we'll get better test coverage of the dev tree because we'll be testing it sooner. And as developers, we won't have to be working out what changed and needs forward porting from the kernel to userspace to make stuff work....

Quoted 3 lines

So, what do the people I'm asking to do more work so I don't have to spend so much time on time consuming maintenance tasks think?

Quoted 5 lines

This seems reasonable to me. As it is, I'd expect to do this anyways for anything that changes on-disk format. Such a change isn't really complete if mkfs/xfs_repair/xfs_db can't handle it appropriately. Provided we keep the development branch up to date, I don't see much harm in extending that to libxfs API level changes as well.

Right - the point of this is making sure we keep userspace up to date, rather than doing an update once every blue moon and it being a lot of work to do.

Quoted 4 lines

What's the proposition with regard to submission/review process? I don't think we necessarily need the userspace bits until there is some review feedback on the kernel bits because that just increases development and review overhead (though nothing precludes posting both, of course). Also

Yes, that's fine - the userspace libxfs bits should be a straight conversion of the kernel patches with a simple bit of path stripping. e.g. the kernel patch path will be a/fs/xfs/libxfs/.... and the xfsprogs patch path is a/libxfs/...

Hence the same patch should apply, simply with a -p 5 (?) path strip level rather than -p 1. i.e. patch -p 5 < kernel.diff should simply apply the kernel patches to the xfsprogs libxfs.

Quoted 4 lines

(and I think we discussed this briefly at LSF), I assume it is reasonable to condense a kernel patch series to a single userspace "sync XYZ feature to xfsprogs" patch for the bits that port directly over, since we have the kernel git log for finer grained history..?

Haven't really thought about it. Fine grained history is more important for the kernel, but for userspace I can still see it being useful for things like API changes where the same set of steps made to the kernel code to make review easy should be made to make the xfs_repair/db/... changes easy to review.

Quoted 5 lines

Case in
point: I could squash the sparse inode kernel patches into a single xfsprogs patch. The functional xfsprogs bits on top of that (e.g., mkfs, repair, etc.) would of course remain as independent patches that require indepenent review.

Right, for something that is all new code this can be done. It does mean we'll have to be vigilant about merging bug fixes into the userspace code, though. Those are really only picked up during the large merges right now...

Quoted 6 lines

Something to note here is that I think this might warrant a bit more positive feedback with regard to when things are considered reviewed on the kernel side (i.e., merge is imminent). Perhaps we could create the kernel topic branch but not merge it until the userspace bits are available..? Just a thought, whatever works best for you is probably fine provided the developer is made clear where things stand.

Well, for on-disk format changes we'll need the userspace code at tha same time so that reviewers can test the changes, but for everything else a port from the topic branch that gets merged into the for-next tree is probably fine from a timing POV.

What I'd really like to is for all the changes merged in a kernel -rc1 window are already merged into the userspace tree, or are at least well on their way to being merged. That way we can even start to think about a simple rolling release of xfsprogs that mirrors the timing of the kernel release cycle. That way for new features we get userspace support released at the same time that kernel support is released....

The other benefit of doing this is that we get the changes committed while it is still fresh in our minds - we don't have to look back to code written 6-9 months ago and scratch our heads wondering why the obvious merge changes aren't working because we've forgotten some little detail about the change....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

What's the proposition with regard to submission/review process? I don't think we necessarily need the userspace bits until there is some review feedback on the kernel bits because that just increases development and review overhead (though nothing precludes posting both, of course). Also

(and I think we discussed this briefly at LSF), I assume it is reasonable to condense a kernel patch series to a single userspace "sync XYZ feature to xfsprogs" patch for the bits that port directly over, since we have the kernel git log for finer grained history..? Case in
point: I could squash the sparse inode kernel patches into a single xfsprogs patch. The functional xfsprogs bits on top of that (e.g., mkfs, repair, etc.) would of course remain as independent patches that require indepenent review.

Hi folks,

This userspace update work needs to
be distributed over the developers making the API and functionality changes, so I'm throwing this out there to see what people think about solving this problem.

So, what do the people I'm asking to do more work so I don't have to spend so much time on time consuming maintenance tasks think?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 1 line · Dave Chinner

Hi folks,

...

Quoted 4 lines

This userspace update work needs to
be distributed over the developers making the API and functionality changes, so I'm throwing this out there to see what people think about solving this problem.

...

Quoted 2 lines

So, what do the people I'm asking to do more work so I don't have to spend so much time on time consuming maintenance tasks think?

I think it makes perfect sense, and I'm happy to do it. The giant sync-ups after the fact have never been ideal from any perspective.

I'm glad you brought this up; I don't think anyone ever meant to simply leave this to the maintainer - it just ended up that way, because the rules or best practices weren't necessarily clear.

So; do you envision a 0/8 patch series, 4 of which touch libxfs, to now be a 0/12 patch series, with the userspace bits i.e. at the end? I'd be perfectly happy with this; once libxfs stays up to date in userspace, it should be trivial.

Quoted 4 lines

So; do you envision a 0/8 patch series, 4 of which touch libxfs, to

Quoted 3 lines

now be a 0/12 patch series, with the userspace bits i.e. at the end? I'd be perfectly happy with this; once libxfs stays up to date in userspace, it should be trivial.

I'd expect separate patch series, because it's kinda hard to do a git send-email or guilt patchbomb that spans mutltiple repositories.... :P

One interesting problem that arises from this is in xfs_format.h:

/* On-disk XFS extended attribute names */

#define SGI_ACL_FILE            (char *)"SGI_ACL_FILE"
#define SGI_ACL_DEFAULT         (char *)"SGI_ACL_DEFAULT"
#define SGI_ACL_FILE_SIZE       (sizeof(SGI_ACL_FILE)-1)
#define SGI_ACL_DEFAULT_SIZE    (sizeof(SGI_ACL_DEFAULT)-1)

(aside: why is it unsigned in the kernel but not here?)

Anyway: that "sizeof" gives us 7, because it's getting the size of the pointer, not the string literal. Cool eh! Never mattered in the kernel, because the _SIZE macros aren't used.

I'm not actually sure what the cleanest way to fix this is; casting string literals gets way past my comfort level with C pedantry; technically if we want an array of unsigned chars, we probably need:

unsigned char *SGI_ACL_FILE[] = "SGI_ACL_FILE"; unsigned char *SGI_ACL_DEFAULT[] = "SGI_ACL_DEFAULT";

Quoted 20 lines · Eric Sandeen

One interesting problem that arises from this is in xfs_format.h:

/* On-disk XFS extended attribute names */

#define SGI_ACL_FILE            (char *)"SGI_ACL_FILE"
#define SGI_ACL_DEFAULT         (char *)"SGI_ACL_DEFAULT"
#define SGI_ACL_FILE_SIZE       (sizeof(SGI_ACL_FILE)-1)
#define SGI_ACL_DEFAULT_SIZE    (sizeof(SGI_ACL_DEFAULT)-1)

(aside: why is it unsigned in the kernel but not here?)

Anyway: that "sizeof" gives us 7, because it's getting the size of the pointer, not the string literal. Cool eh! Never mattered in the kernel, because the _SIZE macros aren't used.

I'm not actually sure what the cleanest way to fix this is; casting string literals gets way past my comfort level with C pedantry; technically if we want an array of unsigned chars, we probably need:

Quoted 2 lines

unsigned char *SGI_ACL_FILE[] = "SGI_ACL_FILE"; unsigned char *SGI_ACL_DEFAULT[] = "SGI_ACL_DEFAULT";

Er, drop the * of course ...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

One interesting problem that arises from this is in xfs_format.h:

/* On-disk XFS extended attribute names */

#define SGI_ACL_FILE            (char *)"SGI_ACL_FILE"
#define SGI_ACL_DEFAULT         (char *)"SGI_ACL_DEFAULT"
#define SGI_ACL_FILE_SIZE       (sizeof(SGI_ACL_FILE)-1)
#define SGI_ACL_DEFAULT_SIZE    (sizeof(SGI_ACL_DEFAULT)-1)

(aside: why is it unsigned in the kernel but not here?)

Anyway: that "sizeof" gives us 7, because it's getting the size of the pointer, not the string literal. Cool eh! Never mattered in the kernel, because the _SIZE macros aren't used.