Messages in this thread | | | Date | Tue, 31 May 2022 14:15:02 +0200 | Subject | Re: [PATCH v3 2/3] fat: add renameat2 RENAME_EXCHANGE flag support | From | Javier Martinez Canillas <> |
| |
Hello OGAWA,
Thanks a lot for your feedback and comments.
On 5/31/22 11:51, OGAWA Hirofumi wrote: > 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? >
Your changes make a lot of sense to me and from a quick glance they look OK to me. I'll look at them in more detail when merging the changes for a v4.
> Main purpose of me is to consolidate helpers with vfat_rename(), and > tweak coding style to use existent fat codes. >
Indeed. What do you think of the following plan for v4 ?
1) Keep patch "fat: add a vfat_rename2() and make existing .rename callback a helper" as the first patch of the series. 2) Add a patch #2 with your authorship that adds the helper and use them in the vfat_rename() function. 3) Make this patch "fat: add renameat2 RENAME_EXCHANGE flag support" to be patch #3 and use the helpers introduced in patch #2. 4) Make patch #4 to not only add a test for RENAME_EXCHANGE but also for renameat() and renameat2(..., RENAME_NOREPLACE). That way it will also cover your changes in patch #2.
>> + 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. >
Got it. I'm OK with that.
>> + /* 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. >
To be honest, I wasn't sure about this either but I saw that the implementation of RENAME_EXCHANGE in other filesystems did. For example btrfs_rename_exchange().
>> +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. >
I see. Thanks for the clarification.
-- Best regards,
Javier Martinez Canillas Linux Engineering Red Hat
| |