Re: [PATCH v3 2/3] fat: add renameat2 RENAME_EXCHANGE flag support

[Javier Martinez Canillas]

On 5/31/22 14:41, OGAWA Hirofumi wrote:
> Javier Martinez Canillas <javierm@xxxxxxxxxx> 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