[PATCH 2/2] Add support to RENAME_EXCHANGE flag V8

[Carlos Maiolino]

Sigh...

For some reason, this thread has been broken, sorry if it was my mistake.


On Tue, Dec 02, 2014 at 12:13:42PM -0200, Carlos Maiolino wrote:
> Hi Dave,
> 
> 
> >> +			error = xfs_dir_replace(tp, ip2, &xfs_name_dotdot,
> >> +						dp1->i_ino, first_block,
> >> +						free_list, spaceres);
> >> +			if (error)
> >> +				goto out;
> 
> >now ip2 is modified, so it ctime/mtime dirty.
> 
> >> +
> >> +			/* transfer target ".." reference to dp1 */
> >> +			if (!S_ISDIR(ip1->i_d.di_mode)) {
> >> +				error = xfs_droplink(tp, dp2);
> >> +				if (error)
> >> +					goto out;
> >> +				error = xfs_bumplink(tp, dp1);
> >> +				if (error)
> >> +					goto out;
> >> +			}
> >> +			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
> >> +			xfs_trans_ichgtime(tp, ip2,
> >> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);
> 
> >But now you're unconditionally changing ctime on ip1 without it
> >having been modified and you aren't logging the change. Why is the
> >ctime changing (comments, please!)?
> 
> Ok, so, I can see that I didn't understand what we should do with logging here,
> for the moment, I thought we should bump the ip1 to notify userspace that
> changes has actually taken place here, even though we are not changing it
> anyway. I'm not sure if I should log it too (and add a xfs_trans_log_inode for
> ip1). So, should we also log it here together with ip1, or bumping ctime of ip1
> here is wrong?
> In this case, a more correct way to write it would be to not bump ip1 here but
> only in the next block, where we actually touches it?
> And regarding dp2 and dp1, we drop/bump its link counts here, should I care
> about logging them in this code block? My xfs logging knowledge is shallow by
> now :-(
> 
> >> +		}
> >> +
> >> +		if (S_ISDIR(ip1->i_d.di_mode)) {
> >> +			error = xfs_dir_replace(tp, ip1, &xfs_name_dotdot,
> >> +						dp2->i_ino, first_block,
> >> +						free_list, spaceres);
> >> +			if (error)
> >> +				goto out;
> 
> >here ip2 is modified
> 
> >> +
> >> +			/* transfer src ".." reference to dp2 */
> >> +			if (!S_ISDIR(ip2->i_d.di_mode)) {
> >> +				error = xfs_droplink(tp, dp1);
> >> +				if (error)
> >> +					goto out;
> >> +				error = xfs_bumplink(tp, dp2);
> >> +				if (error)
> >> +					goto out;
> >> +			}
> >> +			xfs_trans_ichgtime(tp, ip1,
> >> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +			xfs_trans_ichgtime(tp, ip2, XFS_ICHGTIME_CHG);
> >> +			xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);
> 
> >and same again - changing ctime on ip2 without logging it.
> 
> >> +		}
> >> +		xfs_trans_ichgtime(tp, dp2,
> >> +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +		xfs_trans_log_inode(tp, dp2, XFS_ILOG_CORE);
> >> +	}
> >> +	xfs_trans_ichgtime(tp, dp1, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +	xfs_trans_log_inode(tp, dp1, XFS_ILOG_CORE);
> 
> >perhaps:
> 
> >	int	ip1_flags = 0;
> >	int	ip2_flags = 0;
> >	int	dp2_flags = 0;
> >
> >	if (dp1 != dp2)
> >		dp2_flags = XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >
> >		if (S_ISDIR(ip1)) {
> >			ip1_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >			ip2_flags |= XFS_ICHGTIME_CHG; /* because ... */
> >			.....
> >		}
> >
> >		if (S_ISDIR(ip2)) {
> >			ip1_flags |= XFS_ICHGTIME_CHG; /* because ... */
> >			ip2_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >			.....
> >		}
> >	}
> >
> >	if (ip1_flags) {
> >		xfs_trans_ichgtime(tp, ip1, ip1_flags);
> >		xfs_trans_log_inode(tp, ip1);
> >	}
> >	if (ip2_flags) {
> >		xfs_trans_ichgtime(tp, ip2, ip2_flags);
> >		xfs_trans_log_inode(tp, ip2);
> >	}
> >	if (dp2_flags) {
> >		xfs_trans_ichgtime(tp, dp2, ip2_flags);
> >		xfs_trans_log_inode(tp, dp2);
> >	}
> >	xfs_trans_ichgtime(tp, dp2, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >	xfs_trans_log_inode(tp, dp2);
> 
> I don't think that these extra variables will actually clear the code and make
> it more readable IMHO, calling xfs_trans_ichgtime with the flags directly looks
> more clear and readable to me.
> 
> Thanks for the review,
> I have fixed all the another points you mentioned now, I think that handling log
> correct on the above snippet is the last thing to do by now.
> 
> cheers.
> 
> -- 
> Carlos
> 
> _______________________________________________
> xfs mailing list
> xfs at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos