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