Re: [PATCH 6/6] mm: per device dirty threshold

[Miklos Szeredi]

> > I'm worried about two things:
> > 
> > 1) If the per-bdi threshold becomes smaller than the granularity of
> >    the per-bdi stat (due to the per-CPU counters), then things will
> >    break.  Shouldn't there be some sanity checking for the calculated
> >    threshold?
> 
> I'm not sure what you're referring to.
> 
> void get_writeout_scale(struct backing_dev_info *bdi, int *scale, int *div)
> {
>         int bits = vm_cycle_shift - 1;
>         unsigned long total = __global_bdi_stat(BDI_WRITEOUT_TOTAL);
>         unsigned long cycle = 1UL << bits;
>         unsigned long mask = cycle - 1;
> 
>         if (bdi_cap_writeback_dirty(bdi)) {
>                 bdi_writeout_norm(bdi);
>                 *scale = __bdi_stat(bdi, BDI_WRITEOUT);
>         } else
>                 *scale = 0;
> 
>         *div = cycle + (total & mask);
> }
> 
> where cycle ~ vm_total_pages
> scale can be a tad off due to overstep here:
> 
> void __inc_bdi_stat(struct backing_dev_info *bdi, enum bdi_stat_item item)
> {
>         struct bdi_per_cpu_data *pcd = &bdi->pcd[smp_processor_id()];
>         s8 *p = pcd->bdi_stat_diff + item;
> 
>         (*p)++;
> 
>         if (unlikely(*p > pcd->stat_threshold)) {
>                 int overstep = pcd->stat_threshold / 2;
> 
>                 bdi_stat_add(*p + overstep, bdi, item);
>                 *p = -overstep;
>         }
> }
> 
> so it could be that: scale / cycle > 1
> by a very small amount; however:

No, I'm worried about the case when scale is too small.  If the
per-bdi threshold becomes smaller than stat_threshold, then things
won't work, because dirty+writeback will never go below the threshold,
possibly resulting in the deadlock we are trying to avoid.

BTW, the second argument of get_dirty_limits() doesn't seem to get
used by the caller, or does it?

> here we clip to 'reserve' which is the total amount of dirty threshold
> not dirty by others.
> 
> > 2) The loop is sleeping in congestion_wait(WRITE), which seems wrong.
> >    It may well be possible that none of the queues are congested, so
> >    it will sleep the full .1 second.  But by that time the queue may
> >    have become idle and is just sitting there doing nothing.  Maybe
> >    there should be a per-bdi waitq, that is woken up, when the per-bdi
> >    stats are updated.
> 
> Good point, .1 seconds is a lot of time.
> 
> I'll cook up something like that if nobody beats me to it :-)

I realized, that it's maybe worth storing last the threshold in the
bdi as well, so that balance_dirty_pages() doesn't get woken up too
many times unnecessarilty.  But I don't know...

Thanks,
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/