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 E2C00C10F14 for ; Sat, 12 Oct 2019 12:08:41 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B2A08206CD for ; Sat, 12 Oct 2019 12:08:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B2A08206CD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 685A18E0006; Sat, 12 Oct 2019 08:08:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 635F48E0001; Sat, 12 Oct 2019 08:08:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5242D8E0006; Sat, 12 Oct 2019 08:08:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0050.hostedemail.com [216.40.44.50]) by kanga.kvack.org (Postfix) with ESMTP id 2FC5A8E0001 for ; Sat, 12 Oct 2019 08:08:41 -0400 (EDT) Received: from smtpin03.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id AE8C780E0AF7 for ; Sat, 12 Oct 2019 12:08:40 +0000 (UTC) X-FDA: 76035010800.03.aunt51_116992a142555 X-HE-Tag: aunt51_116992a142555 X-Filterd-Recvd-Size: 5494 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by imf05.hostedemail.com (Postfix) with ESMTP for ; Sat, 12 Oct 2019 12:08:39 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 74F582BFDD; Sat, 12 Oct 2019 12:08:38 +0000 (UTC) Received: from bfoster (dhcp-41-2.bos.redhat.com [10.18.41.2]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D666F5D6C8; Sat, 12 Oct 2019 12:08:37 +0000 (UTC) Date: Sat, 12 Oct 2019 08:08:36 -0400 From: Brian Foster To: Dave Chinner 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: <20191012120836.GB3307@bfoster> References: <20191009032124.10541-1-david@fromorbit.com> <20191009032124.10541-17-david@fromorbit.com> <20191011101825.GA29171@infradead.org> <20191011152945.GH61257@bfoster> <20191011232716.GO16973@dread.disaster.area> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191011232716.GO16973@dread.disaster.area> User-Agent: Mutt/1.12.1 (2019-06-15) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Sat, 12 Oct 2019 12:08:38 +0000 (UTC) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sat, Oct 12, 2019 at 10:27:16AM +1100, Dave Chinner wrote: > 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... > Ok, I guess I forgot that this was temporary. FWIW, I think it's worth fixing the small things like this comparison and the couple things Christoph points out, if nothing else to facilitate review. Disregard the larger refactoring feedback.. Brian > > > > + 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