On Thu, 2013-09-12 at 11:26 +0200, Jakub Hrozek wrote:
On Thu, Sep 12, 2013 at 09:05:07AM +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 ?
> >
> >I am not hugely comfortable with pushing it to existing releases.
Can you describe why? Are you afraid of regressions? (Genuine
questions, sorry, I still don't know the memcache code well)
Yes.
Would it help to have a memcache unit test?
No, we are talking about old client trying to read new mmpa file with
incompatible format and failing. A unit test will not do because we have
always the new code in there.
But for the future we should probably have a sample file we dro pin
place and make sure client libraries properly ignore any format they do
not understand.
Both an older format no more supported and a newer format not yet
supported (basically just new major version and then all zeros instead
of content beyond the header.
> The main problem is that existing releases could crash in some
rare situations
> There is a report in bugzilla one week old.
>
https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
>
> User reported crash and he had latest sssd 1-10 with all
> mmap_cache patches; but it didn't help. sssd_nss crashed.
>
> We had a long discussion about solution (revalidation of chains), but we did
> not find any simple "back-compatible" fix.
>
> How do we want to fix it in older branches If we don't want to backport patch
> from previous mail.
Right, I'd like to backport a fix for the above problem to all existing
branches if possible.
I know, but we need some regression testing and not rush it.
Simo.
--
Simo Sorce * Red Hat, Inc * New York