From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63FE2ECE58D for ; Fri, 11 Oct 2019 23:27:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4348721D71 for ; Fri, 11 Oct 2019 23:27:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726642AbfJKX1V (ORCPT ); Fri, 11 Oct 2019 19:27:21 -0400 Received: from mail105.syd.optusnet.com.au ([211.29.132.249]:38426 "EHLO mail105.syd.optusnet.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726243AbfJKX1V (ORCPT ); Fri, 11 Oct 2019 19:27:21 -0400 Received: from dread.disaster.area (pa49-181-198-88.pa.nsw.optusnet.com.au [49.181.198.88]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 9B3923638A4; Sat, 12 Oct 2019 10:27:16 +1100 (AEDT) Received: from dave by dread.disaster.area with local (Exim 4.92.2) (envelope-from ) id 1iJ4JY-0007qb-4M; Sat, 12 Oct 2019 10:27:16 +1100 Date: Sat, 12 Oct 2019 10:27:16 +1100 From: Dave Chinner To: Brian Foster Cc: Christoph Hellwig , linux-xfs@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 16/26] xfs: synchronous AIL pushing Message-ID: <20191011232716.GO16973@dread.disaster.area> References: <20191009032124.10541-1-david@fromorbit.com> <20191009032124.10541-17-david@fromorbit.com> <20191011101825.GA29171@infradead.org> <20191011152945.GH61257@bfoster> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191011152945.GH61257@bfoster> User-Agent: Mutt/1.10.1 (2018-07-13) X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=D+Q3ErZj c=1 sm=1 tr=0 a=ocld+OpnWJCUTqzFQA3oTA==:117 a=ocld+OpnWJCUTqzFQA3oTA==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=XobE76Q3jBoA:10 a=7-415B0cAAAA:8 a=_AcuBihmRUaaDqdcFvEA:9 a=CjuIK1q_8ugA:10 a=biEYGPWJfzWAr4FL6Ov7:22 Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org On Fri, Oct 11, 2019 at 11:29:45AM -0400, Brian Foster wrote: > On Fri, Oct 11, 2019 at 03:18:25AM -0700, Christoph Hellwig wrote: > > On Wed, Oct 09, 2019 at 02:21:14PM +1100, Dave Chinner wrote: > > > Factor the common AIL deletion code that does all the wakeups into a > > > helper so we only have one copy of this somewhat tricky code to > > > interface with all the wakeups necessary when the LSN of the log > > > tail changes. > > > > > > xfs_ail_push_sync() is temporary infrastructure to facilitate > > > non-blocking, IO-less inode reclaim throttling that allows further > > > structural changes to be made. Once those structural changes are > > > made, the need for this function goes away and it is removed, > > > leaving us with only the xfs_ail_update_finish() factoring when this > > > is all done. > > > > The xfs_ail_update_finish work here is in an earlier patch, so the > > changelog will need some updates. > > > > > + spin_lock(&ailp->ail_lock); > > > + while ((lip = xfs_ail_min(ailp)) != NULL) { > > > + prepare_to_wait(&ailp->ail_push, &wait, TASK_UNINTERRUPTIBLE); > > > + if (XFS_FORCED_SHUTDOWN(ailp->ail_mount) || > > > + XFS_LSN_CMP(threshold_lsn, lip->li_lsn) <= 0) > > Wasn't this supposed to change to < 0? The rfc series had that logic, > but it changed from <= to < later in the wrong patch. I probably forgot because this code gets removed at the end of the series. Hence I haven't cared about exact correctness of neatness as it's just temporary scaffolding to keep stuff from breaking horribly as the changeover to non-blocking algorithms is done. It works well enough that I can't break it as it stands - I've tested each patch individually with both load and fstests, and so this code as it stands doesn't introduce any bisect landmines - it prevents a bunch of problems in OOM conditions by retaining the blocking behaviour of reclaim until we no longer need it... > > > + break; > > > + /* XXX: cmpxchg? */ > > > + while (XFS_LSN_CMP(threshold_lsn, ailp->ail_target) > 0) > > > + xfs_trans_ail_copy_lsn(ailp, &ailp->ail_target, &threshold_lsn); > > > > This code looks broken on 32-bit given that xfs_trans_ail_copy_lsn takes > > the ail_lock there. Just replacing the xfs_trans_ail_copy_lsn call with > > a direct assignment would fix that, no need for cmpxchg either as far > > as I can tell (and it would fix that too long line as well). Oh, right. I'll fix that. > > still looks odd, I think this should simply be an if. > > > > > + wake_up_process(ailp->ail_task); > > > + spin_unlock(&ailp->ail_lock); > > > > xfsaild will take ail_lock pretty quickly. I think we should drop > > the lock before waking it. > > Can't we replace this whole thing with something that repurposes > xfs_ail_push_all_sync()? That only requires some tweaks to the existing > function and the new _push_all_sync() wrapper ends up looking something > like: > > while ((threshold_lsn = xfs_ail_max_lsn(ailp)) != 0) > xfs_ail_push_sync(ailp, threshold_lsn); > > There's an extra lock cycle, but that's still only on tail updates. That > doesn't seem unreasonable to me for the usage of _push_all_sync(). The whole thing goes away, so there is zero point in trying to optimise or perfect this code. It's temporary code, treat it as such. Cheers, Dave. -- Dave Chinner david@fromorbit.com