If we don't need a huge amount of memory in ->readdir() then
we can use kmalloc rather than vmalloc to allocate it. This
should cut down on the greater overheads associated with
vmalloc for smaller directories.

We may be able to eliminate vmalloc entirely at some stage,
but this is easy to do right away.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 401deaf..80d9dfb 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -1248,12 +1248,14 @@ static int gfs2_dir_read_leaf(struct inode *inode, u64 *offset, void *opaque,
struct gfs2_leaf *lf;
unsigned entries = 0, entries2 = 0;
unsigned leaves = 0;
+ unsigned alloc_size;
const struct gfs2_dirent **darr, *dent;
struct dirent_gather g;
- struct buffer_head **larr;
+ struct buffer_head **larr = NULL;
int leaf = 0;
int error, i;
u64 lfn = leaf_no;
+ int do_vfree = 0;

do {
error = get_leaf(ip, lfn, &bh);
@@ -1278,9 +1280,15 @@ static int gfs2_dir_read_leaf(struct inode *inode, u64 *offset, void *opaque,
* 99 is the maximum number of entries that can fit in a single
* leaf block.
*/
- larr = vmalloc((leaves + entries + 99) * sizeof(void *));
- if (!larr)
- goto out;
+ alloc_size = (leaves + entries + 99) * sizeof(void *);
+ if (alloc_size < KMALLOC_MAX_SIZE)
+ larr = kmalloc(alloc_size, GFP_NOFS);
+ if (!larr) {
+ larr = vmalloc(alloc_size);
+ if (!larr)
+ goto out;
+ do_vfree = 1;
+ }
darr = (const struct gfs2_dirent **)(larr + leaves);
g.pdent = darr;
g.offset = 0;
@@ -1289,7 +1297,7 @@ static int gfs2_dir_read_leaf(struct inode *inode, u64 *offset, void *opaque,
do {
error = get_leaf(ip, lfn, &bh);
if (error)
- goto out_kfree;
+ goto out_free;
lf = (struct gfs2_leaf *)bh->b_data;
lfn = be64_to_cpu(lf->lf_next);
if (lf->lf_entries) {
@@ -1298,7 +1306,7 @@ static int gfs2_dir_read_leaf(struct inode *inode, u64 *offset, void *opaque,
gfs2_dirent_gather, NULL, &g);
error = PTR_ERR(dent);
if (IS_ERR(dent))
- goto out_kfree;
+ goto out_free;
if (entries2 != g.offset) {
fs_warn(sdp, "Number of entries corrupt in dir "
"leaf %llu, entries2 (%u) != "
@@ -1307,7 +1315,7 @@ static int gfs2_dir_read_leaf(struct inode *inode, u64 *offset, void *opaque,
entries2, g.offset);

error = -EIO;
- goto out_kfree;
+ goto out_free;
}
error = 0;
larr[leaf++] = bh;
@@ -1319,10 +1327,13 @@ static int gfs2_dir_read_leaf(struct inode *inode, u64 *offset, void *opaque,
BUG_ON(entries2 != entries);
error = do_filldir_main(ip, offset, opaque, filldir, darr,
entries, copied);
-out_kfree:
+out_free:
for(i = 0; i < leaf; i++)
brelse(larr[i]);
- vfree(larr);
+ if (do_vfree)
+ vfree(larr);
+ else
+ kfree(larr);
out:
return error;
}

On Wed, Jul 28, 2010 at 4:15 AM, Steven Whitehouse <swhiteho@redhat.com> wrote:
>
> We may be able to eliminate vmalloc entirely at some stage,
> but this is easy to do right away.

Quite frankly, I'd much rather see this abstracted out a bit. Why not just do a

void *memalloc(unsigned int size)
{
if (size < KMALLOC_MAX_SIZE) {
void *ptr = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
if (ptr)
return ptr;
}
return vmalloc(size);
}

void memfree(void *ptr)
{
unsigned long addr = (unsigned long) ptr;

if (is_vmalloc_addr(addr)) {
vfree(ptr);
return;
}

kfree(ptr);
}

wouldn't that be much nicer? No need for that explicit flag, and you
don't mess up an already way-too-ugly function even more.

Also, I do notice that you used GFP_NOFS, but you didn't use that for
the vmalloc() thing. If there really are lock reentrancy reasons, you
_could_ use __vmalloc(size, GFP_NOFS, PAGE_KERNEL). But since you've
been using vmalloc() for a long time, I suspect GFP_KERNEL works fine.
Yes/no?

Linus

Hi,

On Wed, 2010-07-28 at 08:39 -0700, Linus Torvalds wrote:
> On Wed, Jul 28, 2010 at 4:15 AM, Steven Whitehouse <swhiteho@redhat.com> wrote:
> >
> > We may be able to eliminate vmalloc entirely at some stage,
> > but this is easy to do right away.
>
> Quite frankly, I'd much rather see this abstracted out a bit. Why not just do a
>
> void *memalloc(unsigned int size)
> {
> if (size < KMALLOC_MAX_SIZE) {
> void *ptr = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
> if (ptr)
> return ptr;
> }
> return vmalloc(size);
> }
>
> void memfree(void *ptr)
> {
> unsigned long addr = (unsigned long) ptr;
>
> if (is_vmalloc_addr(addr)) {
> vfree(ptr);
> return;
> }
>
> kfree(ptr);
> }
>
> wouldn't that be much nicer? No need for that explicit flag, and you
> don't mess up an already way-too-ugly function even more.
>
> Also, I do notice that you used GFP_NOFS, but you didn't use that for
> the vmalloc() thing. If there really are lock reentrancy reasons, you
> _could_ use __vmalloc(size, GFP_NOFS, PAGE_KERNEL). But since you've
> been using vmalloc() for a long time, I suspect GFP_KERNEL works fine.
> Yes/no?
>
> Linus

Well it had been working ok, but I don't really trust it since we are
holding a glock on the directory in shared mode at this point. That
means that if we (as a result of dropping dcache) need to unlink an
inode, we might land up taking glocks in the wrong order.

We use GFP_NOFS everywhere else under glocks for that reason, so I think
its safer to use GFP_NOFS here.

An updated patch based on your comments follows in the next email,

Steve.