* [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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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 related [flat|nested] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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 related [flat|nested] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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 2019-08-20 6:58 ` Paolo Valente 0 siblings, 1 reply; 16+ 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] 16+ messages in thread
* Re: [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight 2019-06-14 21:05 ` Paolo Valente @ 2019-08-20 6:58 ` Paolo Valente 0 siblings, 0 replies; 16+ messages in thread From: Paolo Valente @ 2019-08-20 6:58 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 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 > ^ permalink raw reply [flat|nested] 16+ 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; 16+ 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] 16+ messages in thread
end of thread, other threads:[~2019-08-20 6:58 UTC | newest] Thread overview: 16+ 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-08-20 6:58 ` Paolo Valente 2019-06-08 19:30 ` [GIT PULL] Block fixes for 5.2-rc4 pr-tracker-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).