Re: [GIT PULL] xfs: Delay Ready Attributes

["Darrick J. Wong" <djwong@kernel.org>]

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