On Wed 09-03-11 16:02:53, Vivek Goyal wrote:
> On Tue, Mar 08, 2011 at 11:31:12PM +0100, Jan Kara wrote:
> > @@ -291,6 +292,12 @@ static unsigned long task_dirty_limit(struct task_struct *tsk,
> > return max(dirty, bdi_dirty/2);
> > }
> >
> > +/* Minimum limit for any task */
> > +static unsigned long task_min_dirty_limit(unsigned long bdi_dirty)
> > +{
> > + return bdi_dirty - bdi_dirty / TASK_LIMIT_FRACTION;
> > +}
> > +
> Should the above be called bdi_min_dirty_limit()? In essense we seem to
> be setting bdi->bdi_exceeded when dirty pages on bdi cross bdi_thresh and
> clear it when dirty pages on bdi are below 7/8*bdi_thresh. So there does
> not seem to be any dependency on task dirty limit here hence string
> "task" sounds confusing to me. In fact, would
> bdi_dirty_exceeded_clear_thresh() be a better name?
See below...
> > /*
> > *
> > */
> > @@ -484,9 +491,11 @@ static void balance_dirty_pages(struct address_space *mapping,
> > unsigned long background_thresh;
> > unsigned long dirty_thresh;
> > unsigned long bdi_thresh;
> > + unsigned long min_bdi_thresh = ULONG_MAX;
> > unsigned long pages_written = 0;
> > unsigned long pause = 1;
> > bool dirty_exceeded = false;
> > + bool min_dirty_exceeded = false;
> > struct backing_dev_info *bdi = mapping->backing_dev_info;
> >
> > for (;;) {
> > @@ -513,6 +522,7 @@ static void balance_dirty_pages(struct address_space *mapping,
> > break;
> >
> > bdi_thresh = bdi_dirty_limit(bdi, dirty_thresh);
> > + min_bdi_thresh = task_min_dirty_limit(bdi_thresh);
> > bdi_thresh = task_dirty_limit(current, bdi_thresh);
> ^^^^^
> This patch aside, we use bdi_thresh name both for bdi threshold as well
> as per task per bdi threshold. will task_bdi_thresh be a better name
> here.
I agree that the naming is a bit confusing altough it is traditional :).
The renaming to task_bdi_thresh makes sense to me. Then we could name the
limit when we clear dirty_exceeded as: min_task_bdi_thresh(). The task in
the name tries to say that this is a limit for "any task" so I'd like to
keep it there. What do you think?
> > @@ -542,6 +552,9 @@ static void balance_dirty_pages(struct address_space *mapping,
> > dirty_exceeded =
> > (bdi_nr_reclaimable + bdi_nr_writeback > bdi_thresh)
> > || (nr_reclaimable + nr_writeback > dirty_thresh);
> > + min_dirty_exceeded =
> > + (bdi_nr_reclaimable + bdi_nr_writeback > min_bdi_thresh)
> > + || (nr_reclaimable + nr_writeback > dirty_thresh);
>
> Would following be easier to understand.
>
> clear_dirty_exceeded =
> (bdi_nr_reclaimable + bdi_nr_writeback <
> dirty_exceeded_reset_thresh)
> && (nr_reclaimable + nr_writeback < dirty_thresh);
Yes, this looks better. I'll change it.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
