From: Dave Chinner <david@fromorbit.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v8 23/28] xfs: Add parent pointers to rename
Date: Mon, 3 Sep 2018 13:20:31 +1000 [thread overview]
Message-ID: <20180903032031.GO5631@dastard> (raw)
In-Reply-To: <1535484161-11059-24-git-send-email-allison.henderson@oracle.com>
On Tue, Aug 28, 2018 at 12:22:36PM -0700, Allison Henderson wrote:
> This patch removes the old parent pointer attribute during the
> rename operation, and re-adds the updated parent pointer
>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
renameat operations in generic/023 trigger an ASSERT failure in the
error handling path.
XFS: Assertion failed: xfs_isilocked(ip, XFS_ILOCK_EXCL), file: fs/xfs/xfs_inode_item.c, line: 576
------------[ cut here ]------------
kernel BUG at fs/xfs/xfs_message.c:102!
invalid opcode: 0000 [#1] PREEMPT SMP
CPU: 0 PID: 10568 Comm: renameat2 Not tainted 4.19.0-rc2-dgc+ #652
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.1-1 04/01/2014
RIP: 0010:assfail+0x28/0x30
Code: c3 90 0f 1f 44 00 00 48 89 f1 41 89 d0 48 c7 c6 c8 52 2e 82 48 89 fa 31 ff e8 64 f9 ff ff 80 3d 05 95 0a 01 00 75 03 0f 0b c3 <0f> 0b 66 0f 1f 44 00 00 0f 1f 44 00 00 48 63 f6 49 8a
RSP: 0018:ffffc90000b7bc18 EFLAGS: 00010202
RAX: 0000000000000000 RBX: ffff88002d20a840 RCX: 0000000000000000
RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffff8227a7dc
RBP: ffff88002b4fb480 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000200 R11: f000000000000000 R12: ffff880034495fc8
R13: ffffffffffffffff R14: 0000000000000000 R15: ffffffff822e6858
FS: 00007f5fc4d4b740(0000) GS:ffff88003ec00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f5fc3d01170 CR3: 0000000034534000 CR4: 00000000000006f0
Call Trace:
xfs_inode_item_unlock+0x63/0x80
xfs_trans_free_items+0x71/0xf0
xfs_trans_cancel+0xaf/0x1f0
xfs_rename+0x435/0xc50
xfs_vn_rename+0xd5/0x140
vfs_rename+0x384/0x9b0
? lookup_dcache+0x17/0x60
do_renameat2+0x49b/0x550
__x64_sys_renameat2+0x20/0x30
do_syscall_64+0x5a/0x180
entry_SYSCALL_64_after_hwframe+0x49/0xbe
> @@ -2985,14 +2987,14 @@ xfs_rename(
> * we can rely on either trans_commit or trans_cancel to unlock
> * them.
> */
> - xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, src_dp, 0);
> if (new_parent)
> - xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL);
> - xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, target_dp, 0);
> + xfs_trans_ijoin(tp, src_ip, 0);
> if (target_ip)
> - xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, target_ip, 0);
> if (wip)
> - xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, wip, 0);
So we change the locking such that failure requires use to unlock
the inodes after the transaction commit or cancel....
>
> /*
> * If we are using project inheritance, we only allow renames
> @@ -3002,15 +3004,16 @@ xfs_rename(
> if (unlikely((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) &&
> (xfs_get_projid(target_dp) != xfs_get_projid(src_ip)))) {
> error = -EXDEV;
> - goto out_trans_cancel;
> + goto out_unlock;
This smells wrong - we need to cancel the transaction, then unlock
the items, not the other way around, so we should still be jumping
to a transaction cancel action.
> }
>
> /* RENAME_EXCHANGE is unique from here on. */
> - if (flags & RENAME_EXCHANGE)
> - return xfs_cross_rename(tp, src_dp, src_name, src_ip,
> + if (flags & RENAME_EXCHANGE) {
> + error = xfs_cross_rename(tp, src_dp, src_name, src_ip,
> target_dp, target_name, target_ip,
> spaceres);
> -
> + goto out;
> + }
This looks problematic, too. i.e. On error, xfs_cross_rename() will
have already called xfs_trans_cancel(), but it hasn't unlocked any
of the inodes. If it succeeds, it commits the transaction. Either
way, the transaction has been finished and freed. However, the
code we jump to expects that the transaction is open and still
running.
Also "out" typically means "goto end of function and return". This
is jumping to parent pointer addition, not the function return
processing. Hence the label needs to be changed to
"parent_pointer"....
[....]
> @@ -3175,16 +3178,51 @@ xfs_rename(
> VFS_I(wip)->i_state &= ~I_LINKABLE;
> }
>
> +out:
> + if (xfs_sb_version_hasparent(&mp->m_sb)) {
> + error = xfs_parent_add_deferred(target_dp, tp, src_ip, target_name,
> + new_diroffset);
And so when we try to add the a parent pointers, we haven't checked
if the cross rename failed and aborted the transaction. Or if it
succeeded, it expects to continue using the transaction which
xfs_cross_rename() committed.
> + if (error)
> + goto out_bmap_cancel;
> +
> + error = xfs_parent_remove_deferred(src_dp, tp, src_ip,
> + old_diroffset);
> + if (error)
> + goto out_bmap_cancel;
> + }
We don't have "bmaps" in these functions any more. We just need to
cancel the trasaction now, right?
> +
> xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
> if (new_parent)
> xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
>
> error = xfs_finish_rename(tp);
> +
> if (wip)
> xfs_irele(wip);
> + if (wip)
> + xfs_iunlock(wip, XFS_ILOCK_EXCL);
> + if (target_ip)
> + xfs_iunlock(target_ip, XFS_ILOCK_EXCL);
> + xfs_iunlock(src_ip, XFS_ILOCK_EXCL);
> + if (new_parent)
> + xfs_iunlock(target_dp, XFS_ILOCK_EXCL);
> + xfs_iunlock(src_dp, XFS_ILOCK_EXCL);
> +
OK, so this unlocks after the final commit. That's fine, but the
rest of the error stack looks wrong.
> return error;
>
> +out_bmap_cancel:
> + xfs_defer_cancel(tp);
xfs_trans_cancel() calls xfs_defer_cancel() now, so it's not needed
anymore.
> +out_unlock:
> + if (wip)
> + xfs_iunlock(wip, XFS_ILOCK_EXCL);
> + if (target_ip)
> + xfs_iunlock(target_ip, XFS_ILOCK_EXCL);
> + xfs_iunlock(src_ip, XFS_ILOCK_EXCL);
> + if (new_parent)
> + xfs_iunlock(target_dp, XFS_ILOCK_EXCL);
> + xfs_iunlock(src_dp, XFS_ILOCK_EXCL);
And I think this has to happen after calling xfs_trans_cancel().
> +
> out_trans_cancel:
> xfs_trans_cancel(tp);
> out_release_wip:
So the return stacking needs work here. I think it should look
something like:
xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
if (new_parent)
xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
error = xfs_finish_rename(tp);
out_unlock:
if (wip)
xfs_irele(wip);
if (wip)
xfs_iunlock(wip, XFS_ILOCK_EXCL);
if (target_ip)
xfs_iunlock(target_ip, XFS_ILOCK_EXCL);
xfs_iunlock(src_ip, XFS_ILOCK_EXCL);
if (new_parent)
xfs_iunlock(target_dp, XFS_ILOCK_EXCL);
xfs_iunlock(src_dp, XFS_ILOCK_EXCL);
return error;
out_trans_cancel:
xfs_trans_cancel(tp);
out_release_wip:
if (wip)
xfs_irele(wip);
goto out_unlock;
}
This means almost all the error goto's remain unchanged,
"out_bmap_cancel" should be "out_trans_cancel", xfs_cross_rename()
needs to roll the transaction, not commit it, and it if fails it
needs to jump to out_trans_cancel().
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2018-09-03 7:38 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-28 19:22 [PATCH v8 00/28] Parent Pointers v8 Allison Henderson
2018-08-28 19:22 ` [PATCH v8 01/28] xfs: Move fs/xfs/xfs_attr.h to fs/xfs/libxfs/xfs_attr.h Allison Henderson
2018-08-28 19:22 ` [PATCH v8 02/28] xfs: Add helper function xfs_attr_try_sf_addname Allison Henderson
2018-08-28 19:22 ` [PATCH v8 03/28] xfs: Add attibute set and helper functions Allison Henderson
2018-08-28 19:22 ` [PATCH v8 04/28] xfs: Add attibute remove " Allison Henderson
2018-08-28 19:22 ` [PATCH v8 05/28] xfs: Hold inode locks in xfs_ialloc Allison Henderson
2018-08-28 19:22 ` [PATCH v8 06/28] xfs: Add trans toggle to attr routines Allison Henderson
2018-08-28 19:22 ` [PATCH v8 07/28] xfs: Set up infastructure for deferred attribute operations Allison Henderson
2018-08-28 19:22 ` [PATCH v8 08/28] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred Allison Henderson
2018-08-28 19:22 ` [PATCH v8 09/28] xfs: Add xfs_has_attr and subroutines Allison Henderson
2018-08-28 19:22 ` [PATCH v8 10/28] xfs: Add attr context to log item Allison Henderson
2018-08-28 19:22 ` [PATCH v8 11/28] xfs: Roll delayed attr operations by returning EAGAIN Allison Henderson
2018-08-28 19:22 ` [PATCH v8 12/28] xfs: Remove roll_trans boolean Allison Henderson
2018-08-28 19:22 ` [PATCH v8 13/28] xfs: Remove all strlen calls in all xfs_attr_* functions for attr names Allison Henderson
2018-08-28 19:22 ` [PATCH v8 14/28] xfs: get directory offset when adding directory name Allison Henderson
2018-08-28 19:22 ` [PATCH v8 15/28] xfs: get directory offset when removing " Allison Henderson
2018-08-28 19:22 ` [PATCH v8 16/28] xfs: get directory offset when replacing a " Allison Henderson
2018-08-28 19:22 ` [PATCH v8 17/28] xfs: add parent pointer support to attribute code Allison Henderson
2018-08-28 19:22 ` [PATCH v8 18/28] xfs: define parent pointer xattr format Allison Henderson
2018-08-28 19:22 ` [PATCH v8 19/28] xfs: extent transaction reservations for parent attributes Allison Henderson
2018-08-28 19:22 ` [PATCH v8 20/28] xfs: parent pointer attribute creation Allison Henderson
2018-08-28 19:22 ` [PATCH v8 21/28] xfs: add parent attributes to link Allison Henderson
2018-08-28 19:22 ` [PATCH v8 22/28] xfs: remove parent pointers in unlink Allison Henderson
2018-08-28 19:22 ` [PATCH v8 23/28] xfs: Add parent pointers to rename Allison Henderson
2018-09-03 3:20 ` Dave Chinner [this message]
2018-09-03 5:28 ` Amir Goldstein
2018-09-04 18:31 ` Allison Henderson
2018-09-04 18:31 ` Allison Henderson
2018-08-28 19:22 ` [PATCH v8 24/28] xfs: Add the parent pointer support to the superblock version 5 Allison Henderson
2018-08-28 19:22 ` [PATCH v8 25/28] xfs: Add helper function xfs_attr_list_context_init Allison Henderson
2018-08-28 19:22 ` [PATCH v8 26/28] xfs: Increase XFS_DEFER_OPS_NR_INODES to 4 Allison Henderson
2018-08-28 19:22 ` [PATCH v8 27/28] xfs: Add parent pointer ioctl Allison Henderson
2018-08-28 19:22 ` [PATCH v8 28/28] xfs: Add delayed attributes error tag Allison Henderson
2018-09-03 1:20 ` [PATCH v8 00/28] Parent Pointers v8 Dave Chinner
2018-09-03 1:40 ` Dave Chinner
2018-09-04 18:31 ` Allison Henderson
2018-09-03 5:41 ` Dave Chinner
2018-09-04 18:32 ` Allison Henderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180903032031.GO5631@dastard \
--to=david@fromorbit.com \
--cc=allison.henderson@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).