[Gluster-devel] Adding xxhash to gluster code base

[Niels de Vos]

On Tue, Jun 27, 2017 at 08:09:25AM -0400, Kaleb S. KEITHLEY wrote:
> 
> xxhash doesn't seem to change much. Last update to the non-test code was six
> months ago.
> 
> bundling giant (for some definition of giant) packages/projects would be
> bad. bundling two (three if you count the test) C files doesn't seem too bad
> when you consider that there are already three or four packages in fedora
> (perl, python, R-digest, ghc (gnu haskell) that have implementations of
> xxhash or murmur but didn't bother to package a C implementation and use it.

I prefer to have as little maintenance components in the Gluster sources
as we can. The maintenance burdon is already very high. The number of
changes to xxhash seem limited, but we still need someone to track and
pay attention to them.

> I'd be for packaging it in Fedora rather than bundling it in gluster. But
> then we get to "carry" it in rhgs as we do with userspace-rcu.

We should descide what the most maintainable solution is. Having package
maintainers with the explicit task to keep xxhash updated and current is
apealing to me. Merging (even small) projects into the Gluster codebase
will add more maintenance need to the project members. Therefor I have a
strong preference to use xxhash (or an other library) that is provided
by distributions. The more common the library is, the better it will be
maintained without our (Gluster Community's) help.

Niels


> On 06/27/2017 04:08 AM, Niels de Vos wrote:
> > On Tue, Jun 27, 2017 at 12:25:11PM +0530, Kotresh Hiremath Ravishankar wrote:
> > > Hi,
> > > 
> > > We were looking for faster non-cryptographic hash to be used for the
> > > gfid2path infra [1]
> > > The initial testing was done with md5 128bit checksum which was a slow,
> > > cryptographic hash
> > > and using it makes software not complaint to FIPS [2]
> > > 
> > > On searching online a bit we found out xxhash [3] seems to be faster from
> > > the results of
> > > benchmark tests shared and lot of projects use it. So we have decided to us
> > > xxHash
> > > and added following files to gluster code base with the patch [4]
> > > 
> > >      BSD 2-Clause License:
> > >         contrib/xxhash/xxhash.c
> > >         contrib/xxhash/xxhash.h
> > > 
> > >      GPL v2 License:
> > >         tests/utils/xxhsum.c
> > > 
> > > NOTE: We have ignored the code guideline check for these files as
> > > maintaining it
> > > further becomes difficult.
> > > 
> > > Please comment on the same if there are any issues around it.
> > 
> > How performance critical is the hashing for gfid2path?
> > 
> > What is the plan to keep these files maintained? At minimal we need to
> > add these files to MAINTAINERS and the maintainers need to cherry-pick
> > updates and bugfixes from the original project. The few patches a year
> > makes this a recurring task that should not be forgoten. It would be
> > much better to use this as an external library that is provided by the
> > distributions. We already rely on OpenSSL, does this library not provide
> > an alternative 'FIPS approved' hashing that performs reasonably well?
> > 
> > Some distributions are very strict on bundling external projects, and we
> > need to inform the packagers about the additions so that they can handle
> > it correctly. Adding an external project to contrib/ should be mentioned
> > in the release notes at the very least.
> > 
> > Note that none of the symbols of any public functions in Gluster may
> > collide with functions in standard distribution libraries. This causes
> > for regular problems with gfapi applications. All exposed symbols that
> > get imported in contrib/ should have a gf_ prefix.
> > 
> > Thanks,
> > Niels
> > 
> > 
> > > 
> > > [1] Issue: https://github.com/gluster/glusterfs/issues/139
> > > [2] https://en.wikipedia.org/wiki/Federal_Information_Processing_Standards
> > > [3] http://cyan4973.github.io/xxHash/
> > > [4] https://review.gluster.org/#/c/17488/10
> > > 
> > > 
> > > 
> > > -- 
> > > Thanks and Regards,
> > > Kotresh H R and Aravinda VK
>