Messages in this thread | | | Date | Tue, 31 May 2022 14:46:05 +0200 | Subject | Re: [PATCH v3 2/3] fat: add renameat2 RENAME_EXCHANGE flag support | From | Javier Martinez Canillas <> |
| |
On 5/31/22 14:41, OGAWA Hirofumi wrote: > Javier Martinez Canillas <javierm@redhat.com> writes: > >>> 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. > > I don't care much about it because whole is not big (in short, I'm ok > with even one patch), so the point is the patches should be able to > bisect easily if separated. >
Yes, git bisect-ability is why I mentioned that we could do it in separate patches but I'll integrate your changes now and see what approach I take depending on how the code looks then.
>>>> + /* 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(). > > Ok. If I'm not overlooking, it looks like only btrfs. Please remove > those inode_inc_iversion() for {new,old}_inode. >
Sure.
-- Best regards,
Javier Martinez Canillas Linux Engineering Red Hat
| |