Re: [PATCH] Revert "mm/page-writeback.c: print a warning if the vm dirtiness settings are illogical" (was: Re: [PATCH] mm: print a warning once the vm dirtiness settings is illogical)

[Jan Kara <jack@suse.cz>]

On Tue 28-11-17 15:52:50, Yafang Shao wrote:
> 2017-11-28 15:45 GMT+08:00 Michal Hocko <mhocko@suse.com>:
> > On Tue 28-11-17 14:12:15, Yafang Shao wrote:
> >> 2017-11-28 11:11 GMT+08:00 Yafang Shao <laoar.shao@gmail.com>:
> >> > Hi Michal,
> >> >
> >> > What about bellow change ?
> >> > It makes the function  domain_dirty_limits() more clear.
> >> > And the result will have a higher precision.
> >> >
> >> >
> >> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> >> > index 8a15511..2b5e507 100644
> >> > --- a/mm/page-writeback.c
> >> > +++ b/mm/page-writeback.c
> >> > @@ -397,8 +397,8 @@ static void domain_dirty_limits(struct
> >> > dirty_throttle_control *dtc)
> >> >     unsigned long bytes = vm_dirty_bytes;
> >> >     unsigned long bg_bytes = dirty_background_bytes;
> >> >     /* convert ratios to per-PAGE_SIZE for higher precision */
> >> > -   unsigned long ratio = (vm_dirty_ratio * PAGE_SIZE) / 100;
> >> > -   unsigned long bg_ratio = (dirty_background_ratio * PAGE_SIZE) / 100;
> >> > +   unsigned long ratio = vm_dirty_ratio;
> >> > +   unsigned long bg_ratio = dirty_background_ratio;
> >> >     unsigned long thresh;
> >> >     unsigned long bg_thresh;
> >> >     struct task_struct *tsk;
> >> > @@ -416,28 +416,33 @@ static void domain_dirty_limits(struct
> >> > dirty_throttle_control *dtc)
> >> >          */
> >> >         if (bytes)
> >> >             ratio = min(DIV_ROUND_UP(bytes, global_avail),
> >> > -                   PAGE_SIZE);
> >> > +                   100);
> >> >         if (bg_bytes)
> >> >             bg_ratio = min(DIV_ROUND_UP(bg_bytes, global_avail),
> >> > -                      PAGE_SIZE);
> >> > +                      99);   /* bg_ratio should less than ratio */
> >> >         bytes = bg_bytes = 0;
> >> >     }
> >>
> >>
> >> Errata:
> >>
> >>         if (bytes)
> >> -           ratio = min(DIV_ROUND_UP(bytes, global_avail),
> >> -                   PAGE_SIZE);
> >> +           ratio = min(DIV_ROUND_UP(bytes / PAGE_SIZE, global_avail),
> >> +                   100);
> >>         if (bg_bytes)
> >> -           bg_ratio = min(DIV_ROUND_UP(bg_bytes, global_avail),
> >> -                      PAGE_SIZE);
> >> +           bg_ratio = min(DIV_ROUND_UP(bg_bytes / PAGE_SIZE, global_avail),
> >> +                      100 - 1); /* bg_ratio should be less than ratio */
> >>         bytes = bg_bytes = 0;
> >
> > And you really think this makes code easier to follow? I am somehow not
> > conviced...
> >
> 
> There's hidden bug in the original code, because it is too complex to
> clearly understand.
> See bellow,
> 
> ratio = min(DIV_ROUND_UP(bytes, global_avail),
>                     PAGE_SIZE)
> 
> Suppose the vm_dirty_bytes is set to 512M (this is a reasonable
> value), and the global_avail is only 10000 pages (this is not low),
> then DIV_ROUND_UP(bytes, global_avail) is 53688, which is bigger than
> 4096, so the ratio will be 4096.
> That's unreasonable.

But that's not a bug in domain_dirty_limits(). It is more a design issue of
the dirty_bytes interface - i.e., if you tell the system that 512M of dirty
pages is fine, then it is fine even if you have only 400M of page cache -
i.e., 100% of page cache can be dirty and that's what the function
computes.  Bad luck if you don't like that but that's how the interface was
(mis)designed. We can talk about changes to what dirty_bytes mean under a
situation when there is low amount of page cache but that will be a
userspace visible change and we will have to be *very* careful not to break
current users.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>