Skip to contentSkip to conversation

bdi_min_ratio never shrinks, ultimately preventing valid setting of min_ratio

DISCUSSION [email protected] 3 participants 7 messages Jun 1, 2011 – Jun 8, 2011

Subsystems (2)
Memory management MM misc
lore
Share & tools
lore

[email protected]

author
#0 lore raw

mm/page-writeback.c
There is a static global bdi_min_ratio used for policing the setting of per-bdi min_ratio's, to ensure the sum doesn't cross 100.

There is no place in this listing where the value is decremented by the respective bdi's min_ratio when a bdi is torn down. This looks like a bug to me, and I have a situation where I'm unable to set a min_ratio to 1 where the sum of /sys/class/bdi/*/min_ratio does not add up to 100, which is what triggered this investigation.

Regards,
Vito Caputo

#1 lore raw

It really helps if you CC the relevant people.

Quoted 6 lines · On Tue, 2011-05-31 at 19:28 -0500, [email protected] wrote:

mm/page-writeback.c
There is a static global bdi_min_ratio used for policing the setting of per-bdi min_ratio's, to ensure the sum doesn't cross 100.

There is no place in this listing where the value is decremented by the respective bdi's min_ratio when a bdi is torn down.

There is, adding a negative number is equal to a subtraction.

                min_ratio -= bdi->min_ratio;
                if (bdi_min_ratio + min_ratio < 100) {
                        bdi_min_ratio += min_ratio;
                        bdi->min_ratio += min_ratio;
                }

is the relevant piece, note that bdi->min_ratio is the current setting, this makes min_ratio the difference between the new and old setting, and adding this to both bdi_min_ratio (the global sum) and bdi->min_ratio dtrt regardless if the new value is larger or smaller than the old value.

Quoted 4 lines

This looks like a bug
to me, and I have a situation where I'm unable to set a min_ratio to 1 where the sum of /sys/class/bdi/*/min_ratio does not add up to 100, which is what triggered this investigation.

Which of the two -EINVAL cases is triggered?

author
#2 lore raw
Quoted 18 lines · Peter Zijlstra

It really helps if you CC the relevant people.

Quoted 6 lines · On Tue, 2011-05-31 at 19:28 -0500, [email protected] wrote:

mm/page-writeback.c
There is a static global bdi_min_ratio used for policing the setting of per-bdi min_ratio's, to ensure the sum doesn't cross 100.

There is no place in this listing where the value is decremented by the respective bdi's min_ratio when a bdi is torn down.

There is, adding a negative number is equal to a subtraction.

                min_ratio -= bdi->min_ratio;
                if (bdi_min_ratio + min_ratio < 100) {
                        bdi_min_ratio += min_ratio;
                        bdi->min_ratio += min_ratio;
                }
Quoted 5 lines

is the relevant piece, note that bdi->min_ratio is the current setting, this makes min_ratio the difference between the new and old setting, and adding this to both bdi_min_ratio (the global sum) and bdi->min_ratio dtrt regardless if the new value is larger or smaller than the old value.

This accounts for the repeated setting of min_ratio on the same bdi. But does bdi_set_min_ratio() get entered with a min_ratio of 0 on bdi removal? If not, we leak the non-zero min_ratio of a removed bdi.

Quoted 7 lines
Quoted 5 lines

This looks like a bug
to me, and I have a situation where I'm unable to set a min_ratio to 1 where the sum of /sys/class/bdi/*/min_ratio does not add up to 100, which is what triggered this investigation.

Which of the two -EINVAL cases is triggered?

It's the bdi_min_ratio + min_ratio >= 100 case.

The system has many usb disks coming and going day to day, with their respective bdi's having min_ratio set to 1 when inserted. It works for some time until eventually min_ratio can no longer be set, even when the active set of bdi's seen in /sys/class/bdi/*/min_ratio doesn't add up to anywhere near 100.

This then leads to an unrelated starvation problem caused by write-heavy fuse mounts being used atop the usb disks, a problem the min_ratio setting at the underlying devices bdi effectively prevents.

Regards,
Vito Caputo

#3 lore raw
Quoted 17 lines · On Thu, 2011-06-02 at 13:32 -0500, [email protected] wrote:
Quoted 11 lines
Quoted 2 lines

There is no place in this listing where the value is decremented by the respective bdi's min_ratio when a bdi is torn down.

There is, adding a negative number is equal to a subtraction.

                min_ratio -= bdi->min_ratio;
                if (bdi_min_ratio + min_ratio < 100) {
                        bdi_min_ratio += min_ratio;
                        bdi->min_ratio += min_ratio;
                }
Quoted 5 lines

is the relevant piece, note that bdi->min_ratio is the current setting, this makes min_ratio the difference between the new and old setting, and adding this to both bdi_min_ratio (the global sum) and bdi->min_ratio dtrt regardless if the new value is larger or smaller than the old value.

Quoted 3 lines

This accounts for the repeated setting of min_ratio on the same bdi. But does bdi_set_min_ratio() get entered with a min_ratio of 0 on bdi removal? If not, we leak the non-zero min_ratio of a removed bdi.

That does not appear to be the case, good catch. Would you be bitten by that particular scenario? If so, does the below cure things for you?

---
 mm/backing-dev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
mm/backing-dev.c +1 -0
index f032e6e..e56fe35 100644
@@ -606,6 +606,7 @@ static void bdi_prune_sb(struct backing_dev_info *bdi)
 void bdi_unregister(struct backing_dev_info *bdi)
 {
 	if (bdi->dev) {
+		bdi_set_min_ratio(bdi, 0);
 		trace_writeback_bdi_unregister(bdi);
 		bdi_prune_sb(bdi);
 		del_timer_sync(&bdi->wb.wakeup_timer);
maintainer
#4 lore raw
Quoted 34 lines · On Thu, 02 Jun 2011 23:25:58 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 17 lines · On Thu, 2011-06-02 at 13:32 -0500, [email protected] wrote:
Quoted 11 lines
Quoted 2 lines

There is no place in this listing where the value is decremented by the respective bdi's min_ratio when a bdi is torn down.

There is, adding a negative number is equal to a subtraction.

                min_ratio -= bdi->min_ratio;
                if (bdi_min_ratio + min_ratio < 100) {
                        bdi_min_ratio += min_ratio;
                        bdi->min_ratio += min_ratio;
                }
Quoted 5 lines

is the relevant piece, note that bdi->min_ratio is the current setting, this makes min_ratio the difference between the new and old setting, and adding this to both bdi_min_ratio (the global sum) and bdi->min_ratio dtrt regardless if the new value is larger or smaller than the old value.

Quoted 3 lines

This accounts for the repeated setting of min_ratio on the same bdi. But does bdi_set_min_ratio() get entered with a min_ratio of 0 on bdi removal? If not, we leak the non-zero min_ratio of a removed bdi.

That does not appear to be the case, good catch. Would you be bitten by that particular scenario? If so, does the below cure things for you?

---
 mm/backing-dev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
mm/backing-dev.c +0 -0
index f032e6e..e56fe35 100644

mm/backing-dev.c static void bdi_prune_sb(struct backing_dev_info *bdi) · line 612

Quoted 7 lines
mm/backing-dev.c +1 -0
@@ -606,6 +606,7 @@ static void bdi_prune_sb(struct backing_dev_info *bdi)
 void bdi_unregister(struct backing_dev_info *bdi)
 {
 	if (bdi->dev) {
+		bdi_set_min_ratio(bdi, 0);
 		trace_writeback_bdi_unregister(bdi);
 		bdi_prune_sb(bdi);
 		del_timer_sync(&bdi->wb.wakeup_timer);

I grabbed this, wrote a changelog and stuck your signed-off-by on it. Vito, it would be great if you are able to test this please.

I also added a cc:stable but I didn't work out how far back in time it goes. A long way, I think?

#5 lore raw
Quoted 1 line · Andrew Morton

I grabbed this, wrote a changelog and stuck your signed-off-by on it.

Thanks, I was still waiting for feedback :/

Quoted 2 lines

I also added a cc:stable but I didn't work out how far back in time it goes. A long way, I think?

Yeah, ages ago:

commit 189d3c4a94ef19fca2a71a6a336e9fda900e25e7

Author: Peter Zijlstra <[email protected]>
Date:   Wed Apr 30 00:54:35 2008 -0700

mm: bdi: allow setting a minimum for the bdi dirty limit

git describe --contains doesn't seem to want to give a -linus release, my git foo is too weak :-(

#6 lore raw
Quoted 11 lines · Peter Zijlstra
Quoted 1 line · Andrew Morton

I grabbed this, wrote a changelog and stuck your signed-off-by on it.

Thanks, I was still waiting for feedback :/

Quoted 2 lines

I also added a cc:stable but I didn't work out how far back in time it goes. A long way, I think?

Yeah, ages ago:

Quoted 8 lines

commit 189d3c4a94ef19fca2a71a6a336e9fda900e25e7

Author: Peter Zijlstra <[email protected]>
Date:   Wed Apr 30 00:54:35 2008 -0700

mm: bdi: allow setting a minimum for the bdi dirty limit

git describe --contains doesn't seem to want to give a -linus release, my git foo is too weak :-(

OK after it was pointed out that --match takes a glob, not a regex, it gives:

# git describe --contains  189d3c4a94ef19fca2a71a6a336e9fda900e25e7 --match 'v*'
v2.6.26-rc1~155

So yeah, _waaaay_ back.