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...
> 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.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
