Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [GIT PULL] Block fixes for 5.2-rc4
@ 2019-06-08  8:20 Jens Axboe
  2019-06-08 19:28 ` Linus Torvalds
  2019-06-08 19:30 ` [GIT PULL] Block fixes for 5.2-rc4 pr-tracker-bot
  0 siblings, 2 replies; 15+ messages in thread
From: Jens Axboe @ 2019-06-08  8:20 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-block

Hi Linus,

Set of fixes that should go into this release. This pull request
contains:

- Allow symlink from the bfq.weight cgroup parameter to the general
  weight (Angelo)

- Damien is new skd maintainer (Bart)

- NVMe pull request from Sagi, with a few small fixes.

- Ensure we set DMA segment size properly, dma-debug is now tripping on
  these (Christoph)

- Remove useless debugfs_create() return check (Greg)

- Remove redundant unlikely() check on IS_ERR() (Kefeng)

- Fixup request freeing on exit (Ming)

Please pull!


  git://git.kernel.dk/linux-block.git tags/for-linus-20190608


----------------------------------------------------------------
Angelo Ruocco (2):
      cgroup: let a symlink too be created with a cftype file
      block, bfq: add weight symlink to the bfq.weight cgroup parameter

Bart Van Assche (1):
      MAINTAINERS: Hand over skd maintainership

Christoph Hellwig (4):
      nvme-pci: don't limit DMA segement size
      rsxx: don't call dma_set_max_seg_size
      mtip32xx: also set max_segment_size in the device
      mmc: also set max_segment_size in the device

Greg Kroah-Hartman (1):
      block: aoe: no need to check return value of debugfs_create functions

Jaesoo Lee (1):
      nvme: Fix u32 overflow in the number of namespace list calculation

Jens Axboe (1):
      Merge branch 'nvme-5.2-rc-next' of git://git.infradead.org/nvme into for-linus

Kefeng Wang (1):
      block: Drop unlikely before IS_ERR(_OR_NULL)

Max Gurtovoy (1):
      nvme-rdma: use dynamic dma mapping per command

Ming Lei (1):
      block: free sched's request pool in blk_cleanup_queue

Minwoo Im (1):
      nvmet: fix data_len to 0 for bdev-backed write_zeroes

Sagi Grimberg (2):
      nvme-rdma: fix queue mapping when queue count is limited
      nvme-tcp: fix queue mapping when queue count is limited

 MAINTAINERS                       |   2 +-
 block/bfq-cgroup.c                |   6 +-
 block/blk-cgroup.c                |   2 +-
 block/blk-core.c                  |  13 ++++
 block/blk-mq-sched.c              |  30 +++++++-
 block/blk-mq-sched.h              |   1 +
 block/blk-sysfs.c                 |   2 +-
 block/blk.h                       |  10 ++-
 block/elevator.c                  |   2 +-
 drivers/block/aoe/aoeblk.c        |  16 +---
 drivers/block/mtip32xx/mtip32xx.c |   1 +
 drivers/block/rsxx/core.c         |   1 -
 drivers/mmc/core/queue.c          |   2 +
 drivers/nvme/host/core.c          |   3 +-
 drivers/nvme/host/pci.c           |   6 ++
 drivers/nvme/host/rdma.c          | 152 ++++++++++++++++++++++++--------------
 drivers/nvme/host/tcp.c           |  57 ++++++++++++--
 drivers/nvme/target/io-cmd-bdev.c |   1 +
 include/linux/cgroup-defs.h       |   3 +
 kernel/cgroup/cgroup.c            |  33 ++++++++-
 20 files changed, 251 insertions(+), 92 deletions(-)

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-08  8:20 [GIT PULL] Block fixes for 5.2-rc4 Jens Axboe
@ 2019-06-08 19:28 ` Linus Torvalds
  2019-06-09  5:59   ` Jens Axboe
  2019-06-08 19:30 ` [GIT PULL] Block fixes for 5.2-rc4 pr-tracker-bot
  1 sibling, 1 reply; 15+ messages in thread
From: Linus Torvalds @ 2019-06-08 19:28 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block

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

We've had enough block layer problems over the years that it's not ok
to start just randomly changing other sub-areas without even notifying
people.

                 Linus

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-08  8:20 [GIT PULL] Block fixes for 5.2-rc4 Jens Axboe
  2019-06-08 19:28 ` Linus Torvalds
@ 2019-06-08 19:30 ` pr-tracker-bot
  1 sibling, 0 replies; 15+ messages in thread
From: pr-tracker-bot @ 2019-06-08 19:30 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Linus Torvalds, linux-block

The pull request you sent on Sat, 8 Jun 2019 02:20:57 -0600:

> git://git.kernel.dk/linux-block.git tags/for-linus-20190608

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8d72e5bd86cb405d8d8b9e92905d8cfffd08dde8

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-08 19:28 ` Linus Torvalds
@ 2019-06-09  5:59   ` Jens Axboe
  2019-06-09 16:06     ` Linus Torvalds
  0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2019-06-09  5:59 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-block

On 6/8/19 1:28 PM, Linus Torvalds wrote:
> On Sat, Jun 8, 2019 at 1:21 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-09  5:59   ` Jens Axboe
@ 2019-06-09 16:06     ` Linus Torvalds
  2019-06-10 10:15       ` Jens Axboe
  0 siblings, 1 reply; 15+ messages in thread
From: Linus Torvalds @ 2019-06-09 16:06 UTC (permalink / raw)
  To: Jens Axboe, Tejun Heo, Li Zefan, Johannes Weiner; +Cc: linux-block

[-- 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);
 }
 
 /**

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-09 16:06     ` Linus Torvalds
@ 2019-06-10 10:15       ` Jens Axboe
  2019-06-10 14:49         ` Paolo Valente
  0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2019-06-10 10:15 UTC (permalink / raw)
  To: Linus Torvalds, Tejun Heo, Li Zefan, Johannes Weiner; +Cc: linux-block

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.

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-10 10:15       ` Jens Axboe
@ 2019-06-10 14:49         ` Paolo Valente
  2019-06-10 15:48           ` Tejun Heo
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Valente @ 2019-06-10 14:49 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Linus Torvalds, Tejun Heo, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [GIT PULL] Block fixes for 5.2-rc4
  2019-06-10 14:49         ` Paolo Valente
@ 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
  0 siblings, 1 reply; 15+ messages in thread
From: Tejun Heo @ 2019-06-10 15:48 UTC (permalink / raw)
  To: Paolo Valente
  Cc: Jens Axboe, Linus Torvalds, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

Hello, Paolo.

On Mon, Jun 10, 2019 at 04:49:47PM +0200, Paolo Valente wrote:
> 1) General solution allowing multiple entities to share common files:
>    rejected by Tejun.

It was printing outputs from multiple policies for the same device in
the same file without any way of telling which one is actually being
used.  It can't work that way.  We can't show stuff which don't make
sense to users as a part of the kernel interface.  A general solution,
if we want to pursue, would be in this direction but something which
actually considers what's being used where, but an easier alternative
would be selecting the active policy system-wide.

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

Yeah, it'd either have to be 2) or 3) but I wasn't really engaged in
the discussion.  The simplest would be just renaming the interface
file given that that was ppl were using anyway.  Jens, if you aren't
too opposed to it, let's just rename bfq.weight to weight.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  2019-06-10 15:48           ` Tejun Heo
@ 2019-06-11 19:49             ` Tejun Heo
  2019-06-11 21:17               ` Tejun Heo
  0 siblings, 1 reply; 15+ messages in thread
From: Tejun Heo @ 2019-06-11 19:49 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jens Axboe, Paolo Valente, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

(Description mostly stolen from 19e9da9e86c4 ("block, bfq: add weight
symlink to the bfq.weight cgroup parameter")

Many userspace tools and services use the proportional-share policy of
the blkio/io cgroups controller. The CFQ I/O scheduler implemented
this policy for the legacy block layer. To modify the weight of a
group in case CFQ was in charge, the 'weight' parameter of the group
must be modified. On the other hand, the BFQ I/O scheduler implements
the same policy in blk-mq, but, with BFQ, the parameter to modify has
a different name: bfq.weight (forced choice until legacy block was
present, because two different policies cannot share a common
parameter in cgroups).

Due to CFQ legacy, most if not all userspace configurations still use
the parameter 'weight', and for the moment do not seem likely to be
changed. But, when CFQ went away with legacy block, such a parameter
ceased to exist.

19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup
parameter") tried to solve the problem by creating a symlink but there
doesn't seem to be a good reason for the added complexity.  Make bfq
simply create interface file io.weight instead of io.bfq.weight.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Paolo Valente <paolo.valente@linaro.org>
Cc: Angelo Ruocco <angeloruocco90@gmail.com>
---
 block/bfq-cgroup.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index b3796a40a61a..c68c6aa22154 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -1045,8 +1045,8 @@ struct blkcg_policy blkcg_policy_bfq = {
 
 struct cftype bfq_blkcg_legacy_files[] = {
 	{
-		.name = "bfq.weight",
-		.flags = CFTYPE_NOT_ON_ROOT,
+		.name = "io.weight",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NO_PREFIX,
 		.seq_show = bfq_io_show_weight,
 		.write_u64 = bfq_io_set_weight_legacy,
 	},
@@ -1165,8 +1165,8 @@ struct cftype bfq_blkcg_legacy_files[] = {
 
 struct cftype bfq_blkg_files[] = {
 	{
-		.name = "bfq.weight",
-		.flags = CFTYPE_NOT_ON_ROOT,
+		.name = "io.weight",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NO_PREFIX,
 		.seq_show = bfq_io_show_weight,
 		.write = bfq_io_set_weight,
 	},

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  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
  0 siblings, 1 reply; 15+ messages in thread
From: Tejun Heo @ 2019-06-11 21:17 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jens Axboe, Paolo Valente, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

On Tue, Jun 11, 2019 at 12:49:59PM -0700, Tejun Heo wrote:
> (Description mostly stolen from 19e9da9e86c4 ("block, bfq: add weight
> symlink to the bfq.weight cgroup parameter")
> 
> Many userspace tools and services use the proportional-share policy of
> the blkio/io cgroups controller. The CFQ I/O scheduler implemented
> this policy for the legacy block layer. To modify the weight of a
> group in case CFQ was in charge, the 'weight' parameter of the group
> must be modified. On the other hand, the BFQ I/O scheduler implements
> the same policy in blk-mq, but, with BFQ, the parameter to modify has
> a different name: bfq.weight (forced choice until legacy block was
> present, because two different policies cannot share a common
> parameter in cgroups).

Sorry, please don't apply this patch.  The cgroup interface currently
implemented by bfq doesn't follow the io.weight interface spec.  It's
different from cfq interface and symlinking to or renaming it won't do
anybody any good.

For now, the only thing we can do looks like keeping it as-is.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  2019-06-11 21:17               ` Tejun Heo
@ 2019-06-12  7:32                 ` Paolo Valente
  2019-06-12 13:39                   ` Tejun Heo
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Valente @ 2019-06-12  7:32 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Linus Torvalds, Jens Axboe, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

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



> Il giorno 11 giu 2019, alle ore 23:17, Tejun Heo <tj@kernel.org> ha scritto:
> 
> On Tue, Jun 11, 2019 at 12:49:59PM -0700, Tejun Heo wrote:
>> (Description mostly stolen from 19e9da9e86c4 ("block, bfq: add weight
>> symlink to the bfq.weight cgroup parameter")
>> 
>> Many userspace tools and services use the proportional-share policy of
>> the blkio/io cgroups controller. The CFQ I/O scheduler implemented
>> this policy for the legacy block layer. To modify the weight of a
>> group in case CFQ was in charge, the 'weight' parameter of the group
>> must be modified. On the other hand, the BFQ I/O scheduler implements
>> the same policy in blk-mq, but, with BFQ, the parameter to modify has
>> a different name: bfq.weight (forced choice until legacy block was
>> present, because two different policies cannot share a common
>> parameter in cgroups).
> 
> Sorry, please don't apply this patch.  The cgroup interface currently
> implemented by bfq doesn't follow the io.weight interface spec.

Could you elaborate a little more on this?

bfq code for setting up and handling io.weight (more precisely
io.bfq.weight) is a copy and paste of cfq code.

Thanks,
Paolo

> It's
> different from cfq interface and symlinking to or renaming it won't do
> anybody any good.
> 
> For now, the only thing we can do looks like keeping it as-is.
> 
> Thanks.
> 
> --
> tejun


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  2019-06-12  7:32                 ` Paolo Valente
@ 2019-06-12 13:39                   ` Tejun Heo
  2019-06-13  6:10                     ` Paolo Valente
  0 siblings, 1 reply; 15+ messages in thread
From: Tejun Heo @ 2019-06-12 13:39 UTC (permalink / raw)
  To: Paolo Valente
  Cc: Linus Torvalds, Jens Axboe, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

On Wed, Jun 12, 2019 at 09:32:07AM +0200, Paolo Valente wrote:
> Could you elaborate a little more on this?

Doesn't seem like you did.

> bfq code for setting up and handling io.weight (more precisely
> io.bfq.weight) is a copy and paste of cfq code.

Please take a look at the documentations under
Documentation/cgroup-v1/blk-iocontroller.txt and
Documentation/admin-guide/cgroup-v2.rst.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  2019-06-12 13:39                   ` Tejun Heo
@ 2019-06-13  6:10                     ` Paolo Valente
  2019-06-14 20:22                       ` Tejun Heo
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Valente @ 2019-06-13  6:10 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Linus Torvalds, Jens Axboe, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

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



> Il giorno 12 giu 2019, alle ore 15:39, Tejun Heo <tj@kernel.org> ha scritto:
> 
> On Wed, Jun 12, 2019 at 09:32:07AM +0200, Paolo Valente wrote:
>> Could you elaborate a little more on this?
> 
> Doesn't seem like you did.
> 
>> bfq code for setting up and handling io.weight (more precisely
>> io.bfq.weight) is a copy and paste of cfq code.
> 
> Please take a look at the documentations under
> Documentation/cgroup-v1/blk-iocontroller.txt and
> Documentation/admin-guide/cgroup-v2.rst.
> 

I'm sorry, but I don't see what doesn't match between BFQ's and CFQ's
implementations of the weight parameter.

So, if you agree, let me paste the two snippets for v1 that involve
weights.  So maybe you can at least tell me 'here' (then I'll try the
same with v2).

----------------------------------------------------------------------

	mount -t tmpfs cgroup_root /sys/fs/cgroup
	mkdir /sys/fs/cgroup/blkio
	mount -t cgroup -o blkio none /sys/fs/cgroup/blkio

- Create two cgroups
	mkdir -p /sys/fs/cgroup/blkio/test1/ /sys/fs/cgroup/blkio/test2

- Set weights of group test1 and test2
	echo 1000 > /sys/fs/cgroup/blkio/test1/blkio.weight
	echo 500 > /sys/fs/cgroup/blkio/test2/blkio.weight

-> This is exactly how you set weights with BFQ

----------------------------------------------------------------------

Details of cgroup files
=======================
Proportional weight policy files
--------------------------------
- blkio.weight
	- Specifies per cgroup weight. This is default weight of the group
	  on all the devices until and unless overridden by per device rule.
	  (See blkio.weight_device).
	  Currently allowed range of weights is from 10 to 1000.

-> This is the exact implementation of the weight parameter in BFQ

BFQ does not implement weight_device, but we are not talking about
weight_device here.  More precisely, *nothing* implements weight_device
any longer in cgroups-v1, so the documentation is wrong altogether.

Looking forward to your feedback,
Paolo



> Thanks.
> 
> --
> tejun


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  2019-06-13  6:10                     ` Paolo Valente
@ 2019-06-14 20:22                       ` Tejun Heo
  2019-06-14 21:05                         ` Paolo Valente
  0 siblings, 1 reply; 15+ messages in thread
From: Tejun Heo @ 2019-06-14 20:22 UTC (permalink / raw)
  To: Paolo Valente
  Cc: Linus Torvalds, Jens Axboe, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng

Hello,

On Thu, Jun 13, 2019 at 08:10:38AM +0200, Paolo Valente wrote:
> BFQ does not implement weight_device, but we are not talking about
> weight_device here.  More precisely, *nothing* implements weight_device
> any longer in cgroups-v1, so the documentation is wrong altogether.

I can't see how renaming an interface file which has different
behaviors on the same input to the same name is something acceptable.
Imagine how confusing that'd be to userspace.  If you want to rename
the control knob to io.weight, please implement full semantics for
both cgroup1 and cgroup2.

I just posted a separate io.weight implementation for cgroup2 but it's
easy to provide a way to override the interface files when bfq gets
selected, so let me know when you have a working implementation of the
interface.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight
  2019-06-14 20:22                       ` Tejun Heo
@ 2019-06-14 21:05                         ` Paolo Valente
  0 siblings, 0 replies; 15+ messages in thread
From: Paolo Valente @ 2019-06-14 21:05 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Linus Torvalds, Jens Axboe, Li Zefan, Johannes Weiner,
	linux-block, Johannes Thumshirn, Ulf Hansson, Linus Walleij,
	Fam Zheng



> Il giorno 14 giu 2019, alle ore 22:22, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello,
> 
> On Thu, Jun 13, 2019 at 08:10:38AM +0200, Paolo Valente wrote:
>> BFQ does not implement weight_device, but we are not talking about
>> weight_device here.  More precisely, *nothing* implements weight_device
>> any longer in cgroups-v1, so the documentation is wrong altogether.
> 
> I can't see how renaming an interface file which has different
> behaviors on the same input to the same name is something acceptable.
> Imagine how confusing that'd be to userspace.  If you want to rename
> the control knob to io.weight, please implement full semantics for
> both cgroup1 and cgroup2.
> 

I want to, and I've tried to make it easier for you to point me to
what is missing exactly.  Once again, the only missing piece I see are
per-device weights.

> I just posted a separate io.weight implementation for cgroup2 but it's
> easy to provide a way to override the interface files when bfq gets
> selected, so let me know when you have a working implementation of the
> interface.
> 

Ok, thanks.  I'll ping you when also per-device weights are available
in BFQ's interface.

Thanks,
Paolo

> Thanks.
> 
> -- 
> tejun


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-08  8:20 [GIT PULL] Block fixes for 5.2-rc4 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
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

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