Re: [PATCH] mm: add strictlimit knob -v2

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 06 Nov 2013 19:05:57 +0400 Maxim Patlasov <MPatlasov@parallels.com> wrote:

> "strictlimit" feature was introduced to enforce per-bdi dirty limits for
> FUSE which sets bdi max_ratio to 1% by default:
> 
> http://article.gmane.org/gmane.linux.kernel.mm/105809
> 
> However the feature can be useful for other relatively slow or untrusted
> BDIs like USB flash drives and DVD+RW. The patch adds a knob to enable the
> feature:
> 
> echo 1 > /sys/class/bdi/X:Y/strictlimit
> 
> Being enabled, the feature enforces bdi max_ratio limit even if global (10%)
> dirty limit is not reached. Of course, the effect is not visible until
> /sys/class/bdi/X:Y/max_ratio is decreased to some reasonable value.
> 
> ...
>
> --- a/Documentation/ABI/testing/sysfs-class-bdi
> +++ b/Documentation/ABI/testing/sysfs-class-bdi
> @@ -53,3 +53,11 @@ stable_pages_required (read-only)
>  
>  	If set, the backing device requires that all pages comprising a write
>  	request must not be changed until writeout is complete.
> +
> +strictlimit (read-write)
> +
> +	Forces per-BDI checks for the share of given device in the write-back
> +	cache even before the global background dirty limit is reached. This
> +	is useful in situations where the global limit is much higher than
> +	affordable for given relatively slow (or untrusted) device. Turning
> +	strictlimit on has no visible effect if max_ratio is equal to 100%.
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index ce682f7..4ee1d64 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -234,11 +234,46 @@ static ssize_t stable_pages_required_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(stable_pages_required);
>  
> +static ssize_t strictlimit_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +	unsigned int val;
> +	ssize_t ret;
> +
> +	ret = kstrtouint(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (val) {
> +	case 0:
> +		bdi->capabilities &= ~BDI_CAP_STRICTLIMIT;
> +		break;
> +	case 1:
> +		bdi->capabilities |= BDI_CAP_STRICTLIMIT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +static ssize_t strictlimit_show(struct device *dev,
> +		struct device_attribute *attr, char *page)
> +{
> +	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +
> +	return snprintf(page, PAGE_SIZE-1, "%d\n",
> +			!!(bdi->capabilities & BDI_CAP_STRICTLIMIT));
> +}
> +static DEVICE_ATTR_RW(strictlimit);
> +
>  static struct attribute *bdi_dev_attrs[] = {
>  	&dev_attr_read_ahead_kb.attr,
>  	&dev_attr_min_ratio.attr,
>  	&dev_attr_max_ratio.attr,
>  	&dev_attr_stable_pages_required.attr,
> +	&dev_attr_strictlimit.attr,
>  	NULL,

Well the patch is certainly simple and straightforward enough and
*seems* like it will be useful.  The main (and large!) downside is that
it adds to the user interface so we'll have to maintain this feature
and its functionality for ever.

Given this, my concern is that while potentially useful, the feature
might not be *sufficiently* useful to justify its inclusion.  So we'll
end up addressing these issues by other means, then we're left
maintaining this obsolete legacy feature.

So I'm thinking that unless someone can show that this is good and
complete and sufficient for a "large enough" set of issues, I'll take a
pass on the patch[1].  What do people think?


[1] Actually, I'll stick it in -mm and maintain it, so next time
someone reports an issue I can say "hey, try this".

--
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>