On 10/03/2002 at 10:05 AM, Christoph Hellwig wrote:
>> +static mempool_t *my_bio_split_pool, *my_bio_pool;
>> +static kmem_cache_t *my_bio_split_slab, *my_bio_pool_slab;
> Umm, static variables in header files?
Yupp, that wasn't accidental.
Based on the conclusions mutually agreed on in the OLS
BOF on bio splitting, it was decided that each driver
having the need to split bios *should* have their own
private bio pools to use exclusively for this purpose.
Thus any plugin that includes this header gets their
own private copies of these variables.
>> +
>> +/**
>> + * slab_pool_alloc
>> + * @gfp_mask: GFP allocation flag
>> + * @data: mempool prototype required fields
>> + *
>> + * mempool allocate function
>> + **/
>> +static void *
>> +slab_pool_alloc(int gfp_mask, void *data)
>> +{
>> + return kmem_cache_alloc(data, gfp_mask);
>> +}
>> +
>> +/**
>> + * slab_pool_free
>> + * @ptr: mempool prototype required fields
>> + * @data: mempool prototype required fields
>> + *
>> + * mempool free function
>> + **/
>> +static void
>> +slab_pool_free(void *ptr, void *data)
>> +{
>> + kmem_cache_free(data, ptr);
>> +}
> I think these two could go to slab.c instead.
I agree. I've seen a few other static versions of the
same thing floating around.
>> + if (!my_bio_split_slab) {
>> + panic("unable to create EVMS Bio Split cache.");
> What about graceful error handling?
Alan already questioned this as well... and I agree. I've
already made the necessary changes.
> All in all I think this should rather be a source file, I can't
> see anything EVMS-specific either.
There may likely be generic code to perform bio splitting
at some (hopefully soon) point in time. I'm not even going
to assume that what EVMS has done will or should be it.
This support was a temporary implementation that does
allow several plugins in EVMS to work correctly with the
large I/Os now available in 2.5. I do agree, what ever is
done should be placed in some commonly usable location.
Mark
On Thu, Oct 03, 2002 at 03:36:24PM -0500, Mark Peloquin wrote:
>
> On 10/03/2002 at 10:05 AM, Christoph Hellwig wrote:
>
> >> +static mempool_t *my_bio_split_pool, *my_bio_pool;
> >> +static kmem_cache_t *my_bio_split_slab, *my_bio_pool_slab;
>
> > Umm, static variables in header files?
>
> Yupp, that wasn't accidental.
>
> Based on the conclusions mutually agreed on in the OLS
> BOF on bio splitting, it was decided that each driver
> having the need to split bios *should* have their own
> private bio pools to use exclusively for this purpose.
> Thus any plugin that includes this header gets their
> own private copies of these variables.
Having variables and (non-inline) functions is very bad style and a
perfect method for the obsfucated C contest. Just let every module
that needs bio-splitting declare it's own variables.