On 6/8/19 1:28 PM, Linus Torvalds wrote:

On Sat, Jun 8, 2019 at 1:21 AM Jens Axboe [off-list ref] wrote:

quoted

Angelo Ruocco (2):
       cgroup: let a symlink too be created with a cftype file

So I'm not seeing any acks by the cgroup people who actually maintain
that file, and honestly, the patch looks butt-ugly to me.

Why are you adding an odd "write_link_name" boolean argument to
cgroup_file_name() that is really hard to explain?

When you see this line of code, what does that "false" tell you?

         return cgroup_fill_name(cgrp, cft, buf, false);

Does that look legible to you?

It looks to me like it would have been much easier and straightforward
- and legible - to just pass in the name itself, and make
cgroup_file_name() do

         return cgroup_fill_name(cgrp, cft, buf, cft->name);

instead, and now the code kind of explains itself, in ways that
"false" does not. (And cgroup_link_name() would obviously just pass in
"cft->link_name").

That would have simplified the code, and I think would have made the
call be a lot more obvious than passing in a random "true/false"
parameter that makes no conceptual sense and just looks odd in that
context.

Maybe there's something I'm missing and there's some advantage to the
incomprehensible bool argument?

I've pulled this, but seriously - when you change files that aren't
maintained by you, you should get their approval.

And if this had been a completely trivial one-liner, I wouldn't care,
but when the change looks _ugly_, I really want that ack.

FWIW, the concept/idea goes back a few months and was discussed with
the cgroup folks. But I totally agree that the implementation could
have been cleaner, especially at this point in time.

I'm fine with you reverting those two patches for 5.2 if you want to,
and the BFQ folks can do this more cleanly for 5.3.

-- 
Jens Axboe