Re: [GIT PULL] xfs: Delay Ready Attributes

[Dave Chinner <david@fromorbit.com>]

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