On Thu, Sep 12, 2013 at 06:35:05PM +0200, Lukas Slebodnik wrote: > On (11/09/13 17:55), Simo Sorce wrote: > >On Wed, 2013-09-11 at 21:21 +0200, Lukas Slebodnik wrote: > >> On (11/09/13 12:50), Simo Sorce wrote: > >> >On Wed, 2013-09-11 at 17:39 +0200, Lukas Slebodnik wrote: > >> >> On (11/09/13 16:30), Michal Židek wrote: > >> >> >On 09/11/2013 04:16 PM, Simo Sorce wrote: > >> >> >>On Wed, 2013-09-11 at 15:58 +0200, Lukas Slebodnik wrote: > >> >> >>>On (11/09/13 09:39), Simo Sorce wrote: > >> >> >>>>On Wed, 2013-09-11 at 12:24 +0200, Lukas Slebodnik wrote: > >> >> >>>>>ehlo, > >> >> >>>>> > >> >> >>>>>Two patches are attached. > >> >> >>>>> > >> >> >>>>>The 1st patch reverts commit > >> >> >>>>>4662725ffef62b3b2502481438effa7c8fef9f80. > >> >> >>>>>"mmap_cache: Skip records which doesn't have same hash" > >> >> >>>>> --next patch lokks nicer with recerted commit 4662725ffe > >> >> >>>>> > >> >> >>>>> > >> >> >>>>>The 2nd patch changes behaviour of record chaining. > >> >> >>>>> > >> >> >>>>>LS > >> >> >>>> > >> >> >>>>NACK, you cannot just change the record structure w/o bumping the > >> >> >>>>whole > >> >> >>>>mmap MAJOR version number. > >> >> >>>> > >> >> >>>>Aside: > >> >> >>>> > >> >> >>>>If we are not I am thinking whether we should do some other > >> >> >>>>optimizations to keep the size down. > >> >> >>>> > >> >> >>>>For example, hashes are generally relatively small quantities as we > >> >> >>>>do > >> >> >>>>not allow huge tables. If we can limit tables size to a max of 65k > >> >> >>>>elements we could change the hash type from uint32_t to uint16_t, > >> >> >>>>and > >> >> >>>>end up using the same size we are using now. > >> >> >>>>We already limit the total mmap size to 4G as rel_prt_t is a > >> >> >>>>uint32_t, > >> >> >>>>and in practice we'll never allow maps with a very large size as it > >> >> >>>>eats > >> >> >>>>into the memory available to 32 bit processes. > >> >> >>>>So perhaps 65k for the hash tables is an acceptable limit. (I think > >> >> >>>>we > >> >> >>>>currently have a 100k limit which is probably a little bit > >> >> >>>>overblown as > >> >> >>>>a default anyway. > >> >> >>>> > >> >> >>>>Simo. > >> >> >>>> > >> >> >>>I would rather reduce "uint64_t expire" into "uint32_t expire_offset" > >> >> >>> > >> >> >>>expire_offset will be time since creation of mmap cache. > >> >> >>>and uint64_t "creation_time" will be stored in "struct sss_mc_ctx" > >> >> >> > >> >> >>Uhmm, I do agree that 4billion seconds offset is probably ok, but then > >> >> >>it means the information is split in 2 different places, and this will > >> >> >>make clients more complicated, as clients will have to keep track of > >> >> >>the > >> >> >>creation time + expiration offset now ... > >> >> >> > >> >> >>>Do you agree? > >> >> >> > >> >> >>I am not 100% convinced yet, although I do agree that it would save > >> >> >>quite some space. > >> >> >> > >> >> >>>Do I need to increment SSS_MC_MAJOR_VNO or only SSS_MC_MINOR_VNO? > >> >> >> > >> >> >>Major, you are completely changing the structure, an older client > >> >> >>cannot > >> >> >>be allowed to use the new map unless it explicitly know the layout > >> >> >>changed. > >> >> > > >> >> >Well, if we already have to change the major version, I would go with > >> >> >the two 32 bit next1 and nex2 members. If we really want to keep the > >> >> >current size of the record (btw. why? We have to change the major ver. > >> >> >anyway) then I like the two 16 bit pointers next1 and next2 more than > >> >> >the 32 bit time offset (the memcache is just another cache layer, so it > >> >> >does not have to be "gigantic"... 16 bit pointers are just fine IMO). > >> >> > > >> >> I don't like 16 bit next pointers. > >> >> > >> >> Default value of hash table size is 100000 (for passwd) > >> >> It is 53% greater value than UINT_16_MAX. > >> >> If we decided to enlarge this default value in future, we would be still > >> >> limited by UINT_16_MAX. > >> >> > >> >> I think it is not good idea to reduce size of next pointers. > >> > > >> >I am thinking we might want to change the way we create the way we > >> >handle record size to see if we can make a change that will be more > >> >future proof. > >> > > >> >If we stored the size of the records in the header and always calculate > >> >the position of b2 based on that information then we could introduce a > >> >variable size record structure. This would allow in future to increase > >> >the size of the structure for optional parameters w/o necessarily having > >> >to bump up the major version. > >> > > >> >However I want to have at the very least 64bit alignemtn always > >> >preserverd, so from 32 bytes we want to grow in 8 bytes steps, which > >> >means adding a uint32_t padding before b2. > >> totally agree. > >> > >> SSS_MC_MAJOR_VNO was increased. > >> padding was added. sizeof(struct sss_mc_ctx) will be 40 > >> > >> Updated patches are attached. > > > >It looks ok to me. > > > >However, just to be clear we are planning on pushing this to 1.12 only > >right ? > Branch 1.11 has not been created yet. (only tag) > > Are we fine to with patches in 1.11 (and maybe 1.10) ? > > LS
I would prefer to have Lukas' patch in RHEL-7. With upstream 1.11 and Fedora packages, I'd be fine with the workaround if there are concerns about backwards compatibility. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
