Re: [GIT PULL] Block fixes for 5.2-rc4

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Jens Axboe <axboe@kernel.dk>, Tejun Heo <tj@kernel.org>,
	Li Zefan <lizefan@huawei.com>,
	Johannes Weiner <hannes@cmpxchg.org>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: [GIT PULL] Block fixes for 5.2-rc4
Date: Sun, 9 Jun 2019 09:06:51 -0700	[thread overview]
Message-ID: <CAHk-=whca9riMqYn6WoQpuq9ehQ5KfBvBb4iVZ314JSfvcgy9Q@mail.gmail.com> (raw)
In-Reply-To: <52daccae-3228-13a1-c609-157ab7e30564@kernel.dk>

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

On Sat, Jun 8, 2019 at 11:00 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> 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.

I don't think the code is _broken_, and I don't think the link_name
thing is wrong. So no point in reverting unless we see more issues.

I just wish it had been done differently, both from the patch details
standpoint, but also in making sure the cgroup people were aware (and
maybe they were, but it certainly didn't show up in the commit).

So I think an incremental patch like the attached would make the code
easier to understand (I really do mis-like random boolean flags being
passed around that change behavior in undocumented and non-obvious
ways), but I'd also want to make sure that Tejun & co are all on board
and know about it..

I'm sure this happens a lot, but during the rc series I just end up
*looking* at details like this a lot more, when I see changes outside
of a subsystem directory.

Tejun&co, we're talking about commit 54b7b868e826 ("cgroup: let a
symlink too be created with a cftype file") which didn't have any sign
of you guys being aware of it or having acked it.

                   Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1698 bytes --]

 kernel/cgroup/cgroup.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 155048b0eca2..fa25f0f6fe23 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1461,7 +1461,7 @@ struct cgroup *task_cgroup_from_root(struct task_struct *task,
 static struct kernfs_syscall_ops cgroup_kf_syscall_ops;
 
 static char *cgroup_fill_name(struct cgroup *cgrp, const struct cftype *cft,
-			      char *buf, bool write_link_name)
+			      char *buf, const char *name)
 {
 	struct cgroup_subsys *ss = cft->ss;
 
@@ -1470,11 +1470,9 @@ static char *cgroup_fill_name(struct cgroup *cgrp, const struct cftype *cft,
 		const char *dbg = (cft->flags & CFTYPE_DEBUG) ? ".__DEBUG__." : "";
 
 		snprintf(buf, CGROUP_FILE_NAME_MAX, "%s%s.%s",
-			 dbg, cgroup_on_dfl(cgrp) ? ss->name : ss->legacy_name,
-			 write_link_name ? cft->link_name : cft->name);
+			 dbg, cgroup_on_dfl(cgrp) ? ss->name : ss->legacy_name, name);
 	} else {
-		strscpy(buf, write_link_name ? cft->link_name : cft->name,
-			CGROUP_FILE_NAME_MAX);
+		strscpy(buf, name, CGROUP_FILE_NAME_MAX);
 	}
 	return buf;
 }
@@ -1482,13 +1480,13 @@ static char *cgroup_fill_name(struct cgroup *cgrp, const struct cftype *cft,
 static char *cgroup_file_name(struct cgroup *cgrp, const struct cftype *cft,
 			      char *buf)
 {
-	return cgroup_fill_name(cgrp, cft, buf, false);
+	return cgroup_fill_name(cgrp, cft, buf, cft->name);
 }
 
 static char *cgroup_link_name(struct cgroup *cgrp, const struct cftype *cft,
 			      char *buf)
 {
-	return cgroup_fill_name(cgrp, cft, buf, true);
+	return cgroup_fill_name(cgrp, cft, buf, cft->link_name);
 }
 
 /**
