On Mon 07-01-08 21:44:26, Marcin Slusarz wrote:
> On Mon, Jan 07, 2008 at 12:26:18PM +0000, Christoph Hellwig wrote:
> > On Sun, Jan 06, 2008 at 01:44:34AM +0100, [EMAIL PROTECTED] wrote:
> > > +static struct udf_bitmap *udf_sb_alloc_bitmap(struct super_block *sb,
> > > __u32 index)
> > > +{
> > > + struct udf_part_map *map = &UDF_SB(sb)->s_partmaps[index];
> > > + int nr_groups = (map->s_partition_len + (sizeof(struct spaceBitmapDesc)
> > > << 3) +
> > > + (sb->s_blocksize * 8) - 1) / (sb->s_blocksize * 8);
> > > + int size = sizeof(struct udf_bitmap) + (sizeof(struct buffer_head *) *
> > > nr_groups);
> > > + struct udf_bitmap *bitmap;
> > > +
> > > + if (size <= PAGE_SIZE)
> > > + bitmap = kmalloc(size, GFP_KERNEL);
> > > + else
> > > + bitmap = vmalloc(size);
> > > + if (bitmap != NULL) {
> > > + memset(bitmap, 0x00, size);
> > > + bitmap->s_block_bitmap = (struct buffer_head **)(bitmap + 1);
> > > + bitmap->s_nr_groups = nr_groups;
> > > + } else
> > > + udf_error(sb, __FUNCTION__, "Unable to allocate space for
> > > bitmap and %d buffer_head pointers", nr_groups);
> > > + return bitmap;
> > > +}
> >
> > There's some overly long lines here and some odd style, this should look
> > more like:
> These long lines were split later in "[PATCH 1/7] udf: fix coding style"
> (but I will fix it in next version of this patch).
>
> > static struct udf_bitmap *udf_sb_alloc_bitmap(struct super_block *sb,
> > u32 index)
> > {
> > struct udf_part_map *map = &UDF_SB(sb)->s_partmaps[index];
> > struct udf_bitmap *bitmap;
> > int nr_groups;
> > int size;
> >
> > nr_groups = (map->s_partition_len +
> > (sizeof(struct spaceBitmapDesc) << 3) +
> > (sb->s_blocksize * 8) - 1) /
> > (sb->s_blocksize * 8);
> > size = sizeof(struct udf_bitmap) +
> > (sizeof(struct buffer_head *) * nr_groups);
> > if (size <= PAGE_SIZE)
> > bitmap = kmalloc(size, GFP_KERNEL);
> > else
> > bitmap = vmalloc(size);
> >
> > if (!bitmap) {
> > udf_error(sb, __FUNCTION__,
> > "Unable to allocate space for bitmap "
> > "and %d buffer_head pointers", nr_groups);
> > return NULL;
> > }
> >
> > memset(bitmap, 0, size);
> > bitmap->s_block_bitmap = (struct buffer_head **)(bitmap + 1);
> > bitmap->s_nr_groups = nr_groups;
> > return bitmap;
> > }
> Yep. This looks better.
>
> > But even that is not quite optimal. The nr_groups calculation should
> > probably move to a helper (I suspect it's used elsewhere too anyway),
> I will look for them. I've seen many weird calculations in udf code
> and I think it's a good idea to move some of them into helpers.
>
> > and instead of using vmalloc for large allocations I'd rather split
> > the allocation of the bitmap from s->block_bitmap and use individual
> > smaller allocations. But that latter part is probably better left
> > for a separate patch.
> So every struct buffer_head * in bitmap will be indexed by a pair of:
> idx >> ilog2(PAGE_SIZE / sizeof(struct buffer_head *))
> idx & ((PAGE_SIZE / sizeof(struct buffer_head *)) - 1)
> Am I right? Or did I misunderstand something?
> I don't know how big nr_groups could be, but it might still need vmalloc
> when array of pages won't fit on one page...
Size of group is a number of bits in a block blocks. So in case of 1 KB
blocks a group has 8192 blocks => 8MB. Since you can fit 1024 pointers to a
page (or even only 512 in 64-bit arch), that gives you 8GB (or 4GB,
respectively). So it's likely that pointers won't fit one page.
Personally, I don't think removing the vmalloc is worth the trouble. At
least for now...
Honza
--
Jan Kara <[EMAIL PROTECTED]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
