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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
