[GIT PULL] bcache revert

[GIT PULL] bcache revert

[Kent Overstreet]

Linus, please pull; this reverts a patch from Jens that was committed without
CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
functional change and is something that damn well should have been discussed.

Jens - what the goddamn fuck!? You've never touched the bcache code until now,
and when you finally get interested this is what you do!?

While I am sympathetic to the arguments in favor of your patch, there _are_ some
damn good reasons I did it the way I did. If you want to have that discussion,
feel free to mail your patch out again after the revert.

The following changes since commit 64291f7db5bd8150a74ad2036f1037e6a0428df2:

  Linux 4.2 (2015-08-30 11:34:09 -0700)

are available in the git repository at:

  git://evilpiepirate.org/~kent/linux-bcache.git bcache-revert

for you to fetch changes up to 308826467c4c17e62554603d7d6bbf9164e3948a:

  Revert "bcache: don't embed 'return' statements in closure macros" (2015-08-31 10:50:21 -0800)

----------------------------------------------------------------
Kent Overstreet (1):
      Revert "bcache: don't embed 'return' statements in closure macros"

 drivers/md/bcache/closure.h |  3 +++
 drivers/md/bcache/io.c      |  1 -
 drivers/md/bcache/journal.c |  2 --
 drivers/md/bcache/request.c | 14 +++-----------
 4 files changed, 6 insertions(+), 14 deletions(-)

Re: [GIT PULL] bcache revert

[Jens Axboe]

On 08/31/2015 01:00 PM, Kent Overstreet wrote:
> Linus, please pull; this reverts a patch from Jens that was committed without
> CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
> functional change and is something that damn well should have been discussed.
>
> Jens - what the goddamn fuck!? You've never touched the bcache code until now,
> and when you finally get interested this is what you do!?
>
> While I am sympathetic to the arguments in favor of your patch, there _are_ some
> damn good reasons I did it the way I did. If you want to have that discussion,
> feel free to mail your patch out again after the revert.

The patch was part of a larger series that I was working on, and I just 
wanted to flush out that dependency. Christoph review and acked it, it 
was by no means a sneaking in of a patch.

So calm down. Is there a bug? The previous code was crap, having hidden 
returns in macros is horrible. The upstream bcache code has been 
effectively unmaintained for more than a year, and THIS patch is now a 
problem? Get real.

-- 
Jens Axboe

Re: [GIT PULL] bcache revert

[Kent Overstreet]

On Mon, Aug 31, 2015 at 01:14:07PM -0600, Jens Axboe wrote:
> On 08/31/2015 01:00 PM, Kent Overstreet wrote:
> >Linus, please pull; this reverts a patch from Jens that was committed without
> >CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
> >functional change and is something that damn well should have been discussed.
> >
> >Jens - what the goddamn fuck!? You've never touched the bcache code until now,
> >and when you finally get interested this is what you do!?
> >
> >While I am sympathetic to the arguments in favor of your patch, there _are_ some
> >damn good reasons I did it the way I did. If you want to have that discussion,
> >feel free to mail your patch out again after the revert.
> 
> The patch was part of a larger series that I was working on, and I just
> wanted to flush out that dependency. Christoph review and acked it, it was
> by no means a sneaking in of a patch.

I didn't see it until I went to rebase bcachefs onto 4.2 this morning. I triple
checked; this patch is not in any mailing list archive. And you certainly didn't
try to contact me. How is that _not_ sneaking it in?

> So calm down. Is there a bug? The previous code was crap, having hidden
> returns in macros is horrible. The upstream bcache code has been effectively
> unmaintained for more than a year, and THIS patch is now a problem? Get
> real.

Oh, so you're taking over now? This is the first I've heard of it...

You may say the previous code was crap, but believe it or not I'm not an idiot
and I had real reasons for doing it that way. For damn sure if you want to start
changing stuff like this now it shouldn't be too much to ask that you _mail the
patch out_ so it can be discussed.

Re: [GIT PULL] bcache revert

[Jens Axboe]

On 08/31/2015 01:29 PM, Kent Overstreet wrote:
> On Mon, Aug 31, 2015 at 01:14:07PM -0600, Jens Axboe wrote:
>> On 08/31/2015 01:00 PM, Kent Overstreet wrote:
>>> Linus, please pull; this reverts a patch from Jens that was committed without
>>> CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
>>> functional change and is something that damn well should have been discussed.
>>>
>>> Jens - what the goddamn fuck!? You've never touched the bcache code until now,
>>> and when you finally get interested this is what you do!?
>>>
>>> While I am sympathetic to the arguments in favor of your patch, there _are_ some
>>> damn good reasons I did it the way I did. If you want to have that discussion,
>>> feel free to mail your patch out again after the revert.
>>
>> The patch was part of a larger series that I was working on, and I just
>> wanted to flush out that dependency. Christoph review and acked it, it was
>> by no means a sneaking in of a patch.
>
> I didn't see it until I went to rebase bcachefs onto 4.2 this morning. I triple
> checked; this patch is not in any mailing list archive. And you certainly didn't
> try to contact me. How is that _not_ sneaking it in?

It's a simple cleanup patch, against a dormant driver. It was reviewed 
by Christoph, which is as good as it gets. Yes, it should have been 
posted, but it's not like we are talking about a rewrite or anything of 
that magnitude. You're grossly overreacting. I would do it again.

>> So calm down. Is there a bug? The previous code was crap, having hidden
>> returns in macros is horrible. The upstream bcache code has been effectively
>> unmaintained for more than a year, and THIS patch is now a problem? Get
>> real.
>
> Oh, so you're taking over now? This is the first I've heard of it...

Right, a cleanup patch to make a later series of patches easier, I'm 
surely taking over all of bcache! As mentioned in the previous reply, my 
motivation was to make subsequent patches easier to do. The issue was 
getting rid of the hidden macro return in bcache make_request_fn. That's 
how I stumbled upon this abomination.

> You may say the previous code was crap, but believe it or not I'm not an idiot
> and I had real reasons for doing it that way. For damn sure if you want to start
> changing stuff like this now it shouldn't be too much to ask that you _mail the
> patch out_ so it can be discussed.

We can continue wasting time talking about how you had really good 
reasons for having returns hidden in macros, or you could actually bring 
those reasons to light.

-- 
Jens Axboe

Re: [GIT PULL] bcache revert

[Kent Overstreet]

On Mon, Aug 31, 2015 at 01:42:18PM -0600, Jens Axboe wrote:
> On 08/31/2015 01:29 PM, Kent Overstreet wrote:
> >On Mon, Aug 31, 2015 at 01:14:07PM -0600, Jens Axboe wrote:
> >>On 08/31/2015 01:00 PM, Kent Overstreet wrote:
> >>>Linus, please pull; this reverts a patch from Jens that was committed without
> >>>CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
> >>>functional change and is something that damn well should have been discussed.
> >>>
> >>>Jens - what the goddamn fuck!? You've never touched the bcache code until now,
> >>>and when you finally get interested this is what you do!?
> >>>
> >>>While I am sympathetic to the arguments in favor of your patch, there _are_ some
> >>>damn good reasons I did it the way I did. If you want to have that discussion,
> >>>feel free to mail your patch out again after the revert.
> >>
> >>The patch was part of a larger series that I was working on, and I just
> >>wanted to flush out that dependency. Christoph review and acked it, it was
> >>by no means a sneaking in of a patch.
> >
> >I didn't see it until I went to rebase bcachefs onto 4.2 this morning. I triple
> >checked; this patch is not in any mailing list archive. And you certainly didn't
> >try to contact me. How is that _not_ sneaking it in?
> 
> It's a simple cleanup patch, against a dormant driver. It was reviewed by
> Christoph, which is as good as it gets. Yes, it should have been posted, but
> it's not like we are talking about a rewrite or anything of that magnitude.
> You're grossly overreacting. I would do it again.

Look, you've had your own periods as an unavailable maintainer so I wouldn't
throw stones - and it's no secret that I'm still working on bcache.

Really, as long as you think it's ok to commit patches without CCing the mailing
list _or_ the maintainer, then fuck you. I wouldn't do that to you and I don't
know anyone else who would, so as long as that's your attitude about it there's
really nothing to discuss.

Re: [GIT PULL] bcache revert

[Jens Axboe]

On 08/31/2015 01:53 PM, Kent Overstreet wrote:
> On Mon, Aug 31, 2015 at 01:42:18PM -0600, Jens Axboe wrote:
>> On 08/31/2015 01:29 PM, Kent Overstreet wrote:
>>> On Mon, Aug 31, 2015 at 01:14:07PM -0600, Jens Axboe wrote:
>>>> On 08/31/2015 01:00 PM, Kent Overstreet wrote:
>>>>> Linus, please pull; this reverts a patch from Jens that was committed without
>>>>> CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
>>>>> functional change and is something that damn well should have been discussed.
>>>>>
>>>>> Jens - what the goddamn fuck!? You've never touched the bcache code until now,
>>>>> and when you finally get interested this is what you do!?
>>>>>
>>>>> While I am sympathetic to the arguments in favor of your patch, there _are_ some
>>>>> damn good reasons I did it the way I did. If you want to have that discussion,
>>>>> feel free to mail your patch out again after the revert.
>>>>
>>>> The patch was part of a larger series that I was working on, and I just
>>>> wanted to flush out that dependency. Christoph review and acked it, it was
>>>> by no means a sneaking in of a patch.
>>>
>>> I didn't see it until I went to rebase bcachefs onto 4.2 this morning. I triple
>>> checked; this patch is not in any mailing list archive. And you certainly didn't
>>> try to contact me. How is that _not_ sneaking it in?
>>
>> It's a simple cleanup patch, against a dormant driver. It was reviewed by
>> Christoph, which is as good as it gets. Yes, it should have been posted, but
>> it's not like we are talking about a rewrite or anything of that magnitude.
>> You're grossly overreacting. I would do it again.
>
> Look, you've had your own periods as an unavailable maintainer so I wouldn't
> throw stones - and it's no secret that I'm still working on bcache.

I am not throwing stones, just stating the upstream bcache has been 
dormant for more than a year.

> Really, as long as you think it's ok to commit patches without CCing the mailing
> list _or_ the maintainer, then fuck you. I wouldn't do that to you and I don't
> know anyone else who would, so as long as that's your attitude about it there's
> really nothing to discuss.

I already said that, yes, it should have been posted. But it's not like 
it was unreviewed. Or a massive change, by any stretch.

And we're still not discussing the motives for why it looked like that 
in the first place?

-- 
Jens Axboe

Re: [GIT PULL] bcache revert

[Kent Overstreet]

On Mon, Aug 31, 2015 at 02:06:35PM -0600, Jens Axboe wrote:
> On 08/31/2015 01:53 PM, Kent Overstreet wrote:
> >On Mon, Aug 31, 2015 at 01:42:18PM -0600, Jens Axboe wrote:
> >>On 08/31/2015 01:29 PM, Kent Overstreet wrote:
> >>>On Mon, Aug 31, 2015 at 01:14:07PM -0600, Jens Axboe wrote:
> >>>>On 08/31/2015 01:00 PM, Kent Overstreet wrote:
> >>>>>Linus, please pull; this reverts a patch from Jens that was committed without
> >>>>>CCing be or being mailed out to any of the lists. Said patch wasn't in any way a
> >>>>>functional change and is something that damn well should have been discussed.
> >>>>>
> >>>>>Jens - what the goddamn fuck!? You've never touched the bcache code until now,
> >>>>>and when you finally get interested this is what you do!?
> >>>>>
> >>>>>While I am sympathetic to the arguments in favor of your patch, there _are_ some
> >>>>>damn good reasons I did it the way I did. If you want to have that discussion,
> >>>>>feel free to mail your patch out again after the revert.
> >>>>
> >>>>The patch was part of a larger series that I was working on, and I just
> >>>>wanted to flush out that dependency. Christoph review and acked it, it was
> >>>>by no means a sneaking in of a patch.
> >>>
> >>>I didn't see it until I went to rebase bcachefs onto 4.2 this morning. I triple
> >>>checked; this patch is not in any mailing list archive. And you certainly didn't
> >>>try to contact me. How is that _not_ sneaking it in?
> >>
> >>It's a simple cleanup patch, against a dormant driver. It was reviewed by
> >>Christoph, which is as good as it gets. Yes, it should have been posted, but
> >>it's not like we are talking about a rewrite or anything of that magnitude.
> >>You're grossly overreacting. I would do it again.
> >
> >Look, you've had your own periods as an unavailable maintainer so I wouldn't
> >throw stones - and it's no secret that I'm still working on bcache.
> 
> I am not throwing stones, just stating the upstream bcache has been dormant
> for more than a year.

Dormant, because I was thoroughly overstressed with being the only one doing any
of the work compounded with a shitty startup situation. I've always been
available and responsible to people who were trying to help out. Really, I would
love to have other people working on the bcache code, and I'd be happy to spend
time on the phone or whatever to explain anything I can.

> >Really, as long as you think it's ok to commit patches without CCing the mailing
> >list _or_ the maintainer, then fuck you. I wouldn't do that to you and I don't
> >know anyone else who would, so as long as that's your attitude about it there's
> >really nothing to discuss.
> 
> I already said that, yes, it should have been posted. But it's not like it
> was unreviewed. Or a massive change, by any stretch.

And then you said you'd do it again.

Look, it's about extending a basic courtesy - other people I work with have no
issue with this. Tejun still pings me and lets me know about percpu refcount
changes even though he's taken over as maintainer of that code since almost
after I wrote it. Similarly with most anyone else I've worked with in the kernel
community. I've always put quite a bit of effort into making sure I don't miss
anyone on my CC lists when I was doing work in the block layer that touched all
kinds of code.

Why not you?

> And we're still not discussing the motives for why it looked like that in
> the first place?

Not terribly interested in doing that after the fact, when you've already
bypassed me and gotten your patch in, and you're still saying you'd do it again.
Sorry, I'm not having the discussion on those terms.

Re: [GIT PULL] bcache revert

[Jens Axboe]

On 08/31/2015 02:17 PM, Kent Overstreet wrote:
>>> Really, as long as you think it's ok to commit patches without CCing the mailing
>>> list _or_ the maintainer, then fuck you. I wouldn't do that to you and I don't
>>> know anyone else who would, so as long as that's your attitude about it there's
>>> really nothing to discuss.
>>
>> I already said that, yes, it should have been posted. But it's not like it
>> was unreviewed. Or a massive change, by any stretch.
>
> And then you said you'd do it again.
>
> Look, it's about extending a basic courtesy - other people I work with have no
> issue with this. Tejun still pings me and lets me know about percpu refcount
> changes even though he's taken over as maintainer of that code since almost
> after I wrote it. Similarly with most anyone else I've worked with in the kernel
> community. I've always put quite a bit of effort into making sure I don't miss
> anyone on my CC lists when I was doing work in the block layer that touched all
> kinds of code.
>
> Why not you?

Kent, can we cut down on the victim playing? I said it should have been 
posted, did I not? And usually patches like that ARE always posted, but 
this beat the series of patches that it was a pre-patch for. Hence it 
just didn't get posted, and that was a mistake, after a private 
discussion where it ended up being cherry-picked for inclusion. Even for 
a trivial patch like this. But it's not the end of the world, it's not 
like I rewrote your architecture or grand caching design.

>> And we're still not discussing the motives for why it looked like that in
>> the first place?
>
> Not terribly interested in doing that after the fact, when you've already
> bypassed me and gotten your patch in, and you're still saying you'd do it again.
> Sorry, I'm not having the discussion on those terms.

Grow up. We should revert a patch cleaning up macros with returns in 
them, but you won't really let us in on why?

Unless we can turn this into a REAL (and technical) discussion on why we 
should revert to the old code, I'm done spending time on this thread.

-- 
Jens Axboe

Re: [GIT PULL] bcache revert

[Kent Overstreet]

On Mon, Aug 31, 2015 at 02:25:25PM -0600, Jens Axboe wrote:
> Kent, can we cut down on the victim playing? I said it should have been
> posted, did I not? And usually patches like that ARE always posted, but this
> beat the series of patches that it was a pre-patch for. Hence it just didn't
> get posted, and that was a mistake, after a private discussion where it
> ended up being cherry-picked for inclusion. Even for a trivial patch like
> this. But it's not the end of the world, it's not like I rewrote your
> architecture or grand caching design.

You're backpedalling and trying not to admit it. Look, would you do it again or
not? Because yes of course I'm going to call you out on it if you think this is
an acceptable thing to do, which is certainly what you started off saying.

> Grow up. We should revert a patch cleaning up macros with returns in them,
> but you won't really let us in on why?
> 
> Unless we can turn this into a REAL (and technical) discussion on why we
> should revert to the old code, I'm done spending time on this thread.

Because what's the point of having a technical discussion if you're checking in
code behind my back, and you refuse to say you won't do so again in the future?

And calling it "just a cleanup" is disingenuous. You're making a real semantic
change to the code, which never mind the pros and cons of the patch itself,
means I have now have to rebase ~1000 patches on top of it and it will _silently
break, in a nasty way_ any patches that make use of closures - you just made
a lot of work for me, especially if I want to keep my tree bisectable.

You remember how patches are supposed to go through the maintainer? This is part
of the reason. Are you starting to see why I'm in such a bad mood?

Re: [GIT PULL] bcache revert

[Jens Axboe]

On 08/31/2015 02:42 PM, Kent Overstreet wrote:
> On Mon, Aug 31, 2015 at 02:25:25PM -0600, Jens Axboe wrote:
>> Kent, can we cut down on the victim playing? I said it should have been
>> posted, did I not? And usually patches like that ARE always posted, but this
>> beat the series of patches that it was a pre-patch for. Hence it just didn't
>> get posted, and that was a mistake, after a private discussion where it
>> ended up being cherry-picked for inclusion. Even for a trivial patch like
>> this. But it's not the end of the world, it's not like I rewrote your
>> architecture or grand caching design.
>
> You're backpedalling and trying not to admit it. Look, would you do it again or
> not? Because yes of course I'm going to call you out on it if you think this is
> an acceptable thing to do, which is certainly what you started off saying.

Kent, this is starting to get into playground territory. Should it have 
been posted/cc'ed to you? Yes. Do I think it's a big deal that it 
wasn't, given the nature of the patch? No. Is/was the patch the right 
thing to do? Yes.

>> Grow up. We should revert a patch cleaning up macros with returns in them,
>> but you won't really let us in on why?
>>
>> Unless we can turn this into a REAL (and technical) discussion on why we
>> should revert to the old code, I'm done spending time on this thread.
>
> Because what's the point of having a technical discussion if you're checking in
> code behind my back, and you refuse to say you won't do so again in the future?

Get to the point.

> And calling it "just a cleanup" is disingenuous. You're making a real semantic
> change to the code, which never mind the pros and cons of the patch itself,
> means I have now have to rebase ~1000 patches on top of it and it will _silently
> break, in a nasty way_ any patches that make use of closures - you just made
> a lot of work for me, especially if I want to keep my tree bisectable.
>
> You remember how patches are supposed to go through the maintainer? This is part
> of the reason. Are you starting to see why I'm in such a bad mood?

You still forgot the part where you explained the very good reasons for 
the why the code looked like that.

-- 
Jens Axboe

Re: [GIT PULL] bcache revert

[Kent Overstreet]

On Mon, Aug 31, 2015 at 02:47:45PM -0600, Jens Axboe wrote:
> On 08/31/2015 02:42 PM, Kent Overstreet wrote:
> >On Mon, Aug 31, 2015 at 02:25:25PM -0600, Jens Axboe wrote:
> >>Kent, can we cut down on the victim playing? I said it should have been
> >>posted, did I not? And usually patches like that ARE always posted, but this
> >>beat the series of patches that it was a pre-patch for. Hence it just didn't
> >>get posted, and that was a mistake, after a private discussion where it
> >>ended up being cherry-picked for inclusion. Even for a trivial patch like
> >>this. But it's not the end of the world, it's not like I rewrote your
> >>architecture or grand caching design.
> >
> >You're backpedalling and trying not to admit it. Look, would you do it again or
> >not? Because yes of course I'm going to call you out on it if you think this is
> >an acceptable thing to do, which is certainly what you started off saying.
> 
> Kent, this is starting to get into playground territory. Should it have been
> posted/cc'ed to you? Yes. Do I think it's a big deal that it wasn't, given
> the nature of the patch? No. Is/was the patch the right thing to do? Yes.

If you maintain that this is how you want to do things - I'm out, you can take
over as bcache maintainer.