From: Andrew Morton <akpm@linux-foundation.org>
To: Maxim Patlasov <MPatlasov@parallels.com>
Cc: miklos@szeredi.hu, riel@redhat.com, dev@parallels.com,
xemul@parallels.com, fuse-devel@lists.sourceforge.net,
bfoster@redhat.com, linux-kernel@vger.kernel.org,
jbottomley@parallels.com, linux-mm@kvack.org,
viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
fengguang.wu@intel.com, devel@openvz.org, mgorman@suse.de
Subject: Re: [PATCH 16/16] mm: strictlimit feature
Date: Mon, 1 Jul 2013 14:16:12 -0700 [thread overview]
Message-ID: <20130701141612.04d867863319bcc23d007a23@linux-foundation.org> (raw)
In-Reply-To: <20130629174706.20175.78184.stgit@maximpc.sw.ru>
On Sat, 29 Jun 2013 21:48:54 +0400 Maxim Patlasov <MPatlasov@parallels.com> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> The feature prevents mistrusted filesystems to grow a large number of dirty
> pages before throttling. For such filesystems balance_dirty_pages always
> check bdi counters against bdi limits. I.e. even if global "nr_dirty" is under
> "freerun", it's not allowed to skip bdi checks. The only use case for now is
> fuse: it sets bdi max_ratio to 1% by default and system administrators are
> supposed to expect that this limit won't be exceeded.
>
> The feature is on if address space is marked by AS_STRICTLIMIT flag.
> A filesystem may set the flag when it initializes a new inode.
>
Fengguang, could you please review this patch?
I suggest you await the next version, which hopefully will be more
reviewable...
>
> ...
>
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -33,6 +33,8 @@ enum bdi_state {
> BDI_sync_congested, /* The sync queue is getting full */
> BDI_registered, /* bdi_register() was done */
> BDI_writeback_running, /* Writeback is in progress */
> + BDI_idle, /* No pages under writeback at the moment of
> + * last update of write bw */
Why does BDI_idle exist?
> BDI_unused, /* Available bits start here */
> };
>
> @@ -43,6 +45,7 @@ enum bdi_stat_item {
> BDI_WRITEBACK,
> BDI_DIRTIED,
> BDI_WRITTEN,
> + BDI_WRITTEN_BACK,
> NR_BDI_STAT_ITEMS
> };
>
> @@ -76,6 +79,8 @@ struct backing_dev_info {
> unsigned long bw_time_stamp; /* last time write bw is updated */
> unsigned long dirtied_stamp;
> unsigned long written_stamp; /* pages written at bw_time_stamp */
> + unsigned long writeback_stamp; /* pages sent to writeback at
> + * bw_time_stamp */
Well this sucks. Some of the "foo_stamp" fields are in units of time
(jiffies? We aren't told) and some of the "foo_stamp" fields are in
units of number-of-pages. It would be good to fix the naming here.
> unsigned long write_bandwidth; /* the estimated write bandwidth */
> unsigned long avg_write_bandwidth; /* further smoothed write bw */
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index e3dea75..baac702 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -25,6 +25,7 @@ enum mapping_flags {
> AS_MM_ALL_LOCKS = __GFP_BITS_SHIFT + 2, /* under mm_take_all_locks() */
> AS_UNEVICTABLE = __GFP_BITS_SHIFT + 3, /* e.g., ramdisk, SHM_LOCK */
> AS_BALLOON_MAP = __GFP_BITS_SHIFT + 4, /* balloon page special map */
> + AS_STRICTLIMIT = __GFP_BITS_SHIFT + 5, /* strict dirty limit */
Thing is, "strict dirty limit" isn't documented anywhere, so this
reference is left dangling.
>
> ...
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -94,6 +94,7 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
> "BackgroundThresh: %10lu kB\n"
> "BdiDirtied: %10lu kB\n"
> "BdiWritten: %10lu kB\n"
> + "BdiWrittenBack: %10lu kB\n"
> "BdiWriteBandwidth: %10lu kBps\n"
> "b_dirty: %10lu\n"
> "b_io: %10lu\n"
I can't imagine what the difference is between BdiWritten and
BdiWrittenBack.
I suggest you document this at the BDI_WRITTEN_BACK definition site in
enum bdi_stat_item. BDI_WRITTEN (at least) will also need
documentation so people can understand the difference.
>
> ...
>
> @@ -679,29 +711,31 @@ static unsigned long bdi_position_ratio(struct backing_dev_info *bdi,
> if (unlikely(dirty >= limit))
> return 0;
>
> + if (unlikely(strictlimit)) {
> + if (bdi_dirty < 8)
> + return 2 << RATELIMIT_CALC_SHIFT;
> +
> + if (bdi_dirty >= bdi_thresh)
> + return 0;
> +
> + bdi_setpoint = bdi_thresh + bdi_dirty_limit(bdi, bg_thresh);
> + bdi_setpoint /= 2;
> +
> + if (bdi_setpoint == 0 || bdi_setpoint == bdi_thresh)
> + return 0;
> +
> + pos_ratio = pos_ratio_polynom(bdi_setpoint, bdi_dirty,
> + bdi_thresh);
> + return min_t(long long, pos_ratio, 2 << RATELIMIT_CALC_SHIFT);
> + }
This would be a suitable site at which to document the strictlimit
feature. What it is, how it works and most importantly, why it exists.
>
> ...
>
> @@ -994,6 +1029,16 @@ static void bdi_update_dirty_ratelimit(struct backing_dev_info *bdi,
> * keep that period small to reduce time lags).
> */
> step = 0;
> +
> + if (unlikely(strictlimit)) {
> + dirty = bdi_dirty;
> + if (bdi_dirty < 8)
> + setpoint = bdi_dirty + 1;
> + else
> + setpoint = (bdi_thresh +
> + bdi_dirty_limit(bdi, bg_thresh)) / 2;
> + }
Explain this to the reader, please.
>
> ...
>
--
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>
next prev parent reply other threads:[~2013-07-01 21:16 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-29 17:41 [PATCH v5 00/16] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
2013-06-29 17:42 ` [PATCH 01/16] fuse: Linking file to inode helper Maxim Patlasov
2013-06-29 17:42 ` [PATCH 02/16] fuse: Getting file for writeback helper Maxim Patlasov
2013-06-29 17:42 ` [PATCH 04/16] fuse: Prepare to handle multiple pages in writeback Maxim Patlasov
2013-06-29 17:42 ` [PATCH 05/16] fuse: Connection bit for enabling writeback Maxim Patlasov
2013-06-29 17:44 ` [PATCH 06/16] fuse: Trust kernel i_size only - v4 Maxim Patlasov
2013-06-29 17:44 ` [PATCH 07/16] fuse: Trust kernel i_mtime only Maxim Patlasov
2013-07-11 16:14 ` [PATCH 07/16] fuse: Trust kernel i_mtime only -v2 Maxim Patlasov
2013-06-29 17:45 ` [PATCH 08/16] fuse: Flush files on wb close Maxim Patlasov
2013-07-11 16:18 ` [PATCH 08/16] fuse: Flush files on wb close -v2 Maxim Patlasov
[not found] ` <20130629172211.20175.70154.stgit-vWG5eQQidJHciZdyczg/7Q@public.gmane.org>
2013-06-29 17:42 ` [PATCH 03/16] fuse: Prepare to handle short reads Maxim Patlasov
2013-06-29 17:45 ` [PATCH 09/16] fuse: restructure fuse_readpage() Maxim Patlasov
2013-06-29 17:45 ` [PATCH 10/16] fuse: Implement writepages callback Maxim Patlasov
2013-07-19 16:50 ` Miklos Szeredi
2013-08-02 15:40 ` Maxim Patlasov
2013-08-06 16:25 ` Miklos Szeredi
2013-08-06 16:26 ` Eric Boxer
2013-08-09 15:02 ` Maxim Patlasov
2013-08-30 10:12 ` Miklos Szeredi
2013-08-30 14:50 ` Maxim Patlasov
2013-09-03 10:31 ` Miklos Szeredi
2013-09-03 16:02 ` Maxim Patlasov
2013-06-29 17:45 ` [PATCH 11/16] fuse: Implement write_begin/write_end callbacks Maxim Patlasov
2013-06-29 17:46 ` [PATCH 12/16] fuse: fuse_writepage_locked() should wait on writeback Maxim Patlasov
2013-06-29 17:46 ` [PATCH 13/16] fuse: fuse_flush() " Maxim Patlasov
2013-06-29 17:46 ` [PATCH 14/16] fuse: Fix O_DIRECT operations vs cached writeback misorder - v2 Maxim Patlasov
2013-06-29 17:47 ` [PATCH 15/16] fuse: Turn writeback cache on Maxim Patlasov
2013-06-29 17:48 ` [PATCH 16/16] mm: strictlimit feature Maxim Patlasov
2013-07-01 21:16 ` Andrew Morton [this message]
2013-07-02 8:33 ` Maxim Patlasov
2013-07-02 17:44 ` [PATCH] mm: strictlimit feature -v2 Maxim Patlasov
2013-07-02 19:38 ` Andrew Morton
2013-07-03 11:01 ` Maxim Patlasov
2013-07-03 23:16 ` Jan Kara
2013-07-05 13:14 ` Maxim Patlasov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130701141612.04d867863319bcc23d007a23@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=MPatlasov@parallels.com \
--cc=bfoster@redhat.com \
--cc=dev@parallels.com \
--cc=devel@openvz.org \
--cc=fengguang.wu@intel.com \
--cc=fuse-devel@lists.sourceforge.net \
--cc=jbottomley@parallels.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=miklos@szeredi.hu \
--cc=riel@redhat.com \
--cc=viro@zeniv.linux.org.uk \
--cc=xemul@parallels.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).