All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Roman Gushchin <guro@fb.com>, Tejun Heo <tj@kernel.org>,
	linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 4/4] vfs: keep inodes with page cache off the inode shrinker LRU
Date: Thu, 17 Jun 2021 10:49:42 +1000	[thread overview]
Message-ID: <20210617004942.GF2419729@dread.disaster.area> (raw)
In-Reply-To: <YMmD9xhBm9wGqYhf@cmpxchg.org>

On Wed, Jun 16, 2021 at 12:54:15AM -0400, Johannes Weiner wrote:
> On Wed, Jun 16, 2021 at 11:20:08AM +1000, Dave Chinner wrote:
> > On Tue, Jun 15, 2021 at 02:50:09PM -0400, Johannes Weiner wrote:
> > > On Tue, Jun 15, 2021 at 04:26:40PM +1000, Dave Chinner wrote:
> > > > On Mon, Jun 14, 2021 at 05:19:04PM -0400, Johannes Weiner wrote:
> > > > > @@ -1123,6 +1125,9 @@ static int __remove_mapping(struct address_space *mapping, struct page *page,
> > > > >  			shadow = workingset_eviction(page, target_memcg);
> > > > >  		__delete_from_page_cache(page, shadow);
> > > > >  		xa_unlock_irq(&mapping->i_pages);
> > > > > +		if (mapping_shrinkable(mapping))
> > > > > +			inode_add_lru(mapping->host);
> > > > > +		spin_unlock(&mapping->host->i_lock);
> > > > >  
> > > > 
> > > > No. Inode locks have absolutely no place serialising core vmscan
> > > > algorithms.
> > > 
> > > What if, and hear me out on this one, core vmscan algorithms change
> > > the state of the inode?
> > 
> > Then the core vmscan algorithm has a layering violation.
> 
> You're just playing a word game here.

No, I've given you plenty of constructive justification and ways to
restructure your patches to acheive what you say needs to be done.

You're the one that is rejecting any proposal I make outright and
making unjustified claims that "I don't understand this code".

Serious, Johannes, I understand how the inode cache shrinkers work
better than most people out there, and for you to just reject my
comments and suggestions outright with "you don't understand this
stuff so go learn about it' is pretty obnoxious and toxic behaviour.

I haven't disagreed at all with what you are trying to do, nor do I
think that being more selective about how we track inodes on the
LRUs is a bad thing. What I'm commenting on is that the proposed
changes are *really bad code*.

If you can work out a *clean* way to move inodes onto the LRU when
they are dirty then I'm all for it. But sprinkling inode->i_lock all
over the mm/ subsystem and then adding seemling randomly placed
inode lru manipulations isn't the way to do it.

You should consider centralising all the work involved marking a
mapping clean somewhere inside the mm/ code. Then add a single
callout that does the inode LRU work, similar to how the
fs-writeback.c code does it when the inode is marked clean by
inode_sync_complete().

IOWs, if you can't accept that there are problems with your approach
nor accept that it needs to be improved because *I* said there are
problems (and it seems that you mostly only react this way when *I*
happen to say "needs improvement"), then you need to go take a good
hard look at yourself.

Fix up your code so that it is acceptible to reviewers - telling
reviewers they don't know shit is a sure way to piss people off
unnecesarily. It doesn't make you look very good at all, and it
makes other people think twice about wanting to review your code or
work with you in future.

So, again, until you see fit to be stop being obnoxious and toxic
and that your patches need work before they are acceptible for
merging, I maintain my NACK on this patch as it standsi and I am
complete done with this discusion.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2021-06-17  0:49 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-14 21:19 [PATCH 1/4] mm: remove irqsave/restore locking from contexts with irqs enabled Johannes Weiner
2021-06-14 21:19 ` [PATCH 2/4] fs: drop_caches: fix skipping over shadow cache inodes Johannes Weiner
2021-06-14 22:31   ` Roman Gushchin
2021-06-14 21:19 ` [PATCH 3/4] fs: inode: count invalidated shadow pages in pginodesteal Johannes Weiner
2021-06-14 21:19 ` [PATCH 4/4] vfs: keep inodes with page cache off the inode shrinker LRU Johannes Weiner
2021-06-14 21:59   ` Andrew Morton
2021-06-14 22:41     ` Johannes Weiner
2021-06-15  6:26   ` Dave Chinner
2021-06-15 18:50     ` Johannes Weiner
2021-06-16  1:20       ` Dave Chinner
2021-06-16  4:54         ` Johannes Weiner
2021-06-17  0:49           ` Dave Chinner [this message]
2021-06-18 16:45             ` Johannes Weiner
2021-06-28 18:58               ` Johannes Weiner
2021-06-17  1:30           ` Andrew Morton
2021-06-18 17:09             ` Johannes Weiner
2021-06-28 17:12         ` Johannes Weiner

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=20210617004942.GF2419729@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=akpm@linux-foundation.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=kernel-team@fb.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=tj@kernel.org \
    --subject='Re: [PATCH 4/4] vfs: keep inodes with page cache off the inode shrinker LRU' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.