On Tue, Nov 26, 2013 at 10:51:25AM +0100, Jan Kara wrote:
> On Wed 20-11-13 14:01:52, Miklos Szeredi wrote:
> > From: Miklos Szeredi <mszeredi@xxxxxxx>
> >
> > Implement RENAME_EXCHANGE flag in renameat2 syscall.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxx>
> > ---
> > fs/ext4/namei.c | 97 ++++++++++++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 69 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> > index d258b354b937..5307e482f403 100644
> > --- a/fs/ext4/namei.c
> > +++ b/fs/ext4/namei.c
> ...
> > - old.dir->i_ctime = old.dir->i_mtime = ext4_current_time(old.dir);
> > - ext4_update_dx_flag(old.dir);
> > + /* S_ISDIR(old.inode->i_mode */
> > if (old.dir_bh) {
> > retval = ext4_rename_dir_finish(handle, &old, new.dir->i_ino);
> > if (retval)
> > goto end_rename;
> >
> > - ext4_dec_count(handle, old.dir);
> > - if (new.inode) {
> > - /* checked empty_dir above, can't have another parent,
> > - * ext4_dec_count() won't work for many-linked dirs */
> > - clear_nlink(new.inode);
> > - } else {
> > + if (!(flags & RENAME_EXCHANGE) || !S_ISDIR(new.inode->i_mode))
> > + ext4_dec_count(handle, old.dir);
> > +
> > + if (!new.inode || !S_ISDIR(new.inode->i_mode)) {
> > ext4_inc_count(handle, new.dir);
> > ext4_update_dx_flag(new.dir);
> > ext4_mark_inode_dirty(handle, new.dir);
> > }
> > }
> > + /* (flags & RENAME_EXCHANGE) && S_ISDIR(new.inode->i_mode */
> > + if (new.dir_bh) {
> > + retval = ext4_rename_dir_finish(handle, &new, old.dir->i_ino);
> > + if (retval)
> > + goto end_rename;
> > +
> > + if (!S_ISDIR(old.inode->i_mode)) {
> > + ext4_dec_count(handle, new.dir);
> > + ext4_inc_count(handle, old.dir);
> > + ext4_mark_inode_dirty(handle, new.dir);
> > + }
> > + }
> > ext4_mark_inode_dirty(handle, old.dir);
> > - if (new.inode) {
> > + if (!(flags & RENAME_EXCHANGE) && new.inode) {
> > + ext4_dec_count(handle, new.inode);
> > + new.inode->i_ctime = ext4_current_time(new.inode);
> > + if (S_ISDIR(old.inode->i_mode)) {
> > + /* checked empty_dir above, can't have another parent,
> > + * ext4_dec_count() won't work for many-linked dirs */
> > + clear_nlink(new.inode);
> > + }
> This hunk looks strange. Why do you check S_ISDIR(old.inode->i_mode)? I'd
> presume we need to clear nlink if new.inode is a directory...
It's confusing, that's for sure. I think it's correct, since S_ISDIR(old) is
equivalent to S_ISDIR(new) if not cross-renaming, but that's not a lot of
consolation to someone trying to understand the code.
>
> > ext4_mark_inode_dirty(handle, new.inode);
> > if (!new.inode->i_nlink)
> > ext4_orphan_add(handle, new.inode);
> Generally, I'm a bit unhappy about the number of various RENAME_EXCHANGE
> checks and the asymmetry between new & old which now shouldn't needed (that
> much). Especially the link count handling looks more complex than it should
> be.
>
> I'd hope that it should be possible to "delete new.inode iff
> !RENAME_EXCHANGE" and then the rest shouldn't need to care about
> RENAME_EXCHANGE at all and treat old & new completely symmetrically... Now I
> realize this isn't that easy because we want to do all error checks first
> before doing any changes on disk but still I hope some improvement can be
> made (maybe just zero out new.inode in our 'new' ext4_renament to allow for
> code to be symmetric and delete it on disk only when ext4_rename() is
> finishing).
>
> If the above won't be workable, we might at least make the link count
> handling more obvious by computing "old_link_cnt_update,
> new_link_cnt_update" - how link counts of parent dirs should be updated
> (-1, 0, +1) and then do the checks and updates based on this in one place.
Okay, will try to clean this up. I agree that it became a bit too complicated.
Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
