linux-kernel - Re: [PATCH] block: make nr

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 09 Jun 2014 09:53:48 -0600
From:	Jens Axboe <axboe@...nel.dk>
To:	Andreas Mohr <andi@...as.de>, Junxiao Bi <junxiao.bi@...cle.com>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block: make nr_requests tunable for loop

On 2014-06-09 01:29, Andreas Mohr wrote:
> Hi,
>
> having had a look at current mainline sources,
> frankly I've (well, initially...) got trouble understanding
> what this patch is doing.
>
> It's replacing an aggressive error-type bail-out (-EINVAL) for NULL request_fn
> with an inoccuous-looking "return ret;", yet that ret content currently
> *implicitly* is a >= 0 value (resulting from processing by earlier code
> which may or may not get incomprehensibly rewritten in future).
> I don't understand the reasons for this huge change in return value handling
> (since it's now not assigning a specific return value
> for this modified bail-out case).
>
> OK, well... you could say that since all this function ever was
> interested in is the result value of queue_var_store()
> (except for error bail-out cases), doing an interim "return ret;"
> (which is exactly what the function tail is also doing)
> is exactly right.
>
> But still simple textual appearance of the resulting patch hunks
> seems strangely asymmetric
> which may easily be a canary for structurally wrong layering of this function.
> Not to mention the now required extra spin_unlock_irq()
> in interim return handler...
>
>
> Well, after further analysis I would come to the conclusion
> that in general queue_requests_store() does a LOT more than it should -
> since blk-sysfs.c's only (expected!) purpose is
> to do parameterization of request_queue behaviour as gathered
> from sysfs attribute space,
> all that function should ever be concerned with is parsing that sysfs value
> and then calling a blk helper for configuration of that very attribute value
> which would *internally* do all the strange internal queue magic
> that is currently being updated *open-coded*
> at this supposedly *sysfs*-specific place. Ugh.
> Main question here: what would one do if one decided to rip out sysfs
> and use something entirely different for parameterization?
> Yeah indeed - thought so...
>
>
> So yeah, I'd definitely say that that function is lacking some cleanup
> which would possibly then even lead (or: would have led ;)
> to a much more nicely symmetric textual appearance
> of the patch hunk of the small but quite likely useful change
> that you currently intend to have here.

If you are done ranting, look at the current tree where it has been 
split out. There was no reason to have it split before, since the sysfs 
entry point was the only place where we updated nr_requests. If that 
code has been duplicated, there would have been a justified reason for 
writing two pages about it.

-- 
Jens Axboe

--
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/