From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp2120.oracle.com ([141.146.126.78]:34770 "EHLO aserp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751760AbeAZS1L (ORCPT ); Fri, 26 Jan 2018 13:27:11 -0500 Date: Fri, 26 Jan 2018 10:26:28 -0800 From: "Darrick J. Wong" Subject: Re: [PATCH 01/11] xfs: reflink should break pnfs leases before sharing blocks Message-ID: <20180126182628.GW9068@magnolia> References: <151676027743.12349.3845769501491774512.stgit@magnolia> <151676028366.12349.7219306586282315379.stgit@magnolia> <20180126090652.GA29115@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180126090652.GA29115@infradead.org> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Christoph Hellwig Cc: linux-xfs@vger.kernel.org On Fri, Jan 26, 2018 at 01:06:52AM -0800, Christoph Hellwig wrote: > On Tue, Jan 23, 2018 at 06:18:03PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong > > > > Before we share blocks between files, we need to break the pnfs leases > > on the layout before we start slicing and dicing the block map. > > > > Signed-off-by: Darrick J. Wong > > --- > > fs/xfs/xfs_reflink.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 47 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > > index 47aea2e..f89a725 100644 > > --- a/fs/xfs/xfs_reflink.c > > +++ b/fs/xfs/xfs_reflink.c > > @@ -1245,6 +1245,50 @@ xfs_reflink_remap_blocks( > > } > > > > /* > > + * Grab the exclusive iolock for a data copy from src to dest, making > > + * sure to abide vfs locking order (lowest pointer value goes first) and > > + * breaking the pnfs layout leases on dest before proceeding. The loop > > + * is needed because we cannot call the blocking break_layout() with the > > + * src iolock held, and therefore have to back out both locks. > > + */ > > +static int > > +xfs_iolock_two_inodes_and_break_layout( > > + struct inode *src, > > + struct inode *dest) > > +{ > > + bool src_first = src < dest; > > + bool src_last = src > dest; > > I find the double predicates here highly confusing. > > Also the code doesn't seem to handle the src == dest case as > far as I can tell. I guess they are confusing; when src == dest, src_first and src_last are both false. > > +retry: > > + if (src_first) { > > + inode_lock(src); > > + inode_lock_nested(dest, I_MUTEX_NONDIR2); > > + } else { > > + inode_lock(dest); > > + } > > Shouldn't this be replaced by a call to lock_two_nondirectories? > Even if that holds both locks over the noon-blocking break_layout > it makes things a lot simpler and only does an additional rountrip > for the layouts outstanding slow path. > > > + error = break_layout(dest, false); > > + if (error == -EWOULDBLOCK) { > > + inode_unlock(dest); > > + if (src_first) > > + inode_unlock(src); > > unlock_two_nondirectories? > > > + error = break_layout(dest, true); > > + if (error) > > + return error; > > + goto retry; > > + } else if (error) { > > no need for an else after a goto. > > > + inode_unlock(dest); > > + if (src_first) > > + inode_unlock(src); > > unlock_two_nondirectories? > > Also seems like this could be simplified to: > > if (error) { > unlock_two_nondirectories() > if (error == -EWOULDBLOCK) > goto retry; > return error; > } > > So I guess the whole thing could simply become something like: > > retry: > lock_two_nondirectories(src, dest); > error = break_layout(dest, false); > if (error) { > unlock_two_nondirectories(src, dest); > if (error == -EWOULDBLOCK) > goto retry; > return error; > } > > and could probably just be inlined into the caller.. Yeah, that's simpler... though at this point I'll have to put all this into a new series having already pushed to for-next. :/ (Sorry, kinda overburdened with this week) --D > 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