| Message ID | 20190917165148.19146-3-paolo.valente@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | block, blkcg, bfq: make bfq disable iocost and delete bfq prefix from cgroup filenames | expand |
Looks good to me. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> On 09/17/2019 09:52 AM, Paolo Valente wrote: > From: Angelo Ruocco<angeloruocco90@gmail.com> > > When bfq was merged into mainline, there were two I/O schedulers that > implemented the proportional-share policy: bfq for blk-mq and cfq for > legacy blk. bfq's interface files in the blkio/io controller have the > same names as cfq. But the cgroups interface doesn't allow two > entities to use the same name for their files, so for bfq we had to > prepend the "bfq" prefix to each of its files. However no legacy code > uses these modified file names. This naming also causes confusion, as, > e.g., in [1]. > > Now cfq has gone with legacy blk, so there is no need any longer for > these prefixes in (the never used) bfq names. In view of this fact, this > commit removes these prefixes, thereby enabling legacy code to truly > use the proportional share policy in blk-mq. > > [1]https://github.com/systemd/systemd/issues/7057 > > Signed-off-by: Angelo Ruocco<angeloruocco90@gmail.com> > Signed-off-by: Paolo Valente<paolo.valente@linaro.org>
Hello, On Tue, Sep 17, 2019 at 06:51:48PM +0200, Paolo Valente wrote: > When bfq was merged into mainline, there were two I/O schedulers that > implemented the proportional-share policy: bfq for blk-mq and cfq for > legacy blk. bfq's interface files in the blkio/io controller have the > same names as cfq. But the cgroups interface doesn't allow two > entities to use the same name for their files, so for bfq we had to > prepend the "bfq" prefix to each of its files. However no legacy code > uses these modified file names. This naming also causes confusion, as, > e.g., in [1]. > > Now cfq has gone with legacy blk, so there is no need any longer for > these prefixes in (the never used) bfq names. In view of this fact, this > commit removes these prefixes, thereby enabling legacy code to truly > use the proportional share policy in blk-mq. So, I wrote the iocost switching patch and don't have a strong interest in whether bfq prefix should get dropped or not. However, I gotta point out that flipping interface this way is way out of the norm. In the previous release cycle, the right thing to do was dropping the bfq prefix but that wasn't possible because bfq's interface wasn't compatible at that point and didn't made to be compatible in time. Non-obviously different interface with the same name is a lot worse than giving it a new name, so the only acceptable course of action at that point was keeping the bfq prefix. Now that the interface has already been published in a released kernel, dropping the prefix would be something extremely unusual as there would already be users who will be affected by the interface flip-flop. We sometimes do change interfaces but I'm having a difficult time seeing the overriding rationales in this case. Thanks. -- tejun
> Il giorno 17 set 2019, alle ore 23:32, Tejun Heo <tj@kernel.org> ha scritto: > > Hello, > > On Tue, Sep 17, 2019 at 06:51:48PM +0200, Paolo Valente wrote: >> When bfq was merged into mainline, there were two I/O schedulers that >> implemented the proportional-share policy: bfq for blk-mq and cfq for >> legacy blk. bfq's interface files in the blkio/io controller have the >> same names as cfq. But the cgroups interface doesn't allow two >> entities to use the same name for their files, so for bfq we had to >> prepend the "bfq" prefix to each of its files. However no legacy code >> uses these modified file names. This naming also causes confusion, as, >> e.g., in [1]. >> >> Now cfq has gone with legacy blk, so there is no need any longer for >> these prefixes in (the never used) bfq names. In view of this fact, this >> commit removes these prefixes, thereby enabling legacy code to truly >> use the proportional share policy in blk-mq. > > So, I wrote the iocost switching patch and don't have a strong > interest in whether bfq prefix should get dropped or not. However, I > gotta point out that flipping interface this way is way out of the > norm. > > In the previous release cycle, the right thing to do was dropping the > bfq prefix but that wasn't possible because bfq's interface wasn't > compatible at that point and didn't made to be compatible in time. > Non-obviously different interface with the same name is a lot worse > than giving it a new name, so the only acceptable course of action at > that point was keeping the bfq prefix. > > Now that the interface has already been published in a released > kernel, dropping the prefix would be something extremely unusual as > there would already be users who will be affected by the interface > flip-flop. We sometimes do change interfaces but I'm having a > difficult time seeing the overriding rationales in this case. > This issue is a nightmare :) Userspace wants the weight to be called weight (I'm not reporting links to threads again). *Any* solution that gets to this is ok for me. A solution that both fulfills userspace request and doesn't break anything for hypothetical users of the current interface already made it to mainline, and Linus liked it too. It is: 19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup parameter") But it was then reverted on Tejun's request to do exactly what we don't want do any longer now: cf8929885de3 ("cgroup/bfq: revert bfq.weight symlink change") So, Jens, Tejun, can we please just revert that revert? Thanks, Paolo > Thanks. > > -- > tejun
Tejun, Paolo, On Tue, 17 Sep 2019 at 23:32, Tejun Heo <tj@kernel.org> wrote: > > Hello, > > On Tue, Sep 17, 2019 at 06:51:48PM +0200, Paolo Valente wrote: > > When bfq was merged into mainline, there were two I/O schedulers that > > implemented the proportional-share policy: bfq for blk-mq and cfq for > > legacy blk. bfq's interface files in the blkio/io controller have the > > same names as cfq. But the cgroups interface doesn't allow two > > entities to use the same name for their files, so for bfq we had to > > prepend the "bfq" prefix to each of its files. However no legacy code > > uses these modified file names. This naming also causes confusion, as, > > e.g., in [1]. > > > > Now cfq has gone with legacy blk, so there is no need any longer for > > these prefixes in (the never used) bfq names. In view of this fact, this > > commit removes these prefixes, thereby enabling legacy code to truly > > use the proportional share policy in blk-mq. > > So, I wrote the iocost switching patch and don't have a strong > interest in whether bfq prefix should get dropped or not. However, I > gotta point out that flipping interface this way is way out of the > norm. > > In the previous release cycle, the right thing to do was dropping the > bfq prefix but that wasn't possible because bfq's interface wasn't > compatible at that point and didn't made to be compatible in time. Sounds like we really should send those relevant patches for stable, to set the correct ground. Then using a symlink, to make sure we don't brake current ABI, right? [...] Kind regards Uffe
Hello, On Wed, Sep 18, 2019 at 07:18:50AM +0200, Paolo Valente wrote: > A solution that both fulfills userspace request and doesn't break > anything for hypothetical users of the current interface already made > it to mainline, and Linus liked it too. It is: Linus didn't like it. The implementation was a bit nasty. That was why it became a subject in the first place. > 19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup parameter") > > But it was then reverted on Tejun's request to do exactly what we > don't want do any longer now: > cf8929885de3 ("cgroup/bfq: revert bfq.weight symlink change") Note that the interface was wrong at the time too. > So, Jens, Tejun, can we please just revert that revert? I think presenting both io.weight and io.bfq.weight interfaces are probably the best course of action at this point but why does it have to be a symlink? What's wrong with just creating another file with the same backing function? Thanks. -- tejun
> Il giorno 18 set 2019, alle ore 17:19, Tejun Heo <tj@kernel.org> ha scritto: > > Hello, > > On Wed, Sep 18, 2019 at 07:18:50AM +0200, Paolo Valente wrote: >> A solution that both fulfills userspace request and doesn't break >> anything for hypothetical users of the current interface already made >> it to mainline, and Linus liked it too. It is: > > Linus didn't like it. The implementation was a bit nasty. That was > why it became a subject in the first place. > >> 19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup parameter") >> >> But it was then reverted on Tejun's request to do exactly what we >> don't want do any longer now: >> cf8929885de3 ("cgroup/bfq: revert bfq.weight symlink change") > > Note that the interface was wrong at the time too. > >> So, Jens, Tejun, can we please just revert that revert? > > I think presenting both io.weight and io.bfq.weight interfaces are > probably the best course of action at this point but why does it have > to be a symlink? What's wrong with just creating another file with > the same backing function? > I think a symlink would be much clearer for users, given the confusion already caused by two names for the same parameter. But let's hear others' opinion too. Thanks, Paolo > Thanks. > > -- > tejun
> Il giorno 18 set 2019, alle ore 18:19, Paolo Valente <paolo.valente@linaro.org> ha scritto: > > > >> Il giorno 18 set 2019, alle ore 17:19, Tejun Heo <tj@kernel.org> ha scritto: >> >> Hello, >> >> On Wed, Sep 18, 2019 at 07:18:50AM +0200, Paolo Valente wrote: >>> A solution that both fulfills userspace request and doesn't break >>> anything for hypothetical users of the current interface already made >>> it to mainline, and Linus liked it too. It is: >> >> Linus didn't like it. The implementation was a bit nasty. That was >> why it became a subject in the first place. >> >>> 19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup parameter") >>> >>> But it was then reverted on Tejun's request to do exactly what we >>> don't want do any longer now: >>> cf8929885de3 ("cgroup/bfq: revert bfq.weight symlink change") >> >> Note that the interface was wrong at the time too. >> >>> So, Jens, Tejun, can we please just revert that revert? >> >> I think presenting both io.weight and io.bfq.weight interfaces are >> probably the best course of action at this point but why does it have >> to be a symlink? What's wrong with just creating another file with >> the same backing function? >> > > I think a symlink would be much clearer for users, given the confusion > already caused by two names for the same parameter. But let's hear > others' opinion too. > Jens, could you express your opinion on this? Any solution you and Tejun agree on is ok for me. Also this new (fourth) possible implementation of this fix, provided that then it is definitely ok for both of you. Thanks, Paolo > Thanks, > Paolo > >> Thanks. >> >> -- >> tejun
On 9/20/19 12:58 AM, Paolo Valente wrote: > > >> Il giorno 18 set 2019, alle ore 18:19, Paolo Valente <paolo.valente@linaro.org> ha scritto: >> >> >> >>> Il giorno 18 set 2019, alle ore 17:19, Tejun Heo <tj@kernel.org> ha scritto: >>> >>> Hello, >>> >>> On Wed, Sep 18, 2019 at 07:18:50AM +0200, Paolo Valente wrote: >>>> A solution that both fulfills userspace request and doesn't break >>>> anything for hypothetical users of the current interface already made >>>> it to mainline, and Linus liked it too. It is: >>> >>> Linus didn't like it. The implementation was a bit nasty. That was >>> why it became a subject in the first place. >>> >>>> 19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup parameter") >>>> >>>> But it was then reverted on Tejun's request to do exactly what we >>>> don't want do any longer now: >>>> cf8929885de3 ("cgroup/bfq: revert bfq.weight symlink change") >>> >>> Note that the interface was wrong at the time too. >>> >>>> So, Jens, Tejun, can we please just revert that revert? >>> >>> I think presenting both io.weight and io.bfq.weight interfaces are >>> probably the best course of action at this point but why does it have >>> to be a symlink? What's wrong with just creating another file with >>> the same backing function? >>> >> >> I think a symlink would be much clearer for users, given the confusion >> already caused by two names for the same parameter. But let's hear >> others' opinion too. >> > > Jens, could you express your opinion on this? Any solution you and > Tejun agree on is ok for me. Also this new (fourth) possible > implementation of this fix, provided that then it is definitely ok for > both of you. Retaining both interfaces is arguably the right solution. It would be nice if we didn't have to, but the first bfq variant was incompatible with the in-kernel one, so we'll always have that out in the wild. Adding everything to stable doesn't work, as we still have existing kernels out there with the interface. In fact, in some ways that's worse, as you definitely don't want interfaces to change between two stable kernels. I know it's not ideal, and some better initial planning would have made it better, but we have to deal with the situation as it stands now. -- Jens Axboe
> Il giorno 20 set 2019, alle ore 15:05, Jens Axboe <axboe@kernel.dk> ha scritto: > > On 9/20/19 12:58 AM, Paolo Valente wrote: >> >> >>> Il giorno 18 set 2019, alle ore 18:19, Paolo Valente <paolo.valente@linaro.org> ha scritto: >>> >>> >>> >>>> Il giorno 18 set 2019, alle ore 17:19, Tejun Heo <tj@kernel.org> ha scritto: >>>> >>>> Hello, >>>> >>>> On Wed, Sep 18, 2019 at 07:18:50AM +0200, Paolo Valente wrote: >>>>> A solution that both fulfills userspace request and doesn't break >>>>> anything for hypothetical users of the current interface already made >>>>> it to mainline, and Linus liked it too. It is: >>>> >>>> Linus didn't like it. The implementation was a bit nasty. That was >>>> why it became a subject in the first place. >>>> >>>>> 19e9da9e86c4 ("block, bfq: add weight symlink to the bfq.weight cgroup parameter") >>>>> >>>>> But it was then reverted on Tejun's request to do exactly what we >>>>> don't want do any longer now: >>>>> cf8929885de3 ("cgroup/bfq: revert bfq.weight symlink change") >>>> >>>> Note that the interface was wrong at the time too. >>>> >>>>> So, Jens, Tejun, can we please just revert that revert? >>>> >>>> I think presenting both io.weight and io.bfq.weight interfaces are >>>> probably the best course of action at this point but why does it have >>>> to be a symlink? What's wrong with just creating another file with >>>> the same backing function? >>>> >>> >>> I think a symlink would be much clearer for users, given the confusion >>> already caused by two names for the same parameter. But let's hear >>> others' opinion too. >>> >> >> Jens, could you express your opinion on this? Any solution you and >> Tejun agree on is ok for me. Also this new (fourth) possible >> implementation of this fix, provided that then it is definitely ok for >> both of you. > > Retaining both interfaces is arguably the right solution. So you also are voting for BFQ to create two files, instead of having a symlink, aren't you? I just want to be certain before submitting one more solution. Looking forward to your confirmation, Paolo > It would be > nice if we didn't have to, but the first bfq variant was incompatible > with the in-kernel one, so we'll always have that out in the wild. > Adding everything to stable doesn't work, as we still have existing > kernels out there with the interface. In fact, in some ways that's > worse, as you definitely don't want interfaces to change between two > stable kernels. > > I know it's not ideal, and some better initial planning would have > made it better, but we have to deal with the situation as it stands > now. > > -- > Jens Axboe
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index decda96770f4..7f0160f5155f 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -1213,7 +1213,7 @@ struct blkcg_policy blkcg_policy_bfq = { struct cftype bfq_blkcg_legacy_files[] = { { - .name = "bfq.weight", + .name = "weight", .flags = CFTYPE_NOT_ON_ROOT, .seq_show = bfq_io_show_weight_legacy, .write_u64 = bfq_io_set_weight_legacy, @@ -1227,42 +1227,42 @@ struct cftype bfq_blkcg_legacy_files[] = { /* statistics, covers only the tasks in the bfqg */ { - .name = "bfq.io_service_bytes", + .name = "io_service_bytes", .private = (unsigned long)&blkcg_policy_bfq, .seq_show = blkg_print_stat_bytes, }, { - .name = "bfq.io_serviced", + .name = "io_serviced", .private = (unsigned long)&blkcg_policy_bfq, .seq_show = blkg_print_stat_ios, }, #ifdef CONFIG_BFQ_CGROUP_DEBUG { - .name = "bfq.time", + .name = "time", .private = offsetof(struct bfq_group, stats.time), .seq_show = bfqg_print_stat, }, { - .name = "bfq.sectors", + .name = "sectors", .seq_show = bfqg_print_stat_sectors, }, { - .name = "bfq.io_service_time", + .name = "io_service_time", .private = offsetof(struct bfq_group, stats.service_time), .seq_show = bfqg_print_rwstat, }, { - .name = "bfq.io_wait_time", + .name = "io_wait_time", .private = offsetof(struct bfq_group, stats.wait_time), .seq_show = bfqg_print_rwstat, }, { - .name = "bfq.io_merged", + .name = "io_merged", .private = offsetof(struct bfq_group, stats.merged), .seq_show = bfqg_print_rwstat, }, { - .name = "bfq.io_queued", + .name = "io_queued", .private = offsetof(struct bfq_group, stats.queued), .seq_show = bfqg_print_rwstat, }, @@ -1270,66 +1270,66 @@ struct cftype bfq_blkcg_legacy_files[] = { /* the same statistics which cover the bfqg and its descendants */ { - .name = "bfq.io_service_bytes_recursive", + .name = "io_service_bytes_recursive", .private = (unsigned long)&blkcg_policy_bfq, .seq_show = blkg_print_stat_bytes_recursive, }, { - .name = "bfq.io_serviced_recursive", + .name = "io_serviced_recursive", .private = (unsigned long)&blkcg_policy_bfq, .seq_show = blkg_print_stat_ios_recursive, }, #ifdef CONFIG_BFQ_CGROUP_DEBUG { - .name = "bfq.time_recursive", + .name = "time_recursive", .private = offsetof(struct bfq_group, stats.time), .seq_show = bfqg_print_stat_recursive, }, { - .name = "bfq.sectors_recursive", + .name = "sectors_recursive", .seq_show = bfqg_print_stat_sectors_recursive, }, { - .name = "bfq.io_service_time_recursive", + .name = "io_service_time_recursive", .private = offsetof(struct bfq_group, stats.service_time), .seq_show = bfqg_print_rwstat_recursive, }, { - .name = "bfq.io_wait_time_recursive", + .name = "io_wait_time_recursive", .private = offsetof(struct bfq_group, stats.wait_time), .seq_show = bfqg_print_rwstat_recursive, }, { - .name = "bfq.io_merged_recursive", + .name = "io_merged_recursive", .private = offsetof(struct bfq_group, stats.merged), .seq_show = bfqg_print_rwstat_recursive, }, { - .name = "bfq.io_queued_recursive", + .name = "io_queued_recursive", .private = offsetof(struct bfq_group, stats.queued), .seq_show = bfqg_print_rwstat_recursive, }, { - .name = "bfq.avg_queue_size", + .name = "avg_queue_size", .seq_show = bfqg_print_avg_queue_size, }, { - .name = "bfq.group_wait_time", + .name = "group_wait_time", .private = offsetof(struct bfq_group, stats.group_wait_time), .seq_show = bfqg_print_stat, }, { - .name = "bfq.idle_time", + .name = "idle_time", .private = offsetof(struct bfq_group, stats.idle_time), .seq_show = bfqg_print_stat, }, { - .name = "bfq.empty_time", + .name = "empty_time", .private = offsetof(struct bfq_group, stats.empty_time), .seq_show = bfqg_print_stat, }, { - .name = "bfq.dequeue", + .name = "dequeue", .private = offsetof(struct bfq_group, stats.dequeue), .seq_show = bfqg_print_stat, }, @@ -1339,7 +1339,7 @@ struct cftype bfq_blkcg_legacy_files[] = { struct cftype bfq_blkg_files[] = { { - .name = "bfq.weight", + .name = "weight", .flags = CFTYPE_NOT_ON_ROOT, .seq_show = bfq_io_show_weight, .write = bfq_io_set_weight,