From patchwork Fri Jul 20 14:41:07 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Foster X-Patchwork-Id: 10537627 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2467A602CA for ; Fri, 20 Jul 2018 14:41:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 10EEA287E9 for ; Fri, 20 Jul 2018 14:41:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 03456297DF; Fri, 20 Jul 2018 14:41:11 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 69B97297C1 for ; Fri, 20 Jul 2018 14:41:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731583AbeGTP3q (ORCPT ); Fri, 20 Jul 2018 11:29:46 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:48840 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1731251AbeGTP3q (ORCPT ); Fri, 20 Jul 2018 11:29:46 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 07850401EF29; Fri, 20 Jul 2018 14:41:09 +0000 (UTC) Received: from bfoster (dhcp-41-2.bos.redhat.com [10.18.41.2]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C1A5D2142F20; Fri, 20 Jul 2018 14:41:08 +0000 (UTC) Date: Fri, 20 Jul 2018 10:41:07 -0400 From: Brian Foster To: "Darrick J. Wong" Cc: Christoph Hellwig , linux-xfs@vger.kernel.org Subject: Re: [PATCH 00/14] xfs: embed dfops in the transaction Message-ID: <20180720144106.GB34727@bfoster> References: <20180719134919.29939-1-bfoster@redhat.com> <20180719200557.GK6558@infradead.org> <20180719203643.GJ29404@bfoster> <20180719213634.GN4813@magnolia> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180719213634.GN4813@magnolia> User-Agent: Mutt/1.10.0 (2018-05-17) X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Fri, 20 Jul 2018 14:41:09 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Fri, 20 Jul 2018 14:41:09 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'bfoster@redhat.com' RCPT:'' Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jul 19, 2018 at 02:36:34PM -0700, Darrick J. Wong wrote: > On Thu, Jul 19, 2018 at 04:36:43PM -0400, Brian Foster wrote: > > On Thu, Jul 19, 2018 at 01:05:57PM -0700, Christoph Hellwig wrote: > > > On Thu, Jul 19, 2018 at 09:49:05AM -0400, Brian Foster wrote: > > > > return a clean transaction. Other things to consider might be to do away > > > > with support for external dfops and the ->t_dfops pointer indirection, > > > > or perhaps even consider going the other direction: allocate dfops from > > > > a separate zone to save some memory on non-permanent transactions (note > > > > that 16 of 28 transactions use a permanent log res. last I looked, so it > > > > may not be worth it atm). > > > > > > The defer_ops aren't really that big, and allocations are relatively > > > costly, so I don't think a separate allocation is a good idea. If we > > > really want to optimize the non-permanent transaction case we could do > > > something like: > > > > > > struct xfs_trans { > > > ... > > > struct xfs_defer_ops dfops[]; > > > }; > > > > > > and then have two caches for the with an without dfops case. But > > > I can't believe that would be worth it, especially in face of... > > > > > > > > > > I know Christoph also had thoughts around condensing some of the items > > > > joined to the dfops to those with the transaction. > > > > > > ... this. > > > > > > > Yeah. I was actually poking around today after writing this up and > > thought that we might be able to replace both dop_inodes/dop_bufs with > > checks in the transaction item list for either held buffers or inode > > items where lock_flags == 0. I _think_ both of those states may be > > essentially equivalent to joined dfops items, but I have to verify that. > > If so, we can probably make the dfops inode/buf relogging "automatic," > > drop both pointer lists and the whole memory thing becomes kind of moot. > > > Ok.. on this one, running some tests shows that pretty much all dfops joined bufs/inodes are essentially held in the transaction (which would probably be a bug if that were not the case). The opposite is also true for buffers, because a held buffers must also be held in subsequent transactions to ensure the caller still has a reference after dfops completion. OTOH, there are some places where inodes are joined to a transaction with lock_flags == 0 and dfops are run without the inode joined to the dfops. I don't necessarily think this is a bug for the inode case because the caller still owns the inode lock, we just don't relog it across the series of transactions required for dfops completion. I don't think it would necessarily be a problem if we did relog the inode in these cases, however. In fact I think the argument has been made in the past to do that explicitly (with xfs_defer_[i|b]join()) in one or two cases. So given that, the overall approach still seems reasonable enough to me. FWIW, the appended patch shows the cases I've caught on a quick xfstests run that would be affected (i.e., where we don't currently defer_ijoin() but implicitly would with an "automatic" relog mechanism). I'm running a longer test to try and catch any others, but let me know if anybody sees problems with this or has other ideas... Brian --- 8< --- --- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 60138514ea86..c3cbe05cdb1b 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -1120,6 +1120,7 @@ xfs_bmap_add_attrfork( xfs_log_sb(tp); } + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); xfs_iunlock(ip, XFS_ILOCK_EXCL); return error; diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 7b40e9fef381..e63712e67fd8 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -979,6 +979,7 @@ xfs_alloc_file_space( /* * Complete the transaction */ + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); xfs_iunlock(ip, XFS_ILOCK_EXCL); if (error) diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 0e4bd559a6a7..24fdca90b588 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1810,6 +1810,7 @@ xfs_inactive_ifree( * Just ignore errors at this point. There is nothing we can do except * to try to keep going. Make sure it's not a silent error. */ + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); if (error) xfs_notice(mp, "%s: xfs_trans_commit returned error %d", diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index 8e8ca9f03f0e..464b4b3225bb 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -261,6 +261,7 @@ xfs_iomap_write_direct( /* * Complete the transaction */ + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); if (error) goto out_unlock; @@ -762,6 +763,7 @@ xfs_iomap_write_allocate( if (error) goto trans_cancel; + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); if (error) goto error0; @@ -879,6 +881,7 @@ xfs_iomap_write_unwritten( xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); } + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); xfs_iunlock(ip, XFS_ILOCK_EXCL); if (error) diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index cddde219630a..8984f283da7b 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -435,6 +435,7 @@ xfs_reflink_allocate_cow( xfs_inode_set_cowblocks_tag(ip); /* Finish up. */ + xfs_defer_ijoin(tp->t_dfops, ip); error = xfs_trans_commit(tp); if (error) return error;