public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, hsiangkao@redhat.com
Subject: Re: [PATCH 2/7] repair: Protect bad inode list with mutex
Date: Sat, 20 Mar 2021 09:20:53 +1100	[thread overview]
Message-ID: <20210319222053.GT63242@dread.disaster.area> (raw)
In-Reply-To: <20210319182011.GT22100@magnolia>

On Fri, Mar 19, 2021 at 11:20:11AM -0700, Darrick J. Wong wrote:
> On Fri, Mar 19, 2021 at 12:33:50PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > To enable phase 6 parallelisation, we need to protect the bad inode
> > list from concurrent modification and/or access. Wrap it with a
> > mutex and clean up the nasty typedefs.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> FWIW, if you (Gao at this point, I surmise) want to dig deeper into the
> comment that Christoph made during the last review of this patchset,
> repair already /does/ have a resizing array structure in repair/slab.c.

Put it in another patchset, please.

Everyone, can we please get out of the habit of asking for extra
cleanups to be done as a condition for getting a change
merged just because the patchset makes a change to slightly crufty
but perfectly working code?

Cleaning up this code is not necessary for this patchset to achieve
it's goals or make forwards progress. Yes, the code that locking is
being added to is slightly crusty, but it is not broken nor is
showing up on profiles as being a performance, scalability or memory
usage limiting factor. Changing the algorithm of this code is
therefore unnecessary to acheive the goals of this patchset.

IOWs, put this aside this cleanup as future work on a rainy day
when we have fixed all the bigger scalability problems and have
nothing better to do.

"If it ain't broke, don't fix it."

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2021-03-19 22:21 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-19  1:33 [PATCH 0/7] repair: Phase 6 performance improvements Dave Chinner
2021-03-19  1:33 ` [PATCH 1/7] workqueue: bound maximum queue depth Dave Chinner
2021-03-19  1:45   ` Darrick J. Wong
2021-03-19  1:33 ` [PATCH 2/7] repair: Protect bad inode list with mutex Dave Chinner
2021-03-19 18:20   ` Darrick J. Wong
2021-03-19 22:20     ` Dave Chinner [this message]
2021-03-19  1:33 ` [PATCH 3/7] repair: protect inode chunk tree records with a mutex Dave Chinner
2021-03-19 18:11   ` Darrick J. Wong
2021-03-19  1:33 ` [PATCH 4/7] repair: parallelise phase 6 Dave Chinner
2021-03-19  1:33 ` [PATCH 5/7] repair: don't duplicate names in " Dave Chinner
2021-03-19  1:33 ` [PATCH 6/7] repair: convert the dir byaddr hash to a radix tree Dave Chinner
2021-03-19 22:44   ` Darrick J. Wong
2021-03-19  1:33 ` [PATCH 7/7] repair: scale duplicate name checking in phase 6 Dave Chinner
2021-03-19  1:38 ` [PATCH 0/7] repair: Phase 6 performance improvements Gao Xiang
2021-03-19 18:22   ` Darrick J. Wong
2021-03-20  2:09     ` Gao Xiang
2021-03-24  1:26       ` Gao Xiang
2021-03-24  2:08         ` Darrick J. Wong
  -- strict thread matches above, loose matches on Subject: below --
2020-10-22  5:15 Dave Chinner
2020-10-22  5:15 ` [PATCH 2/7] repair: Protect bad inode list with mutex Dave Chinner
2020-10-22  5:45   ` Darrick J. Wong
2020-10-29  9:35   ` Christoph Hellwig

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=20210319222053.GT63242@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=djwong@kernel.org \
    --cc=hsiangkao@redhat.com \
    --cc=linux-xfs@vger.kernel.org \
    /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