Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Paolo Valente <paolo.valente@linaro.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Tejun Heo <tj@kernel.org>, Li Zefan <lizefan@huawei.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Johannes Thumshirn <jthumshirn@suse.de>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Fam Zheng <fam@euphon.net>
Subject: Re: [GIT PULL] Block fixes for 5.2-rc4
Date: Mon, 10 Jun 2019 16:49:47 +0200
Message-ID: <90A8C242-E45A-4D6E-8797-598893F86393@linaro.org> (raw)
In-Reply-To: <ebfb27a3-23e2-3ad5-a6b3-5f8262fb9ecb@kernel.dk>

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



> Il giorno 10 giu 2019, alle ore 12:15, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 6/9/19 10:06 AM, Linus Torvalds wrote:
>> 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.
> 
> I talked to Tejun about this offline, and he's not a huge fan of the
> symlink. So let's revert this for now, and Paolo can do this properly
> for 5.3 instead.
> 

Hi all,
we'd be ok with implementing this the right way, but what's the point
in spending further hours on a solution not liked by Tejun?  Here's
what happened so far:
1) General solution allowing multiple entities to share common files:
   rejected by Tejun.
2) Simple replacement bfq.weight -> weight, after the only entity
   using that name, cfq, has gone: rejected by Jens because it is a
   disruptive change of the interface.
3) Symlink, proposed by Johannes: maybe rejected by Tejun.

Tejun, could you please tell us whether you may accept the last
option?  This option may be associated with deprecating the explicit
use of bfq.weight (I don't know of anybody who wants to use this
confusing name).  So, in a few releases we could finally drop this
bfq.weight and turn the symlink back into an actual interface file.

I'm running out of options, apart from giving up.

Thanks,
Paolo

> Sorry for the confusion! Please pull the below.
> 
> 
>  git://git.kernel.dk/linux-block.git tags/for-linus-20190610
> 
> 
> ----------------------------------------------------------------
> Jens Axboe (1):
>      cgroup/bfq: revert bfq.weight symlink change
> 
> block/bfq-cgroup.c          |  6 ++----
> include/linux/cgroup-defs.h |  3 ---
> kernel/cgroup/cgroup.c      | 33 ++++-----------------------------
> 3 files changed, 6 insertions(+), 36 deletions(-)
> 
> --
> Jens Axboe


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-08  8:20 Jens Axboe
2019-06-08 19:28 ` Linus Torvalds
2019-06-09  5:59   ` Jens Axboe
2019-06-09 16:06     ` Linus Torvalds
2019-06-10 10:15       ` Jens Axboe
2019-06-10 14:49         ` Paolo Valente [this message]
2019-06-10 15:48           ` Tejun Heo
2019-06-11 19:49             ` [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight Tejun Heo
2019-06-11 21:17               ` Tejun Heo
2019-06-12  7:32                 ` Paolo Valente
2019-06-12 13:39                   ` Tejun Heo
2019-06-13  6:10                     ` Paolo Valente
2019-06-14 20:22                       ` Tejun Heo
2019-06-14 21:05                         ` Paolo Valente
2019-06-08 19:30 ` [GIT PULL] Block fixes for 5.2-rc4 pr-tracker-bot

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=90A8C242-E45A-4D6E-8797-598893F86393@linaro.org \
    --to=paolo.valente@linaro.org \
    --cc=axboe@kernel.dk \
    --cc=fam@euphon.net \
    --cc=hannes@cmpxchg.org \
    --cc=jthumshirn@suse.de \
    --cc=linus.walleij@linaro.org \
    --cc=linux-block@vger.kernel.org \
    --cc=lizefan@huawei.com \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=ulf.hansson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox