Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9d837076-f666-c3c3-dd09-5c3705da23e6@redhat.com>
Date:   Thu, 9 Jun 2022 22:36:21 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v5 3/4] 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, Peter Jones <pjones@redhat.com>,
        Alexander Larsson <alexl@redhat.com>,
        Colin Walters <walters@verbum.org>,
        Alberto Ruiz <aruiz@redhat.com>,
        Muhammad Usama Anjum <usama.anjum@collabora.com>,
        Lennart Poettering <lennart@poettering.net>,
        Chung-Chiang Cheng <cccheng@synology.com>,
        Christian Kellner <ckellner@redhat.com>,
        Carlos Maiolino <cmaiolin@redhat.com>
References: <20220609093638.664034-1-javierm@redhat.com>
 <20220609093638.664034-4-javierm@redhat.com>
 <878rq54pf1.fsf@mail.parknet.co.jp>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <878rq54pf1.fsf@mail.parknet.co.jp>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello OGAWA,

Thanks for your feedback.

On 6/9/22 21:17, OGAWA Hirofumi wrote:
> Javier Martinez Canillas <javierm@redhat.com> writes:
> 
>> +static void vfat_update_nlink(struct inode *dir, struct inode *inode)
>> +{
>> +		if (S_ISDIR(inode->i_mode))
>> +			drop_nlink(dir);
>> +		else
>> +			inc_nlink(dir);
>> +}
> 
> [...]
> 
>> +	vfat_update_dir_metadata(old_dir, &ts);
>> +	/* if directories are not the same, update new_dir as well */
>> +	if (old_dir != new_dir) {
>> +		vfat_update_dir_metadata(new_dir, &ts);
>> +		/* nlink only needs to be updated if the file types differ */
>> +		if (old_inode->i_mode != new_inode->i_mode) {
>> +			vfat_update_nlink(old_dir, old_inode);
>> +			vfat_update_nlink(new_dir, new_inode);
>> +		}
>> +	}
> 
> Looks like unnecessary complex (and comparing raw i_mode, not S_ISDIR(),
> better to change before make dir dirty).  How about this change, it is
> only tested slightly though? Can you review and test?
>

Your change looks good to me and indeed the logic is simpler than in mine.

I've also tested it and AFAICT it works correctly as well. Do you plan to
squash this or should I respin a new revision of the whole patch-set ? If
you want to post it as a follow-up I'm also OK with that.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat