Message ID | 20180813161357.GB1199@bombadil.infradead.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [GIT,PULL] XArray for 4.19 | expand |
On Mon, Aug 13, 2018 at 9:14 AM Matthew Wilcox <willy@infradead.org> wrote: > > Please consider pulling the XArray patch set. So this merge window has been horrible, but I was just about to start looking at it. And no. I'm not going to pull this. For some unfathomable reason, you have based it on the libnvdimm tree. I don't understand at all wjhy you did that. That libnvdimm tree didn't get merged., because it had complete garbage in the mm/ code. And yes, that buggy shit was what you based the radix tree code on. I seriously have no idea why you have based it on some unstable random tree in the first place. But basing it on something that I independently refused to pull because of obvious bugs from just a quick scan - that completely invalidates this pull request. Why? I guess it makes this merge window easier, since now I don't even have to look at the code, but it annoys the hell out of me when things like that happen. There wasn't even a mention in the pull request about how this was all based on some libnvdimm code that hadn't been merged yet. But you must have known that, since you must have explicitly done the pull request not against my tree, but against the bogus base branch. And since I won't be merging this, I clearly won't be merging your other pull request that depended on this either. Why the f*ck were these features so interlinked to begin with? Linus
On Tue, Aug 21, 2018 at 07:09:31PM -0700, Linus Torvalds wrote: > On Mon, Aug 13, 2018 at 9:14 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > Please consider pulling the XArray patch set. > > So this merge window has been horrible, but I was just about to start > looking at it. > > And no. I'm not going to pull this. > > For some unfathomable reason, you have based it on the libnvdimm tree. > I don't understand at all wjhy you did that. I said in the pull request ... There are two conflicts I wanted to flag; the first is against the linux-nvdimm tree. I rebased on top of one of the branches that went into that tree, so if you pull my tree before linux-nvdimm, you'll get fifteen commits I've had no involvement with. Dan asked me to do that so that his commit (which I had no involvement with) would be easier to backport. At the time I thought this was a reasonable request; I know this API change is disruptive and I wanted to accommodate that. I didn't know his patch was "complete garbage"; I didn't review it. So, should I have based just on your tree and sent you a description of what a resolved conflict should look like? > And since I won't be merging this, I clearly won't be merging your > other pull request that depended on this either. I can yank most of the patches (all but the last two, iirc) out of the IDA patchset and submit those as a separate pull request. Would that be acceptable? I'm really struggling to juggle all the pieces here to get them merged.
On Tue, Aug 21, 2018 at 7:50 PM Matthew Wilcox <willy@infradead.org> wrote: > > So, should I have based just on your tree and sent you a description of > what a resolved conflict should look like? Absolutely. Or preferably not rebasing at all, just starting from a good solid base for new development in the first place. Sometimes you start from the wrong point, and decide that you really need to rebase, but then you should rebase to a more stable point, not on top of some random independent development. Rebasing can be a really good tool to clean up development that was haphazard - maybe as you go along you notice that something you did earlier turned out to be counter-productive, so you rebase and clean up your history that has not been made public yet. But when you send me a big new feature, the absolutely *last* thing I want to ever see is to see it based on some random unstable base. And rebasing to avoid merge conflicts is *always* the wrong thing to do, unless the reason you're rebasing is "hey, I wrote this feature ages ago, I need to really refresh it to a more modern and stable kernel, so I'll rebase it onto the current last release instead, so that I have a good starting point". And even then the basic reason is not so much that there were conflicts, but that you just want your series to make more sense on its own, and not have one horribly complex merge that is just due to the fact that it was based on something ancient. The absolute last thing I want to see during the merge window is multiple independent features that have been tied together just because they are rebased on top of each other. Because that means - as in this case - that if one branch has problems, it now affects all of them. Merge conflicts aren't bad. In 99% of all cases, the conflict is trivial to solve. And the cost of trying to deal with them with rebasing is much much higher. Linus
On Mon, 13 Aug 2018, Matthew Wilcox wrote: > Please consider pulling the XArray patch set. The XArray provides an > improved interface to the radix tree data structure, providing locking > as part of the API, specifying GFP flags at allocation time, eliminating > preloading, less re-walking the tree, more efficient iterations and not > exposing RCU-protected pointers to its users. Is this going in this cycle? I have a bunch of stuff on top of this to enable slab object migration.
On Wed, Aug 22, 2018 at 10:40 AM Christopher Lameter <cl@linux.com> wrote: > > Is this going in this cycle? I have a bunch of stuff on top of this to > enable slab object migration. No. It was based on a buggy branch that isn't getting pulled, so when I started looking at it, the pull request was rejected before I got much further. Linus
On Wed, Aug 22, 2018 at 10:43 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Aug 22, 2018 at 10:40 AM Christopher Lameter <cl@linux.com> wrote: > > > > Is this going in this cycle? I have a bunch of stuff on top of this to > > enable slab object migration. > > No. > > It was based on a buggy branch that isn't getting pulled To be clear, I don't think the problem you identified can be triggered in practice. We are under the equivalent of the page lock for dax in that path, and if ->mapping is NULL we would bail before finding that the mapping-size helper returns zero. > so when I > started looking at it, the pull request was rejected before I got much > further. For the record I think skipping the entirety of the libnvdimm pull request for this cycle due to that misuse of ilog2() is overkill, but it's not my kernel. Andrew, I think this means we need to lean on you to merge dax-memory-failure and Xarray for 4.20 rather than try to coordinate our own git branches for these specific topics. At a minimum for 4.19 I think we should disable MADV_HWPOISON for dax mappings this cycle to at least close that trivial method to crash the kernel when using dax. Dave, I recommend dropping dax-memory-failure and sending the other libnvdimm topics for 4.19 that have been soaking in -next.
On Tue, Aug 21, 2018 at 08:00:18PM -0700, Linus Torvalds wrote: > On Tue, Aug 21, 2018 at 7:50 PM Matthew Wilcox <willy@infradead.org> wrote: > > So, should I have based just on your tree and sent you a description of > > what a resolved conflict should look like? > > Absolutely. > > Or preferably not rebasing at all, just starting from a good solid > base for new development in the first place. Ah, I remember now, it was more complex than a textual conflict. Dan added an entirely new function here: http://git.infradead.org/users/willy/linux-dax.git/commitdiff/c2a7d2a115525d3501d38e23d24875a79a07e15e which needed to be converted to XArray. So I should have pulled in his branch as a merge somewhere close to the end of my series, then done a fresh patch on top of that to convert it? It would have been pretty ugly because he modified a function I deleted. I might try it out just to show how bad it would have been.
On Wed, Aug 22, 2018 at 11:23 AM Matthew Wilcox <willy@infradead.org> wrote: > > Dan added an entirely new function here: > > http://git.infradead.org/users/willy/linux-dax.git/commitdiff/c2a7d2a115525d3501d38e23d24875a79a07e15e > > which needed to be converted to XArray. So I should have pulled in his > branch as a merge somewhere close to the end of my series, then done a > fresh patch on top of that to convert it? No, it doesn't matter if you rebase on top of a broken branch, or you merge it in. Either way, it's broken and I can't merge your end result. You should simply NOT CARE about other branches. Particularly not other branches that might not even get merged in the first place! You should care about *YOUR* work. You should make sure *your* work is rock solid, and that it is based on a rock solid base. Not some random shifting quick-sand of somebody elses development branch. Sure, then linux-next will give a merge conflict, but at that point YOU DO NOT REBASE OR MERGE. You tell linux-next how to merge it, and mention it to me in the pull request. Because at that point, I have the *choice* of merging just one of the branches or both. Or I can merge them in either order, and test them independently? See how that is fundamentally different from you tying the two branches together and handing me a fait accompli? Yes, yes, sometimes you have to tie branches together because one branch fundamentally depends on the features the other branch offers. But that should be avoided like a plague if at all possible. And it damn well isn't an issue for something like xarray, which has a life entirely independently of libnvdimm (and vice versa), and the conflict was just random happenstance, not some "my changes fundamentally rely on the new features you provide". Linus
Hi Linus, On Tue, 21 Aug 2018 19:09:31 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > For some unfathomable reason, you have based it on the libnvdimm tree. > I don't understand at all wjhy you did that. That was partly my fault for giving not very good advice when the (quite complex) merge conflict turned up in the linux-next tree. I will try to give better advice in the future.
On 08/22/2018 07:40 PM, Christopher Lameter wrote: > On Mon, 13 Aug 2018, Matthew Wilcox wrote: > >> Please consider pulling the XArray patch set. The XArray provides an >> improved interface to the radix tree data structure, providing locking >> as part of the API, specifying GFP flags at allocation time, eliminating >> preloading, less re-walking the tree, more efficient iterations and not >> exposing RCU-protected pointers to its users. > > Is this going in this cycle? I have a bunch of stuff on top of this to > enable slab object migration. I think you can just post those for review and say that they apply on top of xarray git? Maybe also with your own git URL with those applied for easier access? I'm curious but also sceptical that something so major would get picked up to mmotm immediately :)
On Fri, 24 Aug 2018, Vlastimil Babka wrote: > > I think you can just post those for review and say that they apply on > top of xarray git? Maybe also with your own git URL with those applied > for easier access? I'm curious but also sceptical that something so > major would get picked up to mmotm immediately :) > I posted it awhile ago and was waiting for something definite to diff against so that testing is simple. The last release is at https://www.spinics.net/lists/linux-mm/msg142496.html