Messages in this thread | | | Date | Mon, 7 Jan 2008 21:44:26 +0100 | From | Marcin Slusarz <> | Subject | Re: [PATCH] udf: convert some macros to functions |
| |
On Mon, Jan 07, 2008 at 12:26:18PM +0000, Christoph Hellwig wrote: > On Sun, Jan 06, 2008 at 01:44:34AM +0100, marcin.slusarz@gmail.com 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
| |