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.
Comments are for explaining why you did something. If you are using
the same reasoning as xfs_rename (i.e. the "We always want to hit
the ctime on the source inode" comment), then the comment should be
present here explaining the reason for the ctime change.
> I'm not sure if I should log it too (and add a xfs_trans_log_inode for
> ip1).
If the code modifies the inode, it also needs to log it.
> 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 :-(
You only need to log them once in a transaction, hence my
suggestion of using variables to track the timstamp changes required
by the cross rename followed by function exit logic that takes
action on all those modifications.
....
> > 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.
I find it much more informative that a random smattering of repeated
calls to xfs_trans_ichgtime/xfs_trans_log_inode throughout the
function. There's a clear separation of intent (i.e. what needs to
be changed in what branch) from action (the actual timestamp
changes and logging) and so is going to be much easier to understand
in a couple of years time. It's also more efficient code because we
only update timestamps once for each inode.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
