Re: [PATCH v6 1/4] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

From: "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Christian Brauner <brauner@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"david@fromorbit.com" <david@fromorbit.com>,
	"djwong@kernel.org" <djwong@kernel.org>,
	"jlayton@kernel.org" <jlayton@kernel.org>
Subject: Re: [PATCH v6 1/4] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip
Date: Tue, 26 Apr 2022 01:22:19 +0000	[thread overview]
Message-ID: <626757B2.70002@fujitsu.com> (raw)
In-Reply-To: <Ymbbg3XbN17l3Jir@casper.infradead.org>

on 2022/4/26 1:33, Matthew Wilcox wrote:
> On Mon, Apr 25, 2022 at 01:29:47PM +0200, Christian Brauner wrote:
>> On Mon, Apr 25, 2022 at 03:08:36AM +0000, xuyang2018.jy@fujitsu.com wrote:
>>> on 2022/4/25 10:45, Matthew Wilcox wrote:
>>>> On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
>>>>> This has no functional change. Just create and export inode_sgid_strip
>>>>> api for the subsequent patch. This function is used to strip inode's
>>>>> S_ISGID mode when init a new inode.
>>>>
>>>> Why would you call this inode_sgid_strip() instead of
>>>> inode_strip_sgid()?
>>>
>>> Because I treated "inode sgid(inode's sgid)" as a whole.
>>>
>>> inode_strip_sgid sounds also ok, but now seems strip_inode_sgid seem
>>> more clear because we strip inode sgid depend on not only inode's
>>> condition but also depend on parent directory's condition.
>>>
>>> What do you think about this?
>>>
>>> ps: I can aceept the above several way, so if you insist, I can change
>>> it to inode_strip_sgid.
>>
>> I agree with Willy. I think inode_strip_sgid() is better. It'll be in
>> good company as<object>_<verb>_<what?>  is pretty common:
>>
>> inode_update_atime()
>> inode_init_once()
>> inode_init_owner()
>> inode_init_early()
>> inode_add_lru()
>> inode_needs_sync()
>> inode_set_flags()
>>
>> Maybe mode_remove_sgid() is even better because it makes it clear that
>> the change happens to @mode and not @dir. But I'm fine with
>> inode_strip_sgid() or inode_remove_sgid() too.
>
> Oh!  Yes, mode_strip_sgid() is better.  We're operating on the mode,
> not the inode.

OK, I will use mode_strip_sgid().