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

[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@xxxxxxxxxx> 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