Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Kara <jack@suse.cz>
To: Tejun Heo <tj@kernel.org>
Cc: axboe@kernel.dk, jack@suse.cz, hannes@cmpxchg.org,
	mhocko@kernel.org, vdavydov.dev@gmail.com,
	cgroups@vger.kernel.org, linux-mm@kvack.org,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel-team@fb.com, guro@fb.com, akpm@linux-foundation.org
Subject: Re: [PATCH 5/5] writeback, memcg: Implement foreign dirty flushing
Date: Fri, 16 Aug 2019 18:02:56 +0200
Message-ID: <20190816160256.GI3041@quack2.suse.cz> (raw)
In-Reply-To: <20190815195930.GF2263813@devbig004.ftw2.facebook.com>

On Thu 15-08-19 12:59:30, Tejun Heo wrote:
> +/* issue foreign writeback flushes for recorded foreign dirtying events */
> +void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
> +{
> +	struct mem_cgroup *memcg = mem_cgroup_from_css(wb->memcg_css);
> +	unsigned long intv = msecs_to_jiffies(dirty_expire_interval * 10);
> +	u64 now = jiffies_64;
> +	int i;
> +
> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
> +		struct memcg_cgwb_frn *frn = &memcg->cgwb_frn[i];
> +
> +		/*
> +		 * If the record is older than dirty_expire_interval,
> +		 * writeback on it has already started.  No need to kick it
> +		 * off again.  Also, don't start a new one if there's
> +		 * already one in flight.
> +		 */
> +		if (frn->at > now - intv && atomic_read(&frn->done.cnt) == 1) {
> +			frn->at = 0;
> +			cgroup_writeback_by_id(frn->bdi_id, frn->memcg_id,
> +					       LONG_MAX, WB_REASON_FOREIGN_FLUSH,
> +					       &frn->done);
> +		}

Hum, two concerns here still:

1) You ask to writeback LONG_MAX pages. That means that you give up any
livelock avoidance for the flusher work and you can writeback almost
forever if someone is busily dirtying pages in the wb. I think you need to
pick something like amount of dirty pages in the given wb (that would have
to be fetched after everything is looked up) or just some arbitrary
reasonably small constant like 1024 (but then I guess there's no guarantee
stuck memcg will make any progress and you've invalidated the frn entry
here).

2) When you invalidate frn entry here by writing 0 to 'at', it's likely to get
reused soon. Possibly while the writeback is still running. And then you
won't start any writeback for the new entry because of the
atomic_read(&frn->done.cnt) == 1 check. This seems like it could happen
pretty frequently?

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

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15 19:56 [PATCHSET v2] writeback, memcg: Implement foreign inode flushing Tejun Heo
2019-08-15 19:57 ` [PATCH 1/5] writeback: Generalize and expose wb_completion Tejun Heo
2019-08-15 19:57 ` [PATCH 2/5] bdi: Add bdi->id Tejun Heo
2019-08-15 19:58 ` [PATCH 3/5] writeback: Separate out wb_get_lookup() from wb_get_create() Tejun Heo
2019-08-16 15:45   ` Jan Kara
2019-08-15 19:59 ` [PATCH 4/5] writeback, memcg: Implement cgroup_writeback_by_id() Tejun Heo
2019-08-16 15:47   ` Jan Kara
2019-08-21 21:02   ` [PATCH v3 " Tejun Heo
2019-08-26 13:49     ` Jan Kara
2019-08-15 19:59 ` [PATCH 5/5] writeback, memcg: Implement foreign dirty flushing Tejun Heo
2019-08-16 16:02   ` Jan Kara [this message]
2019-08-21 16:00     ` Tejun Heo
2019-08-21 16:04       ` Tejun Heo
2019-08-21 21:02   ` [PATCH v3 " Tejun Heo
2019-08-26 13:54     ` Jan Kara
2019-08-26 15:58       ` Tejun Heo
2019-08-26 16:06 [PATCHSET v3] writeback, memcg: Implement foreign inode flushing Tejun Heo
2019-08-26 16:06 ` [PATCH 5/5] writeback, memcg: Implement foreign dirty flushing Tejun Heo
2019-08-27 14:47   ` Jan Kara

Reply instructions:

You may reply publically 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=20190816160256.GI3041@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=cgroups@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=kernel-team@fb.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=tj@kernel.org \
    --cc=vdavydov.dev@gmail.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

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox