On 09/02/2018 10:28 PM, Amir Goldstein wrote:
On Mon, Sep 3, 2018 at 6:26 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
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@xxxxxxxxxx>
[...]
}
/* 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"....
[...]
+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.
Allison,
Not sure if you know that xfs_cross_rename() exchanges src with target, so
the src_dp parent pointer has to be adjusted as well in case of cross rename.
Oh I see. Ok, I will add in a snippet to handle src_dp in the case of
the cross rename. Thank you!
Allison
Thanks,
Amir.
