On Wed, 2012-02-08 at 07:04 -0500, Jeff Layton wrote:
> On Tue, 7 Feb 2012 20:05:55 +0000
> "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote:
>
> > On Tue, 2012-02-07 at 14:44 -0500, Trond Myklebust wrote:
> >
> > > One way to easily shrink the size of that allocation is to convert the
> > > ih_name string into a pointer, and have the downcall allocate the
> > > storage for that string dynamically...
> >
> > Like so....
> >
>
> Nice. I had started looking at this, but was still sorting my way
> through how all of the hashtable cache code works.
>
> One comment inline below:
>
> > 8<-----------------------------------------------------------------------
> > From 5221c8415957604507cd7408a695241096bcb3ad Mon Sep 17 00:00:00 2001
> > From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> > Date: Tue, 7 Feb 2012 14:59:05 -0500
> > Subject: [PATCH] NFSv4: Reduce the footprint of the idmapper
> >
> > Instead of pre-allocating the storage for all the strings, we can
> > significantly reduce the size of that table by doing the allocation
> > when we do the downcall.
> >
> > Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> > ---
> > fs/nfs/idmap.c | 16 +++++++++++++---
> > 1 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
> > index 5a5566f..fff7948 100644
> > --- a/fs/nfs/idmap.c
> > +++ b/fs/nfs/idmap.c
> > @@ -362,7 +362,7 @@ struct idmap_hashent {
> > unsigned long ih_expires;
> > __u32 ih_id;
> > size_t ih_namelen;
> > - char ih_name[IDMAP_NAMESZ];
> > + const char *ih_name;
> > };
> >
> > struct idmap_hashtable {
> > @@ -482,12 +482,17 @@ void
> > nfs_idmap_delete(struct nfs_client *clp)
> > {
> > struct idmap *idmap = clp->cl_idmap;
> > + int i;
> >
> > if (!idmap)
> > return;
> > nfs_idmap_unregister(clp, idmap->idmap_pipe);
> > rpc_destroy_pipe_data(idmap->idmap_pipe);
> > clp->cl_idmap = NULL;
> > + for (i = 0; i < ARRAY_SIZE(idmap->idmap_user_hash.h_entries); i++)
> > + kfree(idmap->idmap_user_hash.h_entries[i].ih_name);
> > + for (i = 0; i < ARRAY_SIZE(idmap->idmap_group_hash.h_entries); i++)
> > + kfree(idmap->idmap_group_hash.h_entries[i].ih_name);
> > kfree(idmap);
> > }
> >
> > @@ -634,9 +639,14 @@ static void
> > idmap_update_entry(struct idmap_hashent *he, const char *name,
> > size_t namelen, __u32 id)
> > {
> > + char *str = kmalloc(namelen + 1, GFP_KERNEL);
>
> Do we need to worry about recursing into direct reclaim here?
No. The write path doesn't use the idmapper any more.
> > + if (str == NULL)
> > + return;
> > + kfree(he->ih_name);
> > he->ih_id = id;
> > - memcpy(he->ih_name, name, namelen);
> > - he->ih_name[namelen] = '\0';
> > + memcpy(str, name, namelen);
> > + str[namelen] = '\0';
> > + he->ih_name = str;
> > he->ih_namelen = namelen;
> > he->ih_expires = jiffies + nfs_idmap_cache_timeout;
> > }
>
> ...and the namelen check in idmap_lookup_name should keep the memcmp
> from tripping over these NULL pointers. I think this looks like a nice
> interim fix for now and should cut down on the memory consumption from
> this code.
>
> On a x86_64 machine:
>
> pre-patch:
> (gdb) p sizeof(struct idmap)
> $1 = 39504
>
> post-patch:
> (gdb) p sizeof(struct idmap)
> $1 = 8784
>
> That changes this from an order 4 to an order 2 allocation. Would be
> even nicer to get it down to under a page, but this will help
> considerably.
>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
We could do that by allocating the two idmap_hashent arrays separately.
I'll see about that later today.
In the mean time, I've got Bryan continuing to look into the possibility
of using the keyrings for both types of upcall.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
