Re: [PATCH 01/11] xfs: reflink should break pnfs leases before sharing blocks
From: Christoph Hellwig <hch@infradead.org>
Date: 2018-01-26 09:06:53
On Tue, Jan 23, 2018 at 06:18:03PM -0800, Darrick J. Wong wrote:
quoted hunk ↗ jump to hunk
From: Darrick J. Wong <redacted> 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 <redacted> --- 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.
+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..