Re: [PATCH UPDATED 09/14] block: clean up request completion API

[Tejun Heo <htejun@gmail.com>]

Hello, Boaz.

Boaz Harrosh wrote:
> Rrrr.
> 
> This patch could be nice, but not after I've seen the previous one.
> The first one was much^3 nicer.

Heh... can't make everyone happy.  :-)

> Maybe all you need to do is push the lock flag into blk_finish_request()
> 
> <original patch>
> +	if (!locked) {
>> +		spin_lock_irqsave(q->queue_lock, flags);
>> +		finish_request(rq, error);
>> +		spin_unlock_irqrestore(q->queue_lock, flags);
>> +	} else
>> +		finish_request(rq, error);
>>
> </original patch>
> 
> Then you have only one call site to finish_request() inside blk_end_io().
> 
> finish_request() will become the ugly site, but if looking at the
> alternative I think it is worth it. Code is smaller, cleaner, and
> clearer. (Sometimes principles must be sacrificed)
> 
> At the end, I hate that lock around finish_request(), I wish the
> req->end_io() was not called with the lock held and the plain
> blk_put_request() (locking version) could be called. Having
> req->end_io() under lock is a pain in the ass that makes you go
> through loops when you need proper error handling. One day I will
> get rid of it.
>
> Tejun? now that you done both, which one do you like better? or is
> it just me?

Frankly, I don't care one way or the other as long as there's no
duplication in code paths.  Pros and cons of both approaches are
minimal and/or cosmetic.  One might remove a branch at the cost of
minutely larger code.  It just doesn't matter all that much.

I think it's best to let the maintainer have his/her way on things
like this.  I mean, maintainership is a hard work.  There gotta be
some perks.  :-)

Joke aside, I think it's a good idea to follow maintainer's decisions
on things like this as it helps making the subsystem code more
consistent which is usually way more important than minute technical
or cosmetic advantages.

Thanks.

-- 
tejun