Bug 11069 - Backport remaining performance patches for vfs_glusterfs to 4.2/4.1
Summary: Backport remaining performance patches for vfs_glusterfs to 4.2/4.1
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2015-01-23 16:18 UTC by Guenther Deschner
Modified: 2015-01-29 20:11 UTC (History)
3 users (show)

See Also:


Attachments
patch for 4.1 (19.78 KB, patch)
2015-01-23 16:46 UTC, Guenther Deschner
ira: review-
Details
patch for 4.2 (30.91 KB, patch)
2015-01-23 16:46 UTC, Guenther Deschner
obnox: review+
ira: review-
Details
new patchset for both 4.2 and 4.1 (including docfix) (32.98 KB, patch)
2015-01-23 23:44 UTC, Guenther Deschner
obnox: review+
ira: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Guenther Deschner 2015-01-23 16:18:03 UTC
Some performance patches reviewed and pushed to master need to be in 4.2 and 4.1.
Comment 1 Guenther Deschner 2015-01-23 16:46:20 UTC
Created attachment 10656 [details]
patch for 4.1
Comment 2 Guenther Deschner 2015-01-23 16:46:44 UTC
Created attachment 10657 [details]
patch for 4.2
Comment 3 Michael Adam 2015-01-23 17:18:36 UTC
Comment on attachment 10657 [details]
patch for 4.2

ACK
Comment 4 Michael Adam 2015-01-23 17:28:27 UTC
Comment on attachment 10656 [details]
patch for 4.1

I'd prefer the patch from 4.2/master beause it keeps the history.
The patch from 4.2 applies cleanly, has the same result as the v4.1 patch and builds cleanly.

I also don't see any advantage in hiding the history.

==> NACK for now.
Comment 5 Ira Cooper 2015-01-23 18:52:11 UTC
The 4.1 patches need to be rewritten for 4.1 mildly anyways, so I don't think that we are really hiding anything.  The squash actually backports much easier.  I favor the squash in this case but I agree with you in general.

Thanks,
Comment 6 Michael Adam 2015-01-23 20:23:53 UTC
(In reply to Ira Cooper from comment #5)

> The 4.1 patches need to be rewritten for 4.1 mildly anyways,

But the patches that you already ACK-ed were not rewritten at all.
They were simply the cherry-picked patches with a few of them
squashed. Each single one applies cleanly though, and it builds
without warnings.

If they need to be rewritten, then the attached patchset must be wrong.
And the reason must be more subtle, and I'd like to see it.

> so I don't think that we are really hiding anything.

Well, we do hide the history.

I think if possible it is preferable to have the histories
of master and release branches as aligned as possible.

> The squash actually backports much easier.

How? There was zero effort back-porting the patches to 4.1.
If we need to create backports for older versions, we
can still squash for those.

> I favor the squash in this case but I agree with you in general.

I still don't see an argument that convinces me.
As I already wrote elsewhere: I can buy it if the back-porting
requires reworking anyways and in particular if the end result
is easier to port than the intermediate steps. But this is not
the case here. (Each patch applies cleanly to 4.1. And it compiles
without warnings.)

It is not that do care extremely, but it is a matter of principle.
I just saw a pattern broken for no reason apparent to me, and I'd like
to understand.

Cheers - Michael
Comment 7 Ira Cooper 2015-01-23 22:15:10 UTC
(In reply to Michael Adam from comment #6)

There is no configure check for HAVE_EVENTFD in 4.1.

The original AIO code is broken on the 4.1 branch.  It may compile, it doesn't work!

I NACK adding broken code ;).
Comment 8 Michael Adam 2015-01-23 22:33:03 UTC
(In reply to Ira Cooper from comment #7)

> There is no configure check for HAVE_EVENTFD in 4.1.

There is no configure check for HAVE_EVENTFD in 4.2 or master either,
and there never was.
(There is one in master in lib/socket_wrapper, but that does not count.)

> The original AIO code is broken on the 4.1 branch.
> It may compile, it doesn't work!

Ok that is information that was not there before.

> I NACK adding broken code ;).

Agreed but I still can't see how 4.1 is different from 4.2 and master in this respect.
Comment 9 Michael Adam 2015-01-23 23:06:57 UTC
Comment on attachment 10656 [details]
patch for 4.1

Withdrawing my NACK. Ira explained on a sidetrack that there were issues with the functionality with the initial patch in 4.1, which is too tedious.
Comment 10 Ira Cooper 2015-01-23 23:43:15 UTC
My apologies, there is an internal to Red Hat version of the 4.1 patchset that does not have the wscript change to add the eventfd change, thus my confusion  We should have the same patches for 4.1 and 4.2.

That said, we should also have my recent doc patch added :).
Comment 11 Guenther Deschner 2015-01-23 23:44:51 UTC
Created attachment 10660 [details]
new patchset for both 4.2 and 4.1 (including docfix)
Comment 12 Michael Adam 2015-01-23 23:55:37 UTC
Comment on attachment 10660 [details]
new patchset for both 4.2 and 4.1 (including docfix)

ACK for 4.2 and 4.1 for this patchset.
Comment 13 Michael Adam 2015-01-24 00:34:11 UTC
Karolin,

please apply to 4.1 and 4.2.

Thanks - Michael
Comment 14 Karolin Seeger 2015-01-24 21:09:07 UTC
Pushed to autobuild-v4-[1|2]-test.
Comment 15 Karolin Seeger 2015-01-29 20:11:13 UTC
Pushed to both branches.
Closing out bug report.

Thanks!