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.
Thanks,
Amir.
