Re: [PATCH] udf: convert some macros to functions

[Marcin Slusarz <marcin.slusarz@gmail.com>]

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