Re: [GIT PULL] xfs: Delay Ready Attributes

[Allison Henderson <allison.henderson@oracle.com>]

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