[GIT PULL] Block fixes for 5.2-rc4

[GIT PULL] Block fixes for 5.2-rc4

[Jens Axboe]

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

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

[Linus Torvalds]

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

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

[pr-tracker-bot]

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

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

[Jens Axboe]

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

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

[Linus Torvalds]

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

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

[Jens Axboe]

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

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

[Paolo Valente]

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

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

[Tejun Heo]

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

[PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Tejun Heo]

(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,
 	},

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Tejun Heo]

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

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Paolo Valente]

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

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Tejun Heo]

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

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Paolo Valente]

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

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Tejun Heo]

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

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Paolo Valente]

> 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

Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight

[Paolo Valente]

> Il giorno 14 giu 2019, alle ore 23:05, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
> 
> 
>> 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.
> 

Hi Tejun,
have you seen the patch introducing per-device weights [1]?

[1] https://lkml.org/lkml/2019/8/5/83

Thanks,
Paolo

> Thanks,
> Paolo
> 
>> Thanks.
>> 
>> -- 
>> tejun
>