Messages in this thread Patch in this message | | | From | OGAWA Hirofumi <> | Subject | Re: [PATCH v3 2/3] fat: add renameat2 RENAME_EXCHANGE flag support | Date | Tue, 31 May 2022 18:51:07 +0900 |
| |
Javier Martinez Canillas <javierm@redhat.com> writes:
> The renameat2 RENAME_EXCHANGE flag allows to atomically exchange two paths > but is currently not supported by the Linux vfat filesystem driver. > > Add a vfat_rename_exchange() helper function that implements this support. > > The super block lock is acquired during the operation to ensure atomicity, > and in the error path actions made are reversed also with the mutex held. > > It makes the operation as transactional as possible, within the limitation > impossed by vfat due not having a journal with logs to replay.
I tweaked your patch (tested only slightly), can you review and merge to this patch if ok?
Main purpose of me is to consolidate helpers with vfat_rename(), and tweak coding style to use existent fat codes.
> + if (old_dir != new_dir) { > + err = vfat_get_dotdot_info(old_inode, &old_dotdot_bh, &old_dotdot_de); > + if (err) > + goto out; > + > + err = vfat_get_dotdot_info(new_inode, &new_dotdot_bh, &new_dotdot_de); > + if (err) > + goto out;
This should not return -ENOENT here. I tweaked to return -EIO in my patch.
> + /* update inode version and timestamps */ > + inode_inc_iversion(old_inode); > + inode_inc_iversion(new_inode);
Why do we need to update iversion of those inodes? I couldn't get intent of this.
> +error_new_dotdot: > + /* data cluster is shared, serious corruption */
Sharing data cluster would happen here only if one inode success to sync and another one failed. Then so I/O error, we would not be able to do much for it.
Thanks. -- OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> --- fs/fat/namei_vfat.c | 205 +++++++++++++++++++++------------------------------ 1 file changed, 87 insertions(+), 118 deletions(-)
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c index 3e3cd4f..84f95eb 100644 --- a/fs/fat/namei_vfat.c 2022-05-31 16:53:56.987009983 +0900 +++ b/fs/fat/namei_vfat.c 2022-05-31 18:37:30.893473188 +0900 @@ -893,16 +893,55 @@ out: return err; } +static int vfat_get_dotdot_de(struct inode *inode, struct buffer_head **bh, + struct msdos_dir_entry **de) +{ + if (S_ISDIR(inode->i_mode)) { + if (fat_get_dotdot_entry(inode, bh, de)) + return -EIO; + } + return 0; +} + +static int vfat_sync_ipos(struct inode *dir, struct inode *inode) +{ + if (IS_DIRSYNC(dir)) + return fat_sync_inode(inode); + mark_inode_dirty(inode); + return 0; +} + +static int vfat_update_dotdot_de(struct inode *dir, struct inode *inode, + struct buffer_head *dotdot_bh, + struct msdos_dir_entry *dotdot_de) +{ + fat_set_start(dotdot_de, MSDOS_I(dir)->i_logstart); + mark_buffer_dirty_inode(dotdot_bh, inode); + if (IS_DIRSYNC(dir)) + return sync_dirty_buffer(dotdot_bh); + return 0; +} + +static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts) +{ + inode_inc_iversion(dir); + fat_truncate_time(dir, ts, S_CTIME | S_MTIME); + if (IS_DIRSYNC(dir)) + (void)fat_sync_inode(dir); + else + mark_inode_dirty(dir); +} + static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry, struct inode *new_dir, struct dentry *new_dentry) { struct buffer_head *dotdot_bh; - struct msdos_dir_entry *dotdot_de; + struct msdos_dir_entry *dotdot_de = NULL; struct inode *old_inode, *new_inode; struct fat_slot_info old_sinfo, sinfo; struct timespec64 ts; loff_t new_i_pos; - int err, is_dir, update_dotdot, corrupt = 0; + int err, is_dir, corrupt = 0; struct super_block *sb = old_dir->i_sb; old_sinfo.bh = sinfo.bh = dotdot_bh = NULL; @@ -913,15 +952,13 @@ static int vfat_rename(struct inode *old if (err) goto out; - is_dir = S_ISDIR(old_inode->i_mode); - update_dotdot = (is_dir && old_dir != new_dir); - if (update_dotdot) { - if (fat_get_dotdot_entry(old_inode, &dotdot_bh, &dotdot_de)) { - err = -EIO; + if (old_dir != new_dir) { + err = vfat_get_dotdot_de(old_inode, &dotdot_bh, &dotdot_de); + if (err) goto out; - } } + is_dir = S_ISDIR(old_inode->i_mode); ts = current_time(old_dir); if (new_inode) { if (is_dir) { @@ -942,21 +979,15 @@ static int vfat_rename(struct inode *old fat_detach(old_inode); fat_attach(old_inode, new_i_pos); - if (IS_DIRSYNC(new_dir)) { - err = fat_sync_inode(old_inode); + err = vfat_sync_ipos(new_dir, old_inode); + if (err) + goto error_inode; + + if (dotdot_de) { + err = vfat_update_dotdot_de(new_dir, old_inode, dotdot_bh, + dotdot_de); if (err) - goto error_inode; - } else - mark_inode_dirty(old_inode); - - if (update_dotdot) { - fat_set_start(dotdot_de, MSDOS_I(new_dir)->i_logstart); - mark_buffer_dirty_inode(dotdot_bh, old_inode); - if (IS_DIRSYNC(new_dir)) { - err = sync_dirty_buffer(dotdot_bh); - if (err) - goto error_dotdot; - } + goto error_dotdot; drop_nlink(old_dir); if (!new_inode) inc_nlink(new_dir); @@ -966,12 +997,7 @@ static int vfat_rename(struct inode *old old_sinfo.bh = NULL; if (err) goto error_dotdot; - inode_inc_iversion(old_dir); - fat_truncate_time(old_dir, &ts, S_CTIME|S_MTIME); - if (IS_DIRSYNC(old_dir)) - (void)fat_sync_inode(old_dir); - else - mark_inode_dirty(old_dir); + vfat_update_dir_metadata(old_dir, &ts); if (new_inode) { drop_nlink(new_inode); @@ -991,10 +1017,9 @@ error_dotdot: /* data cluster is shared, serious corruption */ corrupt = 1; - if (update_dotdot) { - fat_set_start(dotdot_de, MSDOS_I(old_dir)->i_logstart); - mark_buffer_dirty_inode(dotdot_bh, old_inode); - corrupt |= sync_dirty_buffer(dotdot_bh); + if (dotdot_de) { + corrupt |= vfat_update_dotdot_de(old_dir, old_inode, dotdot_bh, + dotdot_de); } error_inode: fat_detach(old_inode); @@ -1021,66 +1046,18 @@ error_inode: goto out; } -/* Helpers for vfat_rename_exchange() */ - -static int vfat_get_dotdot_info(struct inode *inode, struct buffer_head **dotdot_bh, - struct msdos_dir_entry **dotdot_de) -{ - if (!S_ISDIR(inode->i_mode)) - return 0; - - return fat_get_dotdot_entry(inode, dotdot_bh, dotdot_de); -} - -static void vfat_exchange_dentries(struct inode *old_inode, struct inode *new_inode, - loff_t old_i_pos, loff_t new_i_pos) +static void vfat_exchange_ipos(struct inode *old_inode, struct inode *new_inode, + loff_t old_i_pos, loff_t new_i_pos) { fat_detach(old_inode); fat_detach(new_inode); - fat_attach(old_inode, new_i_pos); fat_attach(new_inode, old_i_pos); } -static int vfat_sync_after_exchange(struct inode *dir, struct inode *inode) -{ - int err = 0; - - if (IS_DIRSYNC(dir)) - err = fat_sync_inode(inode); - else - mark_inode_dirty(inode); - - return err; -} - -static int vfat_update_dotdot_info(struct buffer_head *dotdot_bh, struct msdos_dir_entry *dotdot_de, - struct inode *dir, struct inode *inode) -{ - int err = 0; - - fat_set_start(dotdot_de, MSDOS_I(dir)->i_logstart); - mark_buffer_dirty_inode(dotdot_bh, inode); - - if (IS_DIRSYNC(dir)) - err = sync_dirty_buffer(dotdot_bh); - - return err; -} - -static void vfat_update_dir_metadata(struct inode *dir, struct timespec64 *ts) -{ - inode_inc_iversion(dir); - fat_truncate_time(dir, ts, S_CTIME | S_MTIME); - - if (IS_DIRSYNC(dir)) - (void)fat_sync_inode(dir); - else - mark_inode_dirty(dir); -} - -static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry, - struct inode *new_dir, struct dentry *new_dentry) +static int +vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry, + struct inode *new_dir, struct dentry *new_dentry) { struct buffer_head *old_dotdot_bh = NULL, *new_dotdot_bh = NULL; struct msdos_dir_entry *old_dotdot_de = NULL, *new_dotdot_de = NULL; @@ -1098,11 +1075,13 @@ static int vfat_rename_exchange(struct i /* if directories are not the same, get ".." info to update */ if (old_dir != new_dir) { - err = vfat_get_dotdot_info(old_inode, &old_dotdot_bh, &old_dotdot_de); + err = vfat_get_dotdot_de(old_inode, &old_dotdot_bh, + &old_dotdot_de); if (err) goto out; - err = vfat_get_dotdot_info(new_inode, &new_dotdot_bh, &new_dotdot_de); + err = vfat_get_dotdot_de(new_inode, &new_dotdot_bh, + &new_dotdot_de); if (err) goto out; } @@ -1110,45 +1089,41 @@ static int vfat_rename_exchange(struct i old_i_pos = MSDOS_I(old_inode)->i_pos; new_i_pos = MSDOS_I(new_inode)->i_pos; - /* exchange the two dentries */ - vfat_exchange_dentries(old_inode, new_inode, old_i_pos, new_i_pos); + vfat_exchange_ipos(old_inode, new_inode, old_i_pos, new_i_pos); - err = vfat_sync_after_exchange(old_dir, new_inode); + err = vfat_sync_ipos(old_dir, new_inode); if (err) goto error_exchange; - - err = vfat_sync_after_exchange(new_dir, old_inode); + err = vfat_sync_ipos(new_dir, old_inode); if (err) goto error_exchange; /* update ".." directory entry info */ if (old_dotdot_de) { - err = vfat_update_dotdot_info(old_dotdot_bh, old_dotdot_de, new_dir, old_inode); + err = vfat_update_dotdot_de(new_dir, old_inode, old_dotdot_bh, + old_dotdot_de); if (err) goto error_old_dotdot; - drop_nlink(old_dir); inc_nlink(new_dir); } if (new_dotdot_de) { - err = vfat_update_dotdot_info(new_dotdot_bh, new_dotdot_de, old_dir, new_inode); + err = vfat_update_dotdot_de(old_dir, new_inode, new_dotdot_bh, + new_dotdot_de); if (err) goto error_new_dotdot; - drop_nlink(new_dir); inc_nlink(old_dir); } - /* update inode version and timestamps */ inode_inc_iversion(old_inode); inode_inc_iversion(new_inode); - vfat_update_dir_metadata(old_dir, &ts); - /* if directories are not the same, update new_dir as well */ if (old_dir != new_dir) vfat_update_dir_metadata(new_dir, &ts); + out: brelse(old_dotdot_bh); brelse(new_dotdot_bh); @@ -1157,30 +1132,21 @@ out: return err; error_new_dotdot: - /* data cluster is shared, serious corruption */ - corrupt = 1; - if (new_dotdot_de) { - corrupt |= vfat_update_dotdot_info(new_dotdot_bh, new_dotdot_de, - new_dir, new_inode); + corrupt |= vfat_update_dotdot_de(new_dir, new_inode, + new_dotdot_bh, new_dotdot_de); } error_old_dotdot: - /* data cluster is shared, serious corruption */ - corrupt = 1; - if (old_dotdot_de) { - corrupt |= vfat_update_dotdot_info(old_dotdot_bh, old_dotdot_de, - old_dir, old_inode); + corrupt |= vfat_update_dotdot_de(old_dir, old_inode, + old_dotdot_bh, old_dotdot_de); } error_exchange: - vfat_exchange_dentries(old_inode, new_inode, new_i_pos, old_i_pos); - - if (corrupt) { - corrupt |= fat_sync_inode(old_inode); - corrupt |= fat_sync_inode(new_inode); - } + vfat_exchange_ipos(old_inode, new_inode, new_i_pos, old_i_pos); + corrupt |= vfat_sync_ipos(new_dir, new_inode); + corrupt |= vfat_sync_ipos(old_dir, old_inode); if (corrupt < 0) { fat_fs_error(new_dir->i_sb, @@ -1190,15 +1156,18 @@ error_exchange: goto out; } -static int vfat_rename2(struct user_namespace *mnt_userns, struct inode *old_dir, +static int vfat_rename2(struct user_namespace *mnt_userns, + struct inode *old_dir, struct dentry *old_dentry, struct inode *new_dir, struct dentry *new_dentry, unsigned int flags) { if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE)) return -EINVAL; - if (flags & RENAME_EXCHANGE) - return vfat_rename_exchange(old_dir, old_dentry, new_dir, new_dentry); + if (flags & RENAME_EXCHANGE) { + return vfat_rename_exchange(old_dir, old_dentry, + new_dir, new_dentry); + } /* VFS already handled RENAME_NOREPLACE, handle it as a normal rename */ return vfat_rename(old_dir, old_dentry, new_dir, new_dentry); _
| |