* [GIT PULL] xfs: Delay Ready Attributes
@ 2021-06-09 19:44 Allison Henderson
2021-06-09 22:03 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Allison Henderson @ 2021-06-09 19:44 UTC (permalink / raw)
To: linux-xfs
Hi Darrick,
I've created a branch and tag for the delay ready attribute series. I'ved
added the rvbs since the last review, but otherwise it is unchanged since
v20.
Please pull from the tag decsribed below.
Thanks!
Allison
The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2:
xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700)
are available in the git repository at:
https://github.com/allisonhenderson/xfs_work.git tags/xfs-delay-ready-attrs-v20.1
for you to fetch changes up to 816c8e39b7ea0875640312c9ed3be0d5a68d7183:
xfs: Make attr name schemes consistent (2021-06-09 09:34:05 -0700)
----------------------------------------------------------------
xfs: Delay Ready Attributes
Hi all,
This set is a subset of a larger series for Dealyed Attributes. Which is a
subset of a yet larger series for parent pointers. Delayed attributes allow
attribute operations (set and remove) to be logged and committed in the same
way that other delayed operations do. This allows more complex operations (like
parent pointers) to be broken up into multiple smaller transactions. To do
this, the existing attr operations must be modified to operate as a delayed
operation. This means that they cannot roll, commit, or finish transactions.
Instead, they return -EAGAIN to allow the calling function to handle the
transaction. In this series, we focus on only the delayed attribute portion.
We will introduce parent pointers in a later set.
The set as a whole is a bit much to digest at once, so I usually send out the
smaller sub series to reduce reviewer burn out. But the entire extended series
is visible through the included github links.
Updates since v19: Added Darricks fix for the remote block accounting as well as
some minor nits about the default assert in xfs_attr_set_iter. Spent quite
a bit of time testing this cycle to weed out any more unexpected bugs. No new
test failures were observed with the addition of this set.
xfs: Fix default ASSERT in xfs_attr_set_iter
Replaced the assert with ASSERT(0);
xfs: Add delay ready attr remove routines
Added Darricks fix for remote block accounting
This series can be viewed on github here:
https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_v20
As well as the extended delayed attribute and parent pointer series:
https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_v20_extended
And the test cases:
https://github.com/allisonhenderson/xfs_work/tree/pptr_xfstestsv3
In order to run the test cases, you will need have the corresponding xfsprogs
changes as well. Which can be found here:
https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_xfsprogs_v20
https://github.com/allisonhenderson/xfs_work/tree/delay_ready_attrs_xfsprogs_v20_extended
To run the xfs attributes tests run:
check -g attr
To run as delayed attributes run:
export MOUNT_OPTIONS="-o delattr"
check -g attr
To run parent pointer tests:
check -g parent
I've also made the corresponding updates to the user space side as well, and ported anything
they need to seat correctly.
Questions, comment and feedback appreciated!
Thanks all!
Allison
----------------------------------------------------------------
Allison Henderson (14):
xfs: Reverse apply 72b97ea40d
xfs: Add xfs_attr_node_remove_name
xfs: Refactor xfs_attr_set_shortform
xfs: Separate xfs_attr_node_addname and xfs_attr_node_addname_clear_incomplete
xfs: Add helper xfs_attr_node_addname_find_attr
xfs: Hoist xfs_attr_node_addname
xfs: Hoist xfs_attr_leaf_addname
xfs: Hoist node transaction handling
xfs: Add delay ready attr remove routines
xfs: Add delay ready attr set routines
xfs: Remove xfs_attr_rmtval_set
xfs: Clean up xfs_attr_node_addname_clear_incomplete
xfs: Fix default ASSERT in xfs_attr_set_iter
xfs: Make attr name schemes consistent
fs/xfs/libxfs/xfs_attr.c | 910 ++++++++++++++++++++++++----------------
fs/xfs/libxfs/xfs_attr.h | 403 ++++++++++++++++++
fs/xfs/libxfs/xfs_attr_leaf.c | 4 +-
fs/xfs/libxfs/xfs_attr_leaf.h | 2 +-
fs/xfs/libxfs/xfs_attr_remote.c | 167 ++++----
fs/xfs/libxfs/xfs_attr_remote.h | 8 +-
fs/xfs/xfs_attr_inactive.c | 2 +-
fs/xfs/xfs_trace.h | 2 -
8 files changed, 1032 insertions(+), 466 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [GIT PULL] xfs: Delay Ready Attributes 2021-06-09 19:44 [GIT PULL] xfs: Delay Ready Attributes Allison Henderson @ 2021-06-09 22:03 ` Dave Chinner 2021-06-10 0:28 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Dave Chinner @ 2021-06-09 22:03 UTC (permalink / raw) To: Allison Henderson; +Cc: linux-xfs On Wed, Jun 09, 2021 at 12:44:47PM -0700, Allison Henderson wrote: > Hi Darrick, > > I've created a branch and tag for the delay ready attribute series. I'ved > added the rvbs since the last review, but otherwise it is unchanged since > v20. > > Please pull from the tag decsribed below. Yay! At last! Good work, Allison. :) Nothing to worry about here, but I thought I'd make an observation on the construction branches for pull requests seeing as pull request are becoming our way of saying "this code is ready to merge". > Thanks! > Allison > > The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2: > > xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700) This looks like it has been built on top of a specific commit in the linux-xfs tree - perhaps a for-next branch before all the most recent development branches have been merged in. The problem with doing this is that the for-next tree can rebase, which can leave your tree with orphaned commits that are no longer in the main development branch or upstream. While these commits are upstream now, this isn't an issue for this particular branch and pull request. i.e. if the recent rebase of the for-next branch rewrote the above commit, pulling this branch would cause all sorts of problems. So to avoid this sort of issue with pull requests, and to allow the maintainer (darrick) to be able to easily reformulate the for-next tree just by merging branches, pull requests should all come from a known upstream base. In the case of pull requests for the xfs tree, that known base is the master branch of the XFS tree. The only time that you wouldn't do this is when your work is dependent on some other set of fixes. Those fixes then need to be in a stable upstream branch somewhere, which you then merge into your own dev branch based on xfs/master and the put your own commits on top of. IOWs, you start your own branch with a merge commit... If you do this, you should note in the pull request that there are other branches merged into this pull request and where they came from. THat way the maintainer can determine if the branch is actually stable and will end up being merged upstream unchanged from it's current state. It is also nice to tell the maintainer that you've based the branch on a stable XFS commit ahead of the current master branch. This might be necessary because there's a dependency between multiple development branches that are being merged one at a time in seperate pull requests. In terms of workflow, what this means is that development is done on a xfs/master based branch. i.e. dev branches are built like this: $ git checkout -b dev-branch-1 xfs/master $ git merge remote/stable-dev-branch $ git merge local-dependent-dev-branch $ <apply all your changes> $ <build kernel and test> And for testing against the latest -rc (say 5.13-rc5) and for-next kernels you do: $ git checkout -b testing v5.13-rc5 $ git merge xfs/for-next $ git merge dev-branch-1 <resolve conflicts> $ git merge dev-branch-2 <resolve conflicts> .... $ git merge dev-branch-N <resolve conflicts> $ <build kernel and test> This means that each dev branch has all the correct dependencies built into it, and they can be pulled by anyone without perturbing their local tree for testing and review because they are all working on the same xfs/master branch as your branches are. This also means that the xfs/for-next tree can remain based on xfs/master and be reformulated against xfs/master in a repeatable manner. It just makes everything easier if all pull requests are sent from the same stable base commit... Anyway, this isn't an issue for this pull-req because it is based on a stable XFS commit in a branch based on xfs/master, but I thought it's worth pointing out the pitfalls of using random stable commits as the base for pull requests so everyone knows what they should be doing as it's not really documented anywhere. :) Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] xfs: Delay Ready Attributes 2021-06-09 22:03 ` Dave Chinner @ 2021-06-10 0:28 ` Darrick J. Wong 2021-06-10 1:49 ` Dave Chinner 2021-06-10 7:20 ` Allison Henderson 0 siblings, 2 replies; 6+ messages in thread From: Darrick J. Wong @ 2021-06-10 0:28 UTC (permalink / raw) To: Dave Chinner; +Cc: Allison Henderson, linux-xfs On Thu, Jun 10, 2021 at 08:03:39AM +1000, Dave Chinner wrote: > On Wed, Jun 09, 2021 at 12:44:47PM -0700, Allison Henderson wrote: > > Hi Darrick, > > > > I've created a branch and tag for the delay ready attribute series. I'ved > > added the rvbs since the last review, but otherwise it is unchanged since > > v20. > > > > Please pull from the tag decsribed below. > > Yay! At last! Good work, Allison. :) Yes, indeed. Pulled! > Nothing to worry about here, but I thought I'd make an observation > on the construction branches for pull requests seeing as pull > request are becoming our way of saying "this code is ready to > merge". > > > Thanks! > > Allison > > > > The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2: > > > > xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700) > > This looks like it has been built on top of a specific commit in the > linux-xfs tree - perhaps a for-next branch before all the most > recent development branches have been merged in. Yes, it's the xfs-5.13-fixes-3 tag at the end of the 5.13 fixes branch. > The problem with doing this is that the for-next tree can rebase, > which can leave your tree with orphaned commits that are no longer > in the main development branch or upstream. While these commits > are upstream now, this isn't an issue for this particular branch > and pull request. > > i.e. if the recent rebase of the for-next branch rewrote the above > commit, pulling this branch would cause all sorts of problems. > > So to avoid this sort of issue with pull requests, and to allow the > maintainer (darrick) to be able to easily reformulate the for-next > tree just by merging branches, pull requests should all come from a > known upstream base. In the case of pull requests for the xfs tree, > that known base is the master branch of the XFS tree. This is a good point. Branches should be based off of something that's stable, recent, and relatively close to the current development work. Ideally that would be for-next, but I hadn't actually declared that stable yet since I just started accepting pull requests and wanted a couple of days to make sure nothing totally weird happened with Stephen Rothwell's integration merge. With for-next in flux, basing your branch off the end of the fixes branch, or an upstream Linus release some time after that, are good enough choices... since I hadn't updated xfs-linux.git#master in a while. For the past 4.5 years, the pattern has always been that the most recent fixes branch (xfs-5.X-fixes) gets merged into upstream before I create the xfs-5.(X+1)-merge branch. This could get murky if I ever have enough bandwidth to be building a fixes branch and a merge branch at the same time, but TBH if xfs is so unstable that we /need/ fixes past -rc4 then we really should concentrate on that at the expense of merging new code. I guess that means I should be updating xfs-linux.git#master to point to the most recent -rc with any Xfs changes in it. > The only time that you wouldn't do this is when your work is > dependent on some other set of fixes. Those fixes then need to be > in a stable upstream branch somewhere, which you then merge into > your own dev branch based on xfs/master and the put your own commits > on top of. IOWs, you start your own branch with a merge commit... > > If you do this, you should note in the pull request that there are > other branches merged into this pull request and where they came > from. THat way the maintainer can determine if the branch is > actually stable and will end up being merged upstream unchanged from > it's current state. > > It is also nice to tell the maintainer that you've based the branch > on a stable XFS commit ahead of the current master branch. This > might be necessary because there's a dependency between multiple > development branches that are being merged one at a time in seperate > pull requests. Agreed. > > In terms of workflow, what this means is that development is done on > a xfs/master based branch. i.e. dev branches are built like this: > > $ git checkout -b dev-branch-1 xfs/master > $ git merge remote/stable-dev-branch > $ git merge local-dependent-dev-branch > $ <apply all your changes> > $ <build kernel and test> > > And for testing against the latest -rc (say 5.13-rc5) and for-next > kernels you do: > > $ git checkout -b testing v5.13-rc5 > $ git merge xfs/for-next > $ git merge dev-branch-1 > <resolve conflicts> > $ git merge dev-branch-2 > <resolve conflicts> > .... > $ git merge dev-branch-N > <resolve conflicts> > $ <build kernel and test> Whee, the modern era... :) > This means that each dev branch has all the correct dependencies > built into it, and they can be pulled by anyone without perturbing > their local tree for testing and review because they are all working > on the same xfs/master branch as your branches are. > > This also means that the xfs/for-next tree can remain based on > xfs/master and be reformulated against xfs/master in a repeatable > manner. It just makes everything easier if all pull requests are > sent from the same stable base commit... > > Anyway, this isn't an issue for this pull-req because it is based on > a stable XFS commit in a branch based on xfs/master, but I thought > it's worth pointing out the pitfalls of using random stable commits > as the base for pull requests so everyone knows what they should be > doing as it's not really documented anywhere. :) Agreed, though this isn't entirely a "random stable commit", it's the end of the most recent stable branch. --D > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] xfs: Delay Ready Attributes 2021-06-10 0:28 ` Darrick J. Wong @ 2021-06-10 1:49 ` Dave Chinner 2021-06-13 0:36 ` Darrick J. Wong 2021-06-10 7:20 ` Allison Henderson 1 sibling, 1 reply; 6+ messages in thread From: Dave Chinner @ 2021-06-10 1:49 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Allison Henderson, linux-xfs On Wed, Jun 09, 2021 at 05:28:06PM -0700, Darrick J. Wong wrote: > On Thu, Jun 10, 2021 at 08:03:39AM +1000, Dave Chinner wrote: > > On Wed, Jun 09, 2021 at 12:44:47PM -0700, Allison Henderson wrote: > > > Hi Darrick, > > > > > > I've created a branch and tag for the delay ready attribute series. I'ved > > > added the rvbs since the last review, but otherwise it is unchanged since > > > v20. > > > > > > Please pull from the tag decsribed below. > > > > Yay! At last! Good work, Allison. :) > > Yes, indeed. Pulled! > > > Nothing to worry about here, but I thought I'd make an observation > > on the construction branches for pull requests seeing as pull > > request are becoming our way of saying "this code is ready to > > merge". > > > > > Thanks! > > > Allison > > > > > > The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2: > > > > > > xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700) > > > > This looks like it has been built on top of a specific commit in the > > linux-xfs tree - perhaps a for-next branch before all the most > > recent development branches have been merged in. > > Yes, it's the xfs-5.13-fixes-3 tag at the end of the 5.13 fixes branch. > > > The problem with doing this is that the for-next tree can rebase, > > which can leave your tree with orphaned commits that are no longer > > in the main development branch or upstream. While these commits > > are upstream now, this isn't an issue for this particular branch > > and pull request. > > > > i.e. if the recent rebase of the for-next branch rewrote the above > > commit, pulling this branch would cause all sorts of problems. > > > > So to avoid this sort of issue with pull requests, and to allow the > > maintainer (darrick) to be able to easily reformulate the for-next > > tree just by merging branches, pull requests should all come from a > > known upstream base. In the case of pull requests for the xfs tree, > > that known base is the master branch of the XFS tree. > > This is a good point. Branches should be based off of something that's > stable, recent, and relatively close to the current development work. > > Ideally that would be for-next, but I hadn't actually declared that > stable yet since I just started accepting pull requests and wanted a > couple of days to make sure nothing totally weird happened with Stephen > Rothwell's integration merge. I don't think you should ever declare for-next as a "stable" branch to base new work on, for the same reason that linux-next isn't stable. i.e. for-next is a "work in progress" branch that, eventually, becomes the branch that is merged upstream. It's not actually something developers can rely on being "stable" until it is tagged for upstream merge and a pull-request issued. Really, for-next is the intergration branch - it's first place where we get wider coverage of the development code via linux-next. There is -always- a chance that someone using linux-next is going to find a problem that requires a rebase of the for-next branch to fix, and so the for-next branch really isn't "stable". If we need public stable branches for cross-dev development, they need to be published in their own branch as an explicitly stable branch. That stable branch can then be merged into dev trees and for-next like any other stable branch and when those dev branches are merged into the one tree from multiple sources git will recognise this and merge them cleanly. IOWs, for-next is for shaking out problems found during integration and wider testing, not for commit stability.... > With for-next in flux, basing your branch off the end of the fixes > branch, or an upstream Linus release some time after that, are good > enough choices... The base of topic branch should be a Linus release at or before (older) the base commit the xfs fixes branch is based off. Otherwise when the topic branch is merged into the for-next tree, it will also pull all the commits from the upstream branch that the for-next tree doesn't have. IOWs, if the XFS fixes branch is based on 5.13-rc2, then all pull requests need to be based on either the fixes branch or at or something earlier than xfs/master. If you base the pull req on 5.13-rc3, then merging the topic branch into for-next will also move for-next forward to 5.13-rc3. This makes a mess of the tree history, and Linus will complain about the mess when he sees it... > since I hadn't updated xfs-linux.git#master in a > while. In case I haven't already made it clear, I think the master branch should only get updated once per release cycle, some time shortly after the merge window closes. Ideally this happens around -rc2 so the base is somewhat stable. /me does a double take When did xfs/master move forward to 5.13-rc4? > For the past 4.5 years, the pattern has always been that the most recent > fixes branch (xfs-5.X-fixes) gets merged into upstream before I create > the xfs-5.(X+1)-merge branch. This could get murky if I ever have > enough bandwidth to be building a fixes branch and a merge branch at the > same time, but TBH if xfs is so unstable that we /need/ fixes past -rc4 > then we really should concentrate on that at the expense of merging new > code. > > I guess that means I should be updating xfs-linux.git#master to point to > the most recent -rc with any Xfs changes in it. IMO, it should not change at all during a cycle. Having to update the -fixes branch late in the cycle does not need rebasing the master branch or the fixes branch. It just means it needs to be merged into the for-next branch again after the new fix is appended to it. Moving the master branch forwards to pick up all the fixes just leads to downstream problems when people are managing multiple branches locally. i.e. master branch stability isn't about what is convenient for building for-next, but about providing a stable base for all developers that are using the tree to build pull requests. IOWs, I do not want to be using -rc2 for one set of topic branches, and then another branch I set up for merge a week later to be based on -rc4 even though I've used xfs/master as the base for all of them. That results in lots of local merge noise and difficultly in getting diffs between branches. This also causes problems with explicitly stable branches if they end up being based off different commits to the rest of the trees. So unless there's a compelling reason, I don't think the master branch should move more than once per dev cycle. Everyone should be using the same base for their pull requests so they can pull each other's work without getting base kernel revision noise in the merges... > > In terms of workflow, what this means is that development is done on > > a xfs/master based branch. i.e. dev branches are built like this: > > > > $ git checkout -b dev-branch-1 xfs/master > > $ git merge remote/stable-dev-branch > > $ git merge local-dependent-dev-branch > > $ <apply all your changes> > > $ <build kernel and test> > > > > And for testing against the latest -rc (say 5.13-rc5) and for-next > > kernels you do: > > > > $ git checkout -b testing v5.13-rc5 > > $ git merge xfs/for-next > > $ git merge dev-branch-1 > > <resolve conflicts> > > $ git merge dev-branch-2 > > <resolve conflicts> > > .... > > $ git merge dev-branch-N > > <resolve conflicts> > > $ <build kernel and test> > > Whee, the modern era... :) Hardly - that's how I've worked for over a decade :P (Get Off My Lawn!) > > Anyway, this isn't an issue for this pull-req because it is based on > > a stable XFS commit in a branch based on xfs/master, but I thought > > it's worth pointing out the pitfalls of using random stable commits > > as the base for pull requests so everyone knows what they should be > > doing as it's not really documented anywhere. :) > > Agreed, though this isn't entirely a "random stable commit", it's the > end of the most recent stable branch. Right, but my point is that where that commit comes from isn't obvious just by looking at the pull request. I mean, can you tell me what branch/base kernel is the pull request based on just from the commit ID and name? If you look at the pull-reqs I sent you, they say: The following changes since commit d07f6ca923ea0927a1024dfccafc5b53b61cfecc: Linux 5.13-rc2 (2021-05-16 15:27:44 -0700) are available in the Git repository at: .... And that is immediately obvious what the pull-req is based on. That's what xfs/master /was/ based on when I built the topic branches. Using a common, known base for building branches makes things easier for everyone.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] xfs: Delay Ready Attributes 2021-06-10 1:49 ` Dave Chinner @ 2021-06-13 0:36 ` Darrick J. Wong 0 siblings, 0 replies; 6+ messages in thread From: Darrick J. Wong @ 2021-06-13 0:36 UTC (permalink / raw) To: Dave Chinner; +Cc: Allison Henderson, linux-xfs On Thu, Jun 10, 2021 at 11:49:53AM +1000, Dave Chinner wrote: > On Wed, Jun 09, 2021 at 05:28:06PM -0700, Darrick J. Wong wrote: > > On Thu, Jun 10, 2021 at 08:03:39AM +1000, Dave Chinner wrote: > > > On Wed, Jun 09, 2021 at 12:44:47PM -0700, Allison Henderson wrote: > > > > Hi Darrick, > > > > > > > > I've created a branch and tag for the delay ready attribute series. I'ved > > > > added the rvbs since the last review, but otherwise it is unchanged since > > > > v20. > > > > > > > > Please pull from the tag decsribed below. > > > > > > Yay! At last! Good work, Allison. :) > > > > Yes, indeed. Pulled! > > > > > Nothing to worry about here, but I thought I'd make an observation > > > on the construction branches for pull requests seeing as pull > > > request are becoming our way of saying "this code is ready to > > > merge". > > > > > > > Thanks! > > > > Allison > > > > > > > > The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2: > > > > > > > > xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700) > > > > > > This looks like it has been built on top of a specific commit in the > > > linux-xfs tree - perhaps a for-next branch before all the most > > > recent development branches have been merged in. > > > > Yes, it's the xfs-5.13-fixes-3 tag at the end of the 5.13 fixes branch. > > > > > The problem with doing this is that the for-next tree can rebase, > > > which can leave your tree with orphaned commits that are no longer > > > in the main development branch or upstream. While these commits > > > are upstream now, this isn't an issue for this particular branch > > > and pull request. > > > > > > i.e. if the recent rebase of the for-next branch rewrote the above > > > commit, pulling this branch would cause all sorts of problems. > > > > > > So to avoid this sort of issue with pull requests, and to allow the > > > maintainer (darrick) to be able to easily reformulate the for-next > > > tree just by merging branches, pull requests should all come from a > > > known upstream base. In the case of pull requests for the xfs tree, > > > that known base is the master branch of the XFS tree. > > > > This is a good point. Branches should be based off of something that's > > stable, recent, and relatively close to the current development work. > > > > Ideally that would be for-next, but I hadn't actually declared that > > stable yet since I just started accepting pull requests and wanted a > > couple of days to make sure nothing totally weird happened with Stephen > > Rothwell's integration merge. > > I don't think you should ever declare for-next as a "stable" branch > to base new work on, for the same reason that linux-next isn't > stable. i.e. for-next is a "work in progress" branch that, eventually, > becomes the branch that is merged upstream. It's not actually > something developers can rely on being "stable" until it is tagged > for upstream merge and a pull-request issued. Oh! Well that clears up a misconception then! For the whole time I've been maintainer, I'd assumed (and worried about) that for-next was not to be rewritten under any circumstances once pushed and announced, unless a commit was found to be so bad that rewriting history was a better option. "Tagged and pull requested" is a lower and easier standard. Good. > Really, for-next is the intergration branch - it's first place where > we get wider coverage of the development code via linux-next. There > is -always- a chance that someone using linux-next is going to find > a problem that requires a rebase of the for-next branch to fix, and > so the for-next branch really isn't "stable". > > If we need public stable branches for cross-dev development, they > need to be published in their own branch as an explicitly stable > branch. That stable branch can then be merged into dev trees and > for-next like any other stable branch and when those dev branches > are merged into the one tree from multiple sources git will > recognise this and merge them cleanly. <nod> That I can do. > IOWs, for-next is for shaking out problems found during integration > and wider testing, not for commit stability.... > > > With for-next in flux, basing your branch off the end of the fixes > > branch, or an upstream Linus release some time after that, are good > > enough choices... > > The base of topic branch should be a Linus release at or before > (older) the base commit the xfs fixes branch is based off. > Otherwise when the topic branch is merged into the for-next tree, it > will also pull all the commits from the upstream branch that the > for-next tree doesn't have. > > IOWs, if the XFS fixes branch is based on 5.13-rc2, then all pull > requests need to be based on either the fixes branch or at or > something earlier than xfs/master. If you base the pull req on > 5.13-rc3, then merging the topic branch into for-next will also move > for-next forward to 5.13-rc3. This makes a mess of the tree history, > and Linus will complain about the mess when he sees it... > > > since I hadn't updated xfs-linux.git#master in a > > while. > > In case I haven't already made it clear, I think the master branch > should only get updated once per release cycle, some time shortly > after the merge window closes. Ideally this happens around -rc2 > so the base is somewhat stable. > > /me does a double take > > When did xfs/master move forward to 5.13-rc4? After Linus merged the last of the xfs-5.13-fixes branch. I can never really settle my mind as to whether to keep master on -rc1 or push it forward. Leaving it at -rc1 is less churn, but OTOH there are (rather frequently) bugs in other parts of the kernel that I hear about breaking peoples' (or my own) ability to test, so I push it forward when things seem to have stabilized. (or at least, every other kernel since about 5.5 has had /some/ stupid problem in the mm/block/networking code that has caused me personally to see a higher rate of random test regressions, which means I've now internalized pushing master to -rc4 because it takes a month for that to shake out) > > For the past 4.5 years, the pattern has always been that the most recent > > fixes branch (xfs-5.X-fixes) gets merged into upstream before I create > > the xfs-5.(X+1)-merge branch. This could get murky if I ever have > > enough bandwidth to be building a fixes branch and a merge branch at the > > same time, but TBH if xfs is so unstable that we /need/ fixes past -rc4 > > then we really should concentrate on that at the expense of merging new > > code. > > > > I guess that means I should be updating xfs-linux.git#master to point to > > the most recent -rc with any Xfs changes in it. > > IMO, it should not change at all during a cycle. Having to update > the -fixes branch late in the cycle does not need rebasing the > master branch or the fixes branch. It just means it needs to be > merged into the for-next branch again after the new fix is appended > to it. > > Moving the master branch forwards to pick up all the fixes just > leads to downstream problems when people are managing multiple > branches locally. i.e. master branch stability isn't about what is > convenient for building for-next, but about providing a stable base > for all developers that are using the tree to build pull requests. > > IOWs, I do not want to be using -rc2 for one set of topic branches, > and then another branch I set up for merge a week later to be based > on -rc4 even though I've used xfs/master as the base for all of > them. That results in lots of local merge noise and difficultly in > getting diffs between branches. This also causes problems with > explicitly stable branches if they end up being based off different > commits to the rest of the trees. > > So unless there's a compelling reason, I don't think the master > branch should move more than once per dev cycle. Everyone should be > using the same base for their pull requests so they can pull each > other's work without getting base kernel revision noise in the > merges... Fair enough. Next cycle, I'll update master when I put out the first -fixes branch and leave it there all the way through. > > > In terms of workflow, what this means is that development is done on > > > a xfs/master based branch. i.e. dev branches are built like this: > > > > > > $ git checkout -b dev-branch-1 xfs/master > > > $ git merge remote/stable-dev-branch > > > $ git merge local-dependent-dev-branch > > > $ <apply all your changes> > > > $ <build kernel and test> > > > > > > And for testing against the latest -rc (say 5.13-rc5) and for-next > > > kernels you do: > > > > > > $ git checkout -b testing v5.13-rc5 > > > $ git merge xfs/for-next > > > $ git merge dev-branch-1 > > > <resolve conflicts> > > > $ git merge dev-branch-2 > > > <resolve conflicts> > > > .... > > > $ git merge dev-branch-N > > > <resolve conflicts> > > > $ <build kernel and test> > > > > Whee, the modern era... :) > > Hardly - that's how I've worked for over a decade :P > > (Get Off My Lawn!) (Heh, me too, but messily with stgit) > > > Anyway, this isn't an issue for this pull-req because it is based on > > > a stable XFS commit in a branch based on xfs/master, but I thought > > > it's worth pointing out the pitfalls of using random stable commits > > > as the base for pull requests so everyone knows what they should be > > > doing as it's not really documented anywhere. :) > > > > Agreed, though this isn't entirely a "random stable commit", it's the > > end of the most recent stable branch. > > Right, but my point is that where that commit comes from isn't > obvious just by looking at the pull request. I mean, can you tell me > what branch/base kernel is the pull request based on just from the > commit ID and name? > > If you look at the pull-reqs I sent you, they say: > > The following changes since commit d07f6ca923ea0927a1024dfccafc5b53b61cfecc: > > Linux 5.13-rc2 (2021-05-16 15:27:44 -0700) > > are available in the Git repository at: > .... > > And that is immediately obvious what the pull-req is based on. > That's what xfs/master /was/ based on when I built the topic > branches. Using a common, known base for building branches makes > things easier for everyone.... <nod> --D > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL] xfs: Delay Ready Attributes 2021-06-10 0:28 ` Darrick J. Wong 2021-06-10 1:49 ` Dave Chinner @ 2021-06-10 7:20 ` Allison Henderson 1 sibling, 0 replies; 6+ messages in thread From: Allison Henderson @ 2021-06-10 7:20 UTC (permalink / raw) To: Darrick J. Wong, Dave Chinner; +Cc: linux-xfs On 6/9/21 5:28 PM, Darrick J. Wong wrote: > On Thu, Jun 10, 2021 at 08:03:39AM +1000, Dave Chinner wrote: >> On Wed, Jun 09, 2021 at 12:44:47PM -0700, Allison Henderson wrote: >>> Hi Darrick, >>> >>> I've created a branch and tag for the delay ready attribute series. I'ved >>> added the rvbs since the last review, but otherwise it is unchanged since >>> v20. >>> >>> Please pull from the tag decsribed below. >> >> Yay! At last! Good work, Allison. :) > > Yes, indeed. Pulled! Great! Very exciting, I think this chunk was probably the more complex of the sub series for parent pointers. > >> Nothing to worry about here, but I thought I'd make an observation >> on the construction branches for pull requests seeing as pull >> request are becoming our way of saying "this code is ready to >> merge". >> >>> Thanks! >>> Allison >>> >>> The following changes since commit 0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2: >>> >>> xfs: bunmapi has unnecessary AG lock ordering issues (2021-05-27 08:11:24 -0700) >> >> This looks like it has been built on top of a specific commit in the >> linux-xfs tree - perhaps a for-next branch before all the most >> recent development branches have been merged in. > > Yes, it's the xfs-5.13-fixes-3 tag at the end of the 5.13 fixes branch. Yes, i try to stay on top of the announcements when i see them > >> The problem with doing this is that the for-next tree can rebase, >> which can leave your tree with orphaned commits that are no longer >> in the main development branch or upstream. While these commits >> are upstream now, this isn't an issue for this particular branch >> and pull request. >> >> i.e. if the recent rebase of the for-next branch rewrote the above >> commit, pulling this branch would cause all sorts of problems. >> >> So to avoid this sort of issue with pull requests, and to allow the >> maintainer (darrick) to be able to easily reformulate the for-next >> tree just by merging branches, pull requests should all come from a >> known upstream base. In the case of pull requests for the xfs tree, >> that known base is the master branch of the XFS tree. > > This is a good point. Branches should be based off of something that's > stable, recent, and relatively close to the current development work. > > Ideally that would be for-next, but I hadn't actually declared that > stable yet since I just started accepting pull requests and wanted a > couple of days to make sure nothing totally weird happened with Stephen > Rothwell's integration merge. > > With for-next in flux, basing your branch off the end of the fixes > branch, or an upstream Linus release some time after that, are good > enough choices... since I hadn't updated xfs-linux.git#master in a > while. > > For the past 4.5 years, the pattern has always been that the most recent > fixes branch (xfs-5.X-fixes) gets merged into upstream before I create > the xfs-5.(X+1)-merge branch. This could get murky if I ever have > enough bandwidth to be building a fixes branch and a merge branch at the > same time, but TBH if xfs is so unstable that we /need/ fixes past -rc4 > then we really should concentrate on that at the expense of merging new > code. > > I guess that means I should be updating xfs-linux.git#master to point to > the most recent -rc with any Xfs changes in it. > >> The only time that you wouldn't do this is when your work is >> dependent on some other set of fixes. Those fixes then need to be >> in a stable upstream branch somewhere, which you then merge into >> your own dev branch based on xfs/master and the put your own commits >> on top of. IOWs, you start your own branch with a merge commit... >> >> If you do this, you should note in the pull request that there are >> other branches merged into this pull request and where they came >> from. THat way the maintainer can determine if the branch is >> actually stable and will end up being merged upstream unchanged from >> it's current state. >> >> It is also nice to tell the maintainer that you've based the branch >> on a stable XFS commit ahead of the current master branch. This >> might be necessary because there's a dependency between multiple >> development branches that are being merged one at a time in seperate >> pull requests. > > Agreed. > Got it. Will do, I do sort of run into that from time to time. >> >> In terms of workflow, what this means is that development is done on >> a xfs/master based branch. i.e. dev branches are built like this: >> >> $ git checkout -b dev-branch-1 xfs/master >> $ git merge remote/stable-dev-branch >> $ git merge local-dependent-dev-branch >> $ <apply all your changes> >> $ <build kernel and test> >> >> And for testing against the latest -rc (say 5.13-rc5) and for-next >> kernels you do: >> >> $ git checkout -b testing v5.13-rc5 >> $ git merge xfs/for-next >> $ git merge dev-branch-1 >> <resolve conflicts> >> $ git merge dev-branch-2 >> <resolve conflicts> >> .... >> $ git merge dev-branch-N >> <resolve conflicts> >> $ <build kernel and test> > > Whee, the modern era... :) Sure, I will hang on to these instructions then > >> This means that each dev branch has all the correct dependencies >> built into it, and they can be pulled by anyone without perturbing >> their local tree for testing and review because they are all working >> on the same xfs/master branch as your branches are. >> >> This also means that the xfs/for-next tree can remain based on >> xfs/master and be reformulated against xfs/master in a repeatable >> manner. It just makes everything easier if all pull requests are >> sent from the same stable base commit... >> >> Anyway, this isn't an issue for this pull-req because it is based on >> a stable XFS commit in a branch based on xfs/master, but I thought >> it's worth pointing out the pitfalls of using random stable commits >> as the base for pull requests so everyone knows what they should be >> doing as it's not really documented anywhere. :) > > Agreed, though this isn't entirely a "random stable commit", it's the > end of the most recent stable branch. Right, i try to stay in top of the latest branch, but this way makes sense too. I think it's good to establish a common procedure for people to use so that everyone knows what to expect. Thanks for the examples! Allison > > --D > >> >> Cheers, >> >> Dave. >> -- >> Dave Chinner >> david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-13 0:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-09 19:44 [GIT PULL] xfs: Delay Ready Attributes Allison Henderson 2021-06-09 22:03 ` Dave Chinner 2021-06-10 0:28 ` Darrick J. Wong 2021-06-10 1:49 ` Dave Chinner 2021-06-13 0:36 ` Darrick J. Wong 2021-06-10 7:20 ` Allison Henderson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.