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...
> But in generally this is a good cleanup and thanks a lot for working
> on this filesystem driver.
Thanks! :)
Marcin
--
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/
