Re: [PATCH v5 10/17] fanotify: cache fsid in fsnotify_mark_connector

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	Matthew Bobrowski <mbobrowski@mbobrowski.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v5 10/17] fanotify: cache fsid in fsnotify_mark_connector
Date: Fri, 8 Feb 2019 11:15:10 +0100	[thread overview]
Message-ID: <20190208101510.GC6353@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxhT4GMtaAs=Fi6tu8Q_v2hUJ84swNDWwJ78wskCyT-XAw@mail.gmail.com>

On Thu 07-02-19 18:31:19, Amir Goldstein wrote:
> On Thu, Feb 7, 2019 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 10-01-19 19:04:37, Amir Goldstein wrote:
> > > For FAN_REPORT_FID, we need to encode fid with fsid of the filesystem on
> > > every event. To avoid having to call vfs_statfs() on every event to get
> > > fsid, we store the fsid in fsnotify_mark_connector on the first time we
> > > add a mark and on handle event we use the cached fsid.
> > >
> > > Subsequent calls to add mark on the same object are expected to pass the
> > > same fsid, so the call will fail on cached fsid mismatch.
> > >
> > > If an event is reported on several mark types (inode, mount, filesystem),
> > > all connectors should already have the same fsid, so we use the cached
> > > fsid from the first connector.
> > >
> > > Suggested-by: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > I've somewhat modified the patch to simplify code flow around
> > fanotify_get_fid() and also made fsid argument for
> > fsnotify_add_mark_locked() mandatory. The resulting patch is attached.
> >
> 
> ACK. Two nits.
> 
> 1. Dan Carpenter reported a smatch warning. Please fix:
> 
> fs/notify/fanotify/fanotify.c:194 fanotify_encode_fid() error:
> uninitialized symbol 'type'.
> fs/notify/fanotify/fanotify.c:194 fanotify_encode_fid() error:
> uninitialized symbol 'err'.

Yes, but this problem went away with my changes as well since we do 'goto
out_err' only after setting err and type.

> 2. checkpatch doesn't like cutting long debug strings into 80 chars
> (and Greg's linuxsty.vim plugin doesn't warn about them)
> referring to the long string that you broke up
> pr_warn_ratelimited("%s: fsid mismatch on object of type %u: "...

I know and I don't take that too seriously. The point is: You don't want to
break the string so that it is easily greppable. But as long as the core
part of the string is in one line, it's OK. In particular once you get to
the part of the format string where you have the format specifiers,
breaking the string is irrelevant for grepping anymore since nobody can
sanely grep that part of the string. And I took care to follow these
guidances.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR