Re: [GIT PULL] Fsnotify cleanups

From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [GIT PULL] Fsnotify cleanups
Date: Mon, 11 Jun 2018 19:38:38 +0300	[thread overview]
Message-ID: <CAOQ4uxg+-oiY-ykLhe4+nt7QeP5oCp_4HNgGOPZ-8GVJ-nod7g@mail.gmail.com> (raw)
In-Reply-To: <20180611160311.dsehobeuvwiq2j6c@quack2.suse.cz>

On Mon, Jun 11, 2018 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
> On Mon 11-06-18 16:58:14, Amir Goldstein wrote:
>> On Mon, Jun 11, 2018 at 4:36 PM, Jan Kara <jack@suse.cz> wrote:
>> > > I reworked the cleanup patches to get rid of fsnotify_obj and pushed to:
>> > > https://github.com/amir73il/linux.git fsnotify-cleanup
>> >
>> > Thanks!
>> >
>> > > Only last 5 patches from fsnotify_for_v4.18-rc1 have been modified
>> > > and I removed your S-O-B from the modified patches.
>> > >
>> > > This leaves struct inode unchanged, in fact no changes to code outside
>> > > fsnotify/audit at all.
>> > >
>> > > mask is now a member of connector for the purpose of generalizing
>> > > add/remove mark, but struct inode/mount still have a copy of the mask
>> > > for the purpose of the VFS optimizations.
>> >
>> > Looking through those patches, is it really beneficial to add mask to
>> > connector when you keep it in inode / vfsmount? A helper function to get
>> > mask from connector would make the same refactoring possible as well, won't
>> > it?
>> >
>> > And adding a helper function to set mask given connector would get rid of
>> > the remaining checks for connector type due to mask manipulations...
>> >
>>
>> By moving the checks for object type into the helper?
>
> Yes, that's what I meant.
>
>> Anyway, my thinking was:
>>
>> What do we have to loose from keeping the mask also inside the connector?
>>
>> Not much. We didn't change the size of connector struct
>> and it hardly adds any complexity / performance cost.
>
> You've actually grown the connector by 1 long on x86_64 - spinlock_t is
> just 4 bytes there. Also it seems a bit stupid to me to have the mask in
> two places (connector & object) just to save ifs in two helper functions.
>

Oh! I had CONFIG_DEBUG_LOCK_ALLOC on so hadn't noticed that.

>> What do we have to gain from keeping the mask also inside the connector?
>>
>> We can later get rid of the copy of mask in inode struct as I wrote.
>> I will follow up on that.
>
> If we can get rid of the mask in inode, I'm definitely fine with moving the
> mask to the connector.
>

I'll rework mask back out of connector and will bring that back later if that
makes sense.

BTW, found a way to shave 8 bytes from struct inode as well as from
struct file on x86_64 by shrinking _write_hint to u8 and moving it around.
Will test and send an RFC patch.

Thanks,
Amir.