Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <0ca7d264-2522-c820-d26e-19b6685d5016@redhat.com>
Date:   Tue, 31 May 2022 14:15:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v3 2/3] fat: add renameat2 RENAME_EXCHANGE flag support
Content-Language: en-US
To:     OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Cc:     linux-kernel@vger.kernel.org,
        Christian Kellner <ckellner@redhat.com>,
        Muhammad Usama Anjum <usama.anjum@collabora.com>,
        Alexander Larsson <alexl@redhat.com>,
        Alberto Ruiz <aruiz@redhat.com>,
        Peter Jones <pjones@redhat.com>,
        Lennart Poettering <lennart@poettering.net>,
        Colin Walters <walters@verbum.org>,
        Chung-Chiang Cheng <cccheng@synology.com>
References: <20220526134119.242182-1-javierm@redhat.com>
 <20220526134119.242182-3-javierm@redhat.com>
 <87ilpmows4.fsf@mail.parknet.co.jp>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <87ilpmows4.fsf@mail.parknet.co.jp>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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