On Thu, Aug 22, 2019 at 01:45:48PM +0800, kaixuxia wrote:
> On 2019/8/22 13:01, Dave Chinner wrote:
> > On Thu, Aug 22, 2019 at 12:33:23PM +0800, kaixuxia wrote:
> >
> >> @@ -3419,25 +3431,15 @@ struct xfs_iunlink {
> >>
> >> /*
> >> * For whiteouts, we need to bump the link count on the whiteout inode.
> >
> > Shouldn't this line be removed as well?
>
> Because the xfs_bumplink() call below will do this.
Oh, yeah, I just assumed that from the "we have a real link" part of
the new comment :P
> >> - * This means that failures all the way up to this point leave the inode
> >> - * on the unlinked list and so cleanup is a simple matter of dropping
> >> - * the remaining reference to it. If we fail here after bumping the link
> >> - * count, we're shutting down the filesystem so we'll never see the
> >> - * intermediate state on disk.
> >> + * The whiteout inode has been removed from the unlinked list and log
> >> + * recovery will clean up the mess for the failures up to this point.
> >> + * After this point we have a real link, clear the tmpfile state flag
> >> + * from the inode so it doesn't accidentally get misused in future.
> >> */
> >> if (wip) {
> >> ASSERT(VFS_I(wip)->i_nlink == 0);
> >> xfs_bumplink(tp, wip);
> >> - error = xfs_iunlink_remove(tp, wip);
> >> - if (error)
> >> - goto out_trans_cancel;
> >> xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
> >> -
> >> - /*
> >> - * Now we have a real link, clear the "I'm a tmpfile" state
> >> - * flag from the inode so it doesn't accidentally get misused in
> >> - * future.
> >> - */
> >> VFS_I(wip)->i_state &= ~I_LINKABLE;
> >> }
> >
> > Why not move all this up into the same branch that removes the
> > whiteout from the unlinked list? Why separate this logic as none of
> > what is left here could cause a failure even if it is run earlier?
>
> Yep, it could not cause a failure if we move all this into the same
> branch that xfs_iunlink_remove() call. We move the xfs_iunlink_remove()
> first to preserve correct AGI/AGF locking order, and maybe it is better
> we bump the link count after using the whiteout inode really, such as
> xfs_dir_replace(...,wip,...) ...
It makes no difference where we bump the link count as long as we do
it after the xfs_iunlink_remove() call. At that point, any failure
will result in a shutdown and so it doesn't matter that we've
already bumped the link count because the shutdown with prevent
it from reaching the disk...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
