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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 23 Apr 2009 18:59:46 +0900
From:	Tejun Heo <htejun@...il.com>
To:	Boaz Harrosh <bharrosh@...asas.com>
CC:	Christoph Hellwig <hch@...radead.org>, axboe@...nel.dk,
	linux-kernel@...r.kernel.org, bzolnier@...il.com
Subject: Re: [PATCH UPDATED 09/14] block: clean up request completion API

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/