Re: [PATCH 04/18] libosd: OSDv1 preliminary implementation

[Boaz Harrosh <bharrosh@panasas.com>]

Andrew Morton wrote:
> On Tue,  4 Nov 2008 18:44:29 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> Implementation of the most basic OSD functionality and
>> infrastructure. Mainly Format, Create/Remove Partition,
>> Create/Remove Object, and read/write.
>>
>>
>> ...
>>
>> +struct osd_request *_osd_request_alloc(gfp_t gfp)
>> +{
>> +	struct osd_request *or;
>> +
>> +	/* TODO: Use mempool with one saved request */
>> +	or = kzalloc(sizeof(*or), gfp);
>> +	return or;
>> +}
>> +
>> +void _osd_request_free(struct osd_request *or)
>> +{
>> +	kfree(or);
>> +}
> 
> These two functions can/should be made static.  Please generally check
> for this.
> 

Thanks, I usually do, but these are from the last iteration and were
rushed in. Will fix.

> Also it'd probably make sense to declare both these inline.  The
> compiler _shoudl_ get it right, but stranger things have happened...
> 

I do not inline them, because one - I will use mem_pools very soon they
are just place holders for now. two - I let the compiler
do that, I made sure the only user is below the definition and I let
the compiler decide. I like to leave these things controlled from outside,
so when I compile a UML kernel and finally need to fire up a debugger,
I can un-inline them very easily.

(This is why I hate forward declarations. If they are not used
 it is a proof that inlineing of single callers will always happen.)

>> ...
>>
>> +/*
>> + * If osd_finalize_request() was called but the request was not executed through
>> + * the block layer, then we must release BIOs.
>> + */
>> +static void _abort_unexecuted_bios(struct request *rq)
>> +{
>> +	struct bio *bio;
>> +
>> +	while ((bio = rq->bio) != NULL) {
>> +		rq->bio = bio->bi_next;
>> +		bio_endio(bio, 0);
>> +	}
>> +}
> 
> Boy, that's a scary function.  bye-bye data.
> 

Thank's for mentioning that. I use it at the very end just before
the de-allocation of the block request. What happens today is: that
if for some reason the driver failed to call blk_end_request, 
or in this case the driver was never called, the last blk_put_request()
will leak BIOs. There are currently corner cases and bugs in the Kernel
that cause exactly that.

That loop above should be moved from here to inside blk_put_request().
if some one needs to hold the BIOs longer then the life span of the request they
should simply inc-ref them.

Note that here it is totally safe since It's only called just before
blk_put_request().

This code is actually a bug fix, for the error cases when a request is allocated
but is never executed do to other error returns.

Thanks
Boaz